java.lang.Iterable object has Enumerable ancestor but map cannot be called (jruby-9.3.x)

[klobuczek]

The anomaly described below happens on jruby-9.3.x. On jruby-9.2.19.0 everything works as expected.

I have a case where an anonymous object implementing java.lang.Iterable is returned from java, but under some circumstances, the Enumerable methods cannot be called on it although Enumerable is one of the ancestors of that object.
Example:

require 'neo4j_ruby_driver'
node = Neo4j::Driver::GraphDatabase.driver('bolt://localhost:7687', Neo4j::Driver::AuthTokens.basic('neo4j', 'pass')) do |driver|
  driver.session do |session|
    session.run('CREATE (t:Test{a: ["a", "b"]}) RETURN t').first.first
  end
end
node.properties # 1st call
require 'active_graph'
node.properties # 2nd call

There is quite a bit of prepending and including of ruby modules into java classes going on in both gems. If the 1st call is made everything looks good. But if the 1st call is commented out the error is:

NoMethodError:
  undefined method `map' for #<Java::OrgNeo4jDriverInternalValue::ListValue::1:0x26f7b114>
  Did you mean?  tap
# /Users/heinrich/mck/neo4j-ruby-driver/jruby/neo4j/driver/ext/ruby_converter.rb:12:in `as_ruby_object'
# /Users/heinrich/mck/neo4j-ruby-driver/jruby/neo4j/driver/ext/map_converter.rb:9:in `to_h'
# ./lib/active_graph/core/entity.rb:7:in `properties'
# ./spec/spec_helper.rb:16:in `<main>'

the caller side looks like this:

values(&:itself).map(&:as_ruby_object)

values(&:itself) returns an object of an anonymous class implementing java.lang.Iterable which has Enumerable as ancestor.

You can call the each method from Iterable and then chain map onto it and this works but that's just a workaround.

I am sorry for this vague description but this is a close as I can get after multiple hours of investigation.
I believe there are 2 problems:

1. Under some conditions the Enumerable methods are erased from anonymous Iterable
2. Some type of caching problem where the methods of the anonymous Iterable depend on the context in which the first instance of the anonymous class has been created.

I'm happy to do further debugging with your guidance, but right now I'm out of ideas. I was not able to create a minimal example to reproduce the issue.

[klobuczek]

I still don't have a small example but the problem can be easily reproduced:

gem install neo4j-java-driver
gem install activegraph
docker run --name neo4j --env NEO4J_AUTH=neo4j/pass -p7687:7687 --rm neo4j:4.3.6

and then in irb:

require 'neo4j_ruby_driver'
require 'active_graph'
Neo4j::Driver::GraphDatabase.driver('bolt://localhost:7687', Neo4j::Driver::AuthTokens.basic('neo4j', 'pass')) { |driver| driver.session { |session| session.run('CREATE (t:Test{a: ["a", "b"]}) RETURN t').first.first.properties } }

If the last line is additionally copied in between the 2 requires (in fresh irb) there is no exception for the remainder of the irb session and on jruby-9.2.19.0 there is no exception in any case.

The anonymous Iterable class can be found here: https://github.com/neo4j/neo4j-java-driver/blob/4.3/driver/src/main/java/org/neo4j/driver/internal/value/ListValue.java#L86-L113

[ikaronen-relex]

I've been struggling with what seems to be the same issue, and finally managed to reduce it down to a simple spec (loosely based on interfaces/iterable_spec.rb) that fails on JRuby 9.3.1.0:

describe "Classes that implement Iterable" do
  it "should provide #map even if Enumerable has been prepended to" do
    java.util.ArrayList.new  # ArrayList implements Iterable
    Enumerable.prepend(Module.new)

    # SQLException also implements Iterable, and probably has no Ruby class wrapper defined yet
    iterable = java.sql.SQLException.new 
    expect { iterable.map(&:to_s) }.not_to raise_exception
  end
end

(In my case, the actual trigger for this bug was the ActiveSupport::ToJsonWithActiveSupportEncoder module, which gets prepended to Enumerable when ActiveSupport is loaded.)

Ps. I suspect that this regression might be related to this comment in the IncludedModuleWrapper constructor:

// FIXME: Is this ok if we are including a prepended module?
methods = origin.getMethodsForWrite();

Pps. For the time being, the following work-around seems to restore the missing methods adequately:

