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

Mutliple issues on jdk11 after ugrading to jRuby 9.2.9.0 #5969

Closed
klobuczek opened this issue Nov 9, 2019 · 11 comments
Closed

Mutliple issues on jdk11 after ugrading to jRuby 9.2.9.0 #5969

klobuczek opened this issue Nov 9, 2019 · 11 comments

Comments

@klobuczek
Copy link

@klobuczek klobuczek commented Nov 9, 2019

After upgrading to jruby-9.2.9.0 I have multiple test failures on jdk11.
jdk 1.8 and ruby-9.2.9.0 or jruby-9.2.9.0 works fine
jdk 11 and jruby-9.2.8.0 works fine too
It well may be my error but maybe you could give me a hint what change could affect it.

e.g.

  1) Parameters is able to set and return property is able to set and return Boolean property
     Failure/Error:
       session.run(query, value: parameter_processor(value)).each do |record|
         out_value = record['a.value']
         expect(block.call(value)).to be true
         expect(out_value).to eq value
       end

     TypeError:
       illegal access on 'forEach': class org.jruby.javasupport.JavaMethod (in module org.jruby.dist) cannot access a member of class java.util.stream.ReferencePipeline (in module java.base) with modifiers "public"
     # ./jruby/neo4j/driver/ext/internal_statement_result.rb:17:in `block in each'
     # ./jruby/neo4j/driver/ext/exception_checkable.rb:8:in `check'
     # ./jruby/neo4j/driver/ext/internal_statement_result.rb:17:in `each'
     # ./spec/integration/parameters_spec.rb:10:in `test_property'
     # ./spec/integration/parameters_spec.rb:25:in `block in <main>'
     # ./spec/support/neo4j_cleaner.rb:12:in `cleaning'
     # ./spec/spec_helper.rb:29:in `block in <main>'

You can see full details at https://travis-ci.com/neo4jrb/neo4j-ruby-driver/jobs/253161173

Environment

Provide at least:

  • JRuby version (jruby -v) and command line (flags, JRUBY_OPTS, etc) 9.2.9.0
  • Operating system and platform (e.g. uname -a)

Other relevant info you may wish to add:

  • Installed or activated gems
  • Application/framework version (e.g. Rails, Sinatra)
  • Environment variables

Expected Behavior

  • Describe your expectation of how JRuby should behave, perhaps by showing how CRuby/MRI behaves.
  • Provide an executable Ruby script or a link to an example repository.

Actual Behavior

  • Describe or show the actual behavior.
  • Provide text or screen capture showing the behavior.
@headius
Copy link
Member

@headius headius commented Nov 11, 2019

Thanks for the report! Things did indeed change regarding how we handle Java modules in JRuby 9.2.9.0...it's likely that we're avoiding a setAccessible call because it would produce a warning, but for whatever reason proceeding to make the call anyway and causing this error.

A workaround for you would be to add this JVM flag:

--add-opens java.base/java.util.stream=org.jruby.dist

This will open up that package for reflection.

I notice that the error seems to original within the neo4j Ruby driver, so this may be something that needs to be fixed upstream.

@headius
Copy link
Member

@headius headius commented Nov 11, 2019

Oh to be clear about the main change in 9.2.9... instead of eagerly setting Java classes and methods to be accessible, we now first query to see if the package is "open" for that sort of operation. The alternative is to go ahead and do it but produce warnings...which then get reported as bugs of a different sort. 🙄We're still trying to find a happy medium here.

@headius
Copy link
Member

@headius headius commented Nov 11, 2019

This looks to be a problem of selecting the wrong target method signature. We should be invoking the Stream interface's forEach, both of which are public. Instead we attempt to invoke ReferencePipeline class's version, but that class is not public. As a result we get an error, since we're trying to call the non-public class's version of the method.

There's logic in our method binding to prevent this for concrete classes (pick the superclass method that is actually public/public) but perhaps we're not doing that effectively when the super signature is on an interface.

@headius
Copy link
Member

@headius headius commented Nov 12, 2019

Reproduced with a trivial Spliterator implementation:

class MySplitter
  include java.util.Spliterator

  def tryAdvance consumer
    true
  end

  def trySplit
    nil
  end

  def characteristics
    0
  end
end

s = java.util.stream.StreamSupport.stream(MySplitter.new, false)

s.forEach {|x| p x}

Which yields:

