Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[bug] [jruby] RelaxNG.read_memory does not work #2115

Closed
flavorjones opened this issue Nov 27, 2020 · 0 comments · Fixed by #3259
Closed

[bug] [jruby] RelaxNG.read_memory does not work #2115

flavorjones opened this issue Nov 27, 2020 · 0 comments · Fixed by #3259
Labels
platform/jruby topic/xsd
Milestone
v1.17.0

Comments

@flavorjones
Copy link
Member

Please describe the bug

While working on an unrelated issue with schema parsing, I noticed that there was no test coverage of XML::RelaxNG.read_memory and that running a simple test case causes an exception to be raised.

Help us reproduce what you're seeing

#! /usr/bin/env ruby

require 'nokogiri'

Nokogiri::XML::RelaxNG.read_memory(File.read("./test/files/address_book.rlx"))

results, using Nokogiri v1.10.10, in:

IOError: No message available
  read_memory at nokogiri/XmlSchema.java:161

Expected behavior

Running this in CRuby parses the schema successfully.

Environment

# Nokogiri (1.10.10)
    ---
    warnings: []
    nokogiri: 1.10.10
    ruby:
      version: 2.5.7
      platform: java
      description: jruby 9.2.9.0 (2.5.7) 2019-10-30 458ad3e OpenJDK 64-Bit Server VM 11.0.9.1+1-Ubuntu-0ubuntu1.20.04
        on 11.0.9.1+1-Ubuntu-0ubuntu1.20.04 [linux-x86_64]
      engine: jruby
      jruby: 9.2.9.0
    xerces: Xerces-J 2.12.0
    nekohtml: NekoHTML 1.9.21

Additional context

Digging in on the type of exception raise, it's a java.net.MalformedURLException being raised by factory.compileSchema(is); in XmlRelaxng.java for a StreamSource source.
@flavorjones flavorjones added state/needs-triage Inbox for non-installation-related bug reports or help requests platform/jruby and removed state/needs-triage Inbox for non-installation-related bug reports or help requests labels Nov 27, 2020
@flavorjones flavorjones mentioned this issue Nov 28, 2020
Closed
flavorjones added a commit that referenced this issue Nov 28, 2020
@flavorjones
ci(jruby): skip new failing test for #2115
b1a7986 
and also raise a better-formatted exception in that case
flavorjones added a commit that referenced this issue Dec 3, 2020
@flavorjones
ci(jruby): skip new failing test for #2115
ee3a6aa 
and also raise a better-formatted exception in that case
@flavorjones flavorjones added this to the v1.13.0 milestone Aug 19, 2021
@flavorjones flavorjones mentioned this issue Aug 24, 2021
Closed
@flavorjones flavorjones modified the milestones: v1.13.0, v1.14.0 Jan 6, 2022
@flavorjones flavorjones modified the milestones: v1.14.0, v1.15.0 Aug 28, 2022
@flavorjones flavorjones modified the milestones: v1.15.0, v1.16.0 Apr 28, 2023
flavorjones added a commit that referenced this issue Jun 24, 2024
@flavorjones
fix: RelaxNG.read_memory is now reliable
4f5fb23 
On both JRuby and CRuby impls, replace the native `read_memory` method
from XML::Schema and XML::RelaxNG with a ruby method that uses
`from_document`. `read_memory` was buggy on both platforms, but
especially on the JRuby impl.

This is comparable in performance on CRuby. From a benchmark taken
before this change:

    Warming up --------------------------------------
              Schema.new     1.219k i/100ms
    Schema.from_document     1.258k i/100ms
      Schema.read_memory     1.118k i/100ms
    Calculating -------------------------------------
              Schema.new     12.160k (± 8.3%) i/s -    121.900k in  10.093638s
    Schema.from_document     12.216k (± 8.6%) i/s -    122.026k in  10.059696s
      Schema.read_memory     12.790k (±10.7%) i/s -    127.452k in  10.105931s

    Comparison:
      Schema.read_memory:    12789.6 i/s
    Schema.from_document:    12215.9 i/s - same-ish: difference falls within error
              Schema.new:    12160.1 i/s - same-ish: difference falls within error

IMHO the resulting code is less buggy and easier to maintain, and the
slight (if any) performance hit is worth it.

Closes #2113
Closes #2115
@flavorjones flavorjones mentioned this issue Jun 24, 2024
Merged
flavorjones added a commit that referenced this issue Jun 24, 2024
@flavorjones
fix: RelaxNG.read_memory is now reliable (#3259)
925f3be 
**What problem is this PR intended to solve?**

On both JRuby and CRuby impls, replace the native `read_memory` method
from XML::Schema and XML::RelaxNG with a ruby method that uses
`from_document`. `read_memory` was buggy on both platforms, but
especially on the JRuby impl.

This is comparable in performance on CRuby. From a benchmark taken
before this change:

```
Warming up --------------------------------------
          Schema.new     1.219k i/100ms
Schema.from_document     1.258k i/100ms
  Schema.read_memory     1.118k i/100ms
Calculating -------------------------------------
          Schema.new     12.160k (± 8.3%) i/s -    121.900k in  10.093638s
Schema.from_document     12.216k (± 8.6%) i/s -    122.026k in  10.059696s
  Schema.read_memory     12.790k (±10.7%) i/s -    127.452k in  10.105931s

Comparison:
  Schema.read_memory:    12789.6 i/s
Schema.from_document:    12215.9 i/s - same-ish: difference falls within error
          Schema.new:    12160.1 i/s - same-ish: difference falls within error
```

IMHO the resulting code is less buggy and easier to maintain, and the
slight (if any) performance hit is worth it.

Also: improve documentation for Schema and RelaxNG classes.

Closes #2113
Closes #2115


**Have you included adequate test coverage?**

Yes.


**Does this change affect the behavior of either the C or the Java
implementations?**

Brings JRuby and CRuby into alignment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
platform/jruby topic/xsd
Projects
None yet
Milestone
v1.17.0
Development

Successfully merging a pull request may close this issue.

fix: RelaxNG.read_memory is now reliable sparklemotion/nokogiri
1 participant
@flavorjones