sparklemotion / nokogiri

Nokogiri (鋸) makes it easy and painless to work with XML and HTML from Ruby.
https://nokogiri.org/
MIT License
6.14k stars 897 forks source link

class cache not compatible with multiple JRuby Scripting containers #852

Closed munkyboy closed 10 years ago

munkyboy commented 11 years ago

When running multiple JRuby containers in the same JVM with any context scope besides SINGLETON, NokogiriHelpers.getNokogiriClass returns classes from the latest scripting container. In practice, Nokogiri::XML::Element#xpath is returning empty results because extract_params is comparing two different copies of Ruby classes.

Here's a script which illustrates the issue:

#!/usr/bin/env jruby
require 'rubygems'; gem 'nokogiri', '=1.5.7.rc1'; require 'nokogiri'
puts Nokogiri::XML("<foo/>").root.send(:extract_params, [".//*"]).inspect

require 'java'
sc = org.jruby.embed.ScriptingContainer.new(org.jruby.embed.LocalContextScope::THREADSAFE)
sc.runScriptlet("require 'rubygems'; gem 'nokogiri', '=1.5.7.rc1'; require 'nokogiri'")

puts Nokogiri::XML("<foo/>").root.send(:extract_params, [".//*"]).inspect

This was introduced in 1.5.0.beta.4. most likely commit c0445a892e93aa8503846d1afe486ebcd81b5592

munkyboy commented 11 years ago

Perhaps NokogiriService's class cache should be tied to the org.jruby.Ruby instance.

headius commented 10 years ago

There's a few paths here...

If you need thread- and JRuby instance-local variables, simply using RubyThread's thread-local storage would be sufficient. You'd get at current thread via ruby.getThreadContext().getThread() and set/get from there.

If you only need this to be JRuby instance-local, then stuffing the values into either hidden constants (probably under Nokogiri namespace somewhere) or "internal variables" (like instance vars on any Ruby object, but not visible to Ruby code), you will by definition be local to that JRuby instance. The logic there would be ruby.getConstant or getConstantFromPath, or someModule.getInternalVariables().getInternalVariable(name) and so on.

munkyboy commented 10 years ago

thanks for the response Charles. This patch has been working well for me... https://github.com/bigfix/nokogiri/commit/83df3a97c53cb439ff3e28614e8b770372a64b38

headius commented 10 years ago

@munkyboy That patch will work to fix the issue, but if any of the runtimes goes away you need extra work to clean up their entries in that static map. If you attach the map to the Ruby instance itself (via one of my described mechanisms) that would not be necessary.

Static fields are also subject to JVM classloading (they're basically classloader-global, not JVM-global) so under some servers they might stick around too long or not long enough.