# Klugy work-around for https://github.com/jruby/jruby/issues/6924
[Java::JavaLang::Iterable, Java::JavaUtil::Enumeration, Java::JavaUtil::Iterator, Java::JavaUtil::Collection].each do |mod|
  mod.include(Enumerable)
end

(FWIW, those are all the Java interfaces whose Ruby wrappers are defined, in JavaLang.java and JavaUtil.Java, to include Enumerable. Just patching Iterable alone is not sufficient — it leaves e.g. java.util.Collections.synchronizedList() broken — but the kluge above is at least enough to allow our rather extensive test suite to pass.)

[headius]

@ikaronen-relex Good investigation! So is something prepending on to Enumerable and breaking our inclusion logic?

@enebo Could this be related to changes you were making for looking up prepends and includes?

[ikaronen-relex]

So is something prepending on to Enumerable and breaking our inclusion logic?

@headius Yes, at least ActiveSupport does that, see link above. And apparently active_graph depends on ActiveSupport, so that's probably the trigger in @klobuczek's example, too.

[headius]

@ikaronen-relex Ok, great... so we have a cause and can dig into a solution.

[enebo]

Some start of notes on this I am using @ikaronen-relex example to work from. The FIXME probably is not the issue (I should have removed it) but that commit and a couple around it is definitely where the issue came from. The idea of swapping out methods is one of the bigger changes. It could be the problem but I will dig more.

My first observation is that ArrayList if it is moved after the prepend it works fine. An ArrayList before will break SQLException because of the prepend. Both SQLException and ArrayList have Iterable in its inheritance so I guess the prepend has to break Enumerable in Iterable in some way? What is strange is without the ArrayList the SQLException case will work again. What is even stranger is that if I reverse ArrayList and SQLException it works again as well.

[enebo]

More notes. When prepend(Module.new) it uses the literal Enumerable instance to modify. This isn't wrong but this will for sure muck with older things which still reference the method table of the original Enumerable. That is it will only muck with the users if they are actually using the actual method table of the original Enumerable.

So a couple of things worth pointing out:

module A
  def one
  end
end


class B
  include A
end

p B.instance_methods(true)

module A
  def two
  end
end

p B.instance_methods(true)

Ordinary method updates will be seen. Second call to instance_methods will see two.

If I change two to be in another module and include that into A then we will not see it:

module A
  def one
  end
end

class B
  include A
end

p B.instance_methods(true)

module C
  def beh
  end
end

module A
  include C
end

p B.instance_methods(true)

This is because when B did the include it resolves the inheritance chain at that point (note: this is not true in 3.0...well it is but 3.0 will revisit all previous includes and fix things up).

So the big problem here is that a prepend changes hierarchy and the current implementation will change the module to no methods then set the prependedmodule with the original methods. Since the older includes no nothing of this prepend they do not have the same inheritance so they do not see the methods in the new location. EXCEPT we have methodLocation which for Enumerable will be the prependedModule which contains the original methods.

If we are lucky then we are missing one piece of logic within Java integration which is somehow looking for a method and bypassing methodLocation. Otherwise I am off the map here and it is something else. The reason I think this is likely the issue is we have only seen this with Java classes we pull in.

[enebo]

Amusing but not done yet. That FIXME comment eerily did predict the issue. I made the comment and it was for a different concern yet it was the main cause of this regression:

methods = origin.getMethodsForWrite();

should be

methods = origin.getMethodLocation().getMethodsForWrite();

This makes sense because after a preperd all the methods have left Enumerable and went into a PrependedModule (horrible name for this). That prepended module is the method location for Enumerable. Simple....yet....

1)
Module#refine method lookup looks in prepended modules from the refinement first FAILED
Expected "foo from refinement" == "foo from prepended module"
to be truthy but was false
/home/enebo/work/jruby/spec/ruby/core/module/refine_spec.rb:319:in `block in <main>'

So there is a sole failure in both MRI and ruby/spec test suites from this fix invovling refinements. I can say with certainty the fix is the right one but refinements is a beast. So I will try and see why this no longer works (how many people prepend within refinements?).

[enebo]

@klobuczek @ikaronen-relex we worked on this from the perspective of what @ikaronen-relex submitted with Iterable/Enumerable. The cause was fairly clear that once something was prepended we were still trying to look to the original module instead of the place where we store the methods after a prepend takes place.