$ jruby spliterator.rb 
TypeError: illegal access on 'forEach': class org.jruby.javasupport.JavaMethod (in module org.jruby.dist) cannot access a member of class java.util.stream.ReferencePipeline (in module java.base) with modifiers "public"
  <main> at spliterator.rb:19

@headius
Copy link
Member

@headius headius commented Nov 12, 2019

Confirmed the above --add-opens flag does remedy the error.

@headius
Copy link
Member

@headius headius commented Nov 12, 2019

I believe I see the problem.

In 02c5040 I removed logic we used to probe for a coarse-grained security restriction on setting classes accessible. Unfortunately I replaced it with a simple boolean.

As a result, the code at

if (Modifier.isPublic(klass.getModifiers()) || JavaUtil.CAN_SET_ACCESSIBLE) {
now always attempts to bind public methods on all classes, even if they are not public and their methods can't be set accessible. Because the methods themselves are public, we proceed to bind them, fail quietly to set them accessible, and later attempt to call them...which produces this error because the containing class is not public.

This would probably have been caught with better testing on Java 11. We need to remedy that for JRuby 9.2.10.

The fix is fairly trivial; go back to the original logic that only attempted to bind methods from public classes.

@kares @enebo A possibly better fix would be to go ahead and bind methods from non-public classes if their package is open to us, but I have worries about that causing non-public methods to get bind that should not be callable. It seems like for normal dispatch we may not want to go this far.

We probably do want to go this far for super invocation from subclasses, but this is only one of many problems with the current Java class extending logic in JRuby.

I will commit the simple fix for now.

@rsim
Copy link

@rsim rsim commented Nov 25, 2019

@headius We have a similar error in our application when using JRuby 9.2.9.0 with Java 11:
java.lang.management.ManagementFactory.getOperatingSystemMXBean.getTotalPhysicalMemorySize raises:

TypeError:
  illegal access on 'getTotalPhysicalMemorySize': class org.jruby.javasupport.JavaMethod (in module org.jruby.dist) cannot access class com.sun.management.internal.OperatingSystemImpl (in module jdk.management) because module jdk.management does not export com.sun.management.internal to module org.jruby.dist

This method call was working fine using JRuby 9.2.8.0 with Java 11.

Is the root cause of this the same as in this issue?

Did some further investigation and it seems that the method lookup on JRuby 9.2.9.0 is returning the wrong method:

jruby-9.2.9.0 > java.lang.management.ManagementFactory.getOperatingSystemMXBean.method(:getTotalPhysicalMemorySize)
 => #<Method: Java::ComSunManagementInternal::OperatingSystemImpl#getTotalPhysicalMemorySize>

But on JRuby 9.2.8.0 it was returning the method from a different class:

jruby-9.2.8.0 > java.lang.management.ManagementFactory.getOperatingSystemMXBean.method(:getTotalPhysicalMemorySize)
 => #<Method: Java::SunManagement::OperatingSystemImpl#getTotalPhysicalMemorySize>

@headius
Copy link
Member

@headius headius commented Nov 26, 2019

@rsim Yes, it does seem like the same issue. I did not commit because I made the change immediately before RubyConf and it did not pass a quick local test. I'll take care of that today and get it into a PR.

@rsim
Copy link

@rsim rsim commented Nov 26, 2019

@headius I just wanted to comment that I found a workaround that instead of

java.lang.management.ManagementFactory.getOperatingSystemMXBean.getTotalPhysicalMemorySize
# => TypeError (illegal access on 'getTotalPhysicalMemorySize': class org.jruby.javasupport.JavaMethod (in module org.jruby.dist) cannot access class com.sun.management.internal.OperatingSystemImpl (in module jdk.management) because module jdk.management does not export com.sun.management.internal to module org.jruby.dist)

I can use

os = java.lang.management.ManagementFactory.getOperatingSystemMXBean
method = os.java_class.interfaces.first.java_method "getTotalPhysicalMemorySize"
method.invoke(os)
# => 34359738368

As I see

os.java_class
# => class com.sun.management.internal.OperatingSystemImpl
os.java_class.interfaces.first
# => class com.sun.management.UnixOperatingSystemMXBean

and on Java 11 I cannot access classes and methods from the package com.sun.management.internal but I can access them via interface from the package com.sun.management.

@headius
Copy link
Member

@headius headius commented Dec 2, 2019

@rsim Good find! Thanks for reporting back.

@headius
Copy link
Member

@headius headius commented Dec 3, 2019

Fixed by #5984.

@headius headius closed this as completed Dec 3, 2019
@enebo enebo added this to the JRuby 9.2.10.0 milestone Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants