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.

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

9.2.7.0 sometimes skips adding Enumerable to implementors of java.util.List #5748

Open
shibz opened this issue May 29, 2019 · 7 comments

Comments

Projects
None yet
2 participants
@shibz
Copy link

commented May 29, 2019

Environment

  • Tested with JRuby 9.2.7.0 as well as a few snapshot builds to narrow down the commit that introduced the bug
  • Linux 4.14.114-105.126.amzn2.x86_64 #1 SMP Tue May 7 02:26:40 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux

Expected Behavior

Expected behavior observed on 9.1.17.0, 9.2.6.0, and snapshots of 9.2.7.0 up-to (and including) commit 8a69eb6. Classes that implement the java.util.List interface expose the Enumerable methods such as #map. This example below demonstrates the behavior.

# Load java.util.Collection.  Must be done prior to loading java.util.List for
# the bug to manifest.
Java::JavaUtil::Collection

# Prepend a module (any module) to Enumerable
module TestModule; end
Enumerable.prepend(TestModule)

# Create a class that implements the java.util.List interface
class TestList
  include Java::JavaUtil::List
end

# Enumerator is created as expected
TestList.new.map
 => #<Enumerator: #<TestList:0x4d339552>:map>

Actual Behavior

Commit be0ada0 and later (including 9.2.7.0 release and the latest 9.2.8.0-SNAPSHOT from master) sometimes do NOT provide Enumerable behavior on classes implementing the java.util.List interface. However if either of the setup steps (Java::JavaUtil::Collection or Enumerable.prepend) in this reproduction are omitted, the Enumerable behavior works as expected on TestList.

# Load java.util.Collection.  Must be done prior to loading java.util.List for
# the bug to manifest.
Java::JavaUtil::Collection

# Prepend a module (any module) to Enumerable
module TestModule; end
Enumerable.prepend(TestModule)

# Now any Ruby class that implements the java.util.List interface will no
# longer have the Enumerable methods such as #map available.
class TestList
  include Java::JavaUtil::List
end

# map raises an error rather than returning an Enumerator
TestList.new.map
Traceback (most recent call last):
        6: from bin/irb:13:in `<main>'
        5: from org/jruby/RubyKernel.java:1192:in `catch'
        4: from org/jruby/RubyKernel.java:1192:in `catch'
        3: from org/jruby/RubyKernel.java:1424:in `loop'
        2: from org/jruby/RubyKernel.java:1060:in `eval'
        1: from (irb):17:in `evaluate'
NoMethodError (undefined method `map' for #<TestList:0x55a561cf>)
Did you mean?  tap

I am seeing impact from this bug when using require 'active_support/all' (version 5.1), which prepends a module to Enumerable.

@headius

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

What a peculiar bug!

@headius

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

This appears to be a bug in the lazy loading of these extensions. As a workaround you can disable that laziness by passing -Xji.load.lazy=false to JRuby.

I'm looking into it, but @kares might know what's wrong.

@headius

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

I stepped through the logic for setting up java.util.Collection, and at the point where it tries to include Enumerable, it walks Collection's hierarchy and finds Iterable there, which already has Enumerable. That causes it to not do a second include of Enumerable and I believe.

The sequence here may be at fault; includes of modules have some temporal qualities that could be getting in the way.

With the lazy logic in place, I believe some sequence like the following is happening:

  1. Through normal boot logic, Iterable gets set up earlier than Collection.
  2. Collection is accessed and proceeds to skip the Enumerable logic since it appears to already be in its hierarchy.
  3. Enumerable gets a prepend added, which changes its hierarchy and moves the actual Enumerable methods to a synthetic intermediate class.
  4. A Ruby class includes java.util.List, which should bring along Enumerable from one of the interfaces it descends (Collection, Iterable).

I believe the problem revolves around the Enumerable prepend moving methods around in a way that confuses the Java interface implementation logic. It may be that we're not handling prepended modules correctly there.

@headius

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

The logic here in RubyModule gathers all modules from the incoming include. With the prepended Enumerable, it seems to be skipping that item from the hierarchy:

    private List<RubyModule> gatherModules(RubyModule baseModule) {
        // build a list of all modules to consider for inclusion
        List<RubyModule> modulesToInclude = new ArrayList<RubyModule>();
        for (; baseModule != null; baseModule = baseModule.superClass) {
            // skip prepended roots
            if (baseModule != baseModule.getMethodLocation()) continue;

            modulesToInclude.add(baseModule.getDelegate());
        }

        return modulesToInclude;
    }

However the following script, which should be simulating the same rough sequence of events, appears to work just fine:

module Foo
  include Enumerable
end
module Bar
  include Foo
end
Enumerable.prepend(Module.new)
class Baz
  include Bar
end
Baz.new.map
@headius

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Ok nevermind, the script I posted does fail in JRuby and works properly in MRI. This may be the reproduction we need.

@shibz

This comment has been minimized.

Copy link
Author

commented Jun 4, 2019

Setting -Xji.load.lazy=false does stop the error from occurring when I perform the steps in my reproduction above, however I am still seeing the bug manifest in the actual application even with this flag set to false. For simplicity in this bug report I provided reproduction steps that were 100% Ruby so they could be tested through IRB, however in the actual application where I first noticed this bug, the class that isn't receiving the Ruby Enumerable methods is a Java class. Specifically this one.

It would seem that either the bug isn't actually in the ji.load.lazy behavior, or else there are actually two separate bugs here. Let me know if you need me to work up new reproduction steps that reproduce the issue even with ji.load.lazy=false.

@headius

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

@shibz Yeah the lazy load flag appears to avoid the issue in some cases, but the ultimate bug is in how we gather all the relevant modules to include when one of them has been prepended. The logic I showed above is clearly wrong for this case; just because Enumerable has been prepended (i.e. baseModule != baseModule.getMethodLocation) should not prevent it from being included.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.