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

org.jruby.javasupport.JavaMethod invocation does not wrap returned JavaObject #2857

Closed
vietj opened this Issue Apr 21, 2015 · 36 comments

Comments

Projects
None yet
3 participants
@vietj
Copy link

vietj commented Apr 21, 2015

The Vert.x 3 Ruby lang integration uses java_class.declared_method for dispatching the Ruby Vert.x API to the Java Vert.x API.

The JavaMethod object does not wrap the returned objects instance of JavaObject, so such objects are not usable from JRuby as they are and must be rewrapped with their corresponding JRuby wrappers.

The current implementation extends the returned JavaMethod object to avoid such issues:

public class ConvertingJavaMethod extends JavaMethod {

  private final Class boxedReturnType;
  private final JavaUtil.JavaConverter returnConverter;

  public ConvertingJavaMethod(Ruby runtime, Method method) {
    super(runtime, method);
    method.setAccessible(true);
    if (method.getReturnType().isPrimitive() && method.getReturnType() != void.class) {
      this.boxedReturnType = CodegenUtils.getBoxType(method.getReturnType());
    } else {
      this.boxedReturnType = method.getReturnType();
    }
    returnConverter = JavaUtil.getJavaConverter(method.getReturnType());
  }

  @Override
  public IRubyObject invoke(IRubyObject[] args) {
    IRubyObject ret = super.invoke(args);
    if (ret instanceof JavaObject) {
      ret = convertReturn(((JavaObject) ret).getValue());
    }
    return ret;
  }

  private IRubyObject convertReturn(Object result) {
    if (result != null && result.getClass() != boxedReturnType) {
      // actual type does not exactly match method return type, re-get converter
      // FIXME: when the only autoconversions are primitives, this won't be needed
      return JavaUtil.convertJavaToUsableRubyObject(getRuntime(), result);
    }
    return JavaUtil.convertJavaToUsableRubyObjectWithConverter(getRuntime(), result, returnConverter);
  }
}

Such code is used in such way:

(Java::IoVertxLangJruby::Helper.fixJavaMethod(@j_del.java_class.declared_method(:runOnContext,Java::IoVertxCore::Handler.java_class))).invoke(@j_del,Proc.new { yield })

(the helper transforms the JavaMethod returned by declared_method to ConvertingJavaMethod)

The current code needs to use method.setAccessible(true) otherwise some Vert.x methods cannot be invoked leading to a runtime error like:

org.jruby.exceptions.RaiseException: (TypeError) illegal access on 'complete': Class org.jruby.javasupport.JavaMethod can not access a member of class io.vertx.core.impl.FutureImpl with modifiers "public"
    at org.jruby.javasupport.JavaMethod.invoke(org/jruby/javasupport/JavaMethod.java:257)
    at RUBY.complete(/Users/julien/java/vertx-lang-ruby/target/classes/vertx/future.rb:72)
@headius

This comment has been minimized.

Copy link
Member

headius commented Apr 21, 2015

JavaObject is a very old relic of the original Java integration subsystem that we almost never use internally anymore. That goes too for most of the Java* classes like JavaMethod, JavaField, and so on. We would strongly recommend they not be used.

If vert.x is integrating with JRuby in this way, it's no wonder performance has not been very good. These classes are very heavy and include many layers of abstraction no longer present in the current Java integration implementation.

For fixing the visibility/accessibility of the method, just use Java reflection normally from Ruby, or else don't use the JavaMethod object to actually do invocation. setAccessible applies globally to all consumers of Java reflection, and so it does not need to be performed repeatedly and you do not need to use the same JavaMethod to invoke.

As a workaround, you should be able to add an instanceof JavaProxy check alongside the JavaObject check. All Java objects coming out of JRuby's Java integration will be either JavaProxy (normal Java calling) or JavaObject (old reflective calling).

@vietj

This comment has been minimized.

Copy link
Author

vietj commented Apr 21, 2015

what api would you recommend to use to invoke a very precise Java method of a Java method that will provide the correct implementation ?

@vietj

This comment has been minimized.

Copy link
Author

vietj commented Apr 21, 2015

from this wiki page java_method does not seem harmful https://github.com/jruby/jruby/wiki/CallingJavaFromJRuby . What is the api to invoke a precise Java method from jruby ?

@headius

This comment has been minimized.

Copy link
Member

headius commented Apr 21, 2015

Calling java_method is the correct way to get a specific Java method (given name and argument types), and java_method is the correct way to call a specific Java method. Neither of these return or use JavaObject or JavaMethod, however.

Can you tell me why you need to use java_method at all, rather than just dispatching normally (a la obj.someJavaMethod)?

@headius

This comment has been minimized.

Copy link
Member

headius commented Apr 21, 2015

Sorry, I meant that java_method is the right way to get a specific Java method (as a Ruby Method object) to invoke against, and java_send is the right way to invoke a specific Java method.

@vietj

This comment has been minimized.

Copy link
Author

vietj commented Apr 21, 2015

ah right we are using java_class.declared_method now :-), for instance:

return (Java::IoVertxLangJruby::Helper.fixJavaMethod(@j_del.java_class.declared_method(:protocol))).invoke(@j_del)

I worked back on this code after two months due to other priorities. Please apologise for the wrong things I said so to clarify: we are not using java_method/java_send but instead java_class.declared_method instead.

The main reason is that java_method/java_send does not work well with Java 8 static methods in interfaces. We discussed that http://logs.jruby.org/jruby/2015-01-26#11632375; . If there is a plan to make it work for Java 8 that could be the way to go.

Concerning letting JRuby doing the dispatch, we tried it initially and we experienced bugs that we could not resolve and that were not reproducible, I mentioned it there http://logs.jruby.org/jruby/2015-01-23#11601158

We want to control which method is invoked when overloading occurs and I think there could be ambiguities leading to issues: we experienced this with Nashorn because the dynamic dispatch was working against the Java implementation of the Java interface that was not a public method leading to dispatch runtime errors. The only way to fix it was to use a similar Nashorn feature (vert-x3/vertx-lang-js@766b879) . We fear that we could have the same problem.

@vietj

This comment has been minimized.

Copy link
Author

vietj commented Apr 21, 2015

Concerning the unreproduceable bug, we can use GIT to provide a reproducible build. The bug could not be observed when running a specific test, it has to run the whole test suite to produce it. As I'm far from being a jruby specialist, I could only give up (not specialist + hard to reproduce locally) . We can still provide the test suite and someone more experienced can try to resolve it though.

@headius

This comment has been minimized.

Copy link
Member

headius commented Apr 22, 2015

The main reason is that java_method/java_send does not work well with Java 8 static methods in interfaces.

Dispatching to static interface methods should work in recent versions of JRuby. I don't think we have java_method/java_send on interface modules right now, but that would not be difficult to add.

Concerning letting JRuby doing the dispatch, we tried it initially and we experienced bugs that we could not resolve and that were not reproducible, I mentioned it there http://logs.jruby.org/jruby/2015-01-23#11601158

Please file a bug for that and provide the reproduction. A git repo or test command is sufficient. Based on the log you posted, it looked like a Ruby method implementing a method from a Java interface was supposed to return a string but returned true. True is not automatically coerced to String in Java calls.

We want to control which method is invoked when overloading occurs and I think there could be ambiguities leading to issues

java_method/java_send would be good for this, but you can also java_alias a specific method to a specific name if you only want to call that overload. Again, this isn't on the interface modules, but it could be.

I'd like to help improve the JRuby integration in vert.x so let's keep talking. Seems like there may be lots of room for improvement.

@headius

This comment has been minimized.

Copy link
Member

headius commented Apr 22, 2015

Incidentally, do you know when this broke? I started looking at the code for JavaMethod and it hasn't returned JavaObject for quite some time.

@headius

This comment has been minimized.

Copy link
Member

headius commented Apr 22, 2015

Sorry, I keep getting my names crossed. It's java_method that hasn't returned JavaObject (ever).

But I'm not seeing a problem when I try to use java_class.declared_method either:

$ rvm jruby-1.7.19 do jruby -e "p java.util.Collections.java_class.declared_method(:unmodifiableList, java.util.List).invoke(nil, []).class"
Java::JavaObject

Is this not what you want, or do you have an example case that's not doing this?

Note that language primitives (numerics, booleans, nil and String) automatically coerce to their native Java type.

@vietj

This comment has been minimized.

Copy link
Author

vietj commented Apr 22, 2015

I will do investigations today with various options and make a status. For static methods in interfaces it was late January that it did not work for me.

@vietj

This comment has been minimized.

Copy link
Author

vietj commented Apr 22, 2015

Hi @headius I've done a branch that uses java_method there : https://github.com/vert-x3/vertx-lang-ruby/tree/with_java_method

At the moment I'm seing failures with static interface methods, for instance with the test:

mvn clean test -Dtest=ApiTest#testCustomModule

I get:

testCustomModule(io.vertx.test.lang.jruby.ApiTest)  Time elapsed: 1.928 sec  <<< FAILURE!
java.lang.AssertionError: org.jruby.embed.EvalFailedException: (NoMethodError) undefined method `java_method' for Java::ComAcmePkg::MyInterface:Module
    at org.jruby.embed.internal.EmbedEvalUnitImpl.run(EmbedEvalUnitImpl.java:133)
    at org.jruby.embed.jsr223.JRubyEngine.eval(JRubyEngine.java:90)
    at org.jruby.embed.jsr223.JRubyEngine.eval(JRubyEngine.java:142)
    at io.vertx.test.lang.jruby.RubyTestBase.runTest(RubyTestBase.java:17)
    at io.vertx.test.lang.jruby.ApiTest.runTest(ApiTest.java:510)
    at io.vertx.test.lang.jruby.ApiTest.testCustomModule(ApiTest.java:491)
Caused by: org.jruby.exceptions.RaiseException: (NoMethodError) undefined method `java_method' for Java::ComAcmePkg::MyInterface:Module
    at RUBY.create(/Users/julien/java/vertx-lang-ruby/target/test-classes/acme/my_interface.rb:21)
    at RUBY.test_custom_module(/Users/julien/java/vertx-lang-ruby/target/test-classes/api_test.rb:1025)
    at RUBY.(root)(<script>:1)

The generated code is:

    def self.create
      if !block_given?
        return ::Acme::MyInterface.new(Java::ComAcmePkg::MyInterface.java_method(:create, []).call())
      end
      raise ArgumentError, "Invalid arguments when calling create()"
    end

And the interface is:

public interface MyInterface {

  static MyInterface create() {
    return new MyInterfaceImpl();
  }

  SubInterface sub();

  TestInterface method();

}

headius added a commit that referenced this issue Apr 22, 2015

Add java_method, java_alias, and java_send to interface modules.
Part of improvements to support #2857. Specs pending.
@kares

This comment has been minimized.

Copy link
Member

kares commented Apr 22, 2015

@vietj it's kind of unfortunate but you need to do a java_class to get the "right" Java wrapper (as showcased above - probably missed it) : Java::ComAcmePkg::MyInterface.java_class.java_method

@headius

This comment has been minimized.

Copy link
Member

headius commented Apr 22, 2015

I have committed a fix to JRuby master that adds java_method, java_send, and java_alias to interface modules. That should get us closer to removing all legacy classes from the vert.x integration.

@headius

This comment has been minimized.

Copy link
Member

headius commented Apr 22, 2015

Er, sorry, not master... test-ji-improvements branch.

@kares We could probably make this change in 1.7.20 as well. I have some tweaks there I need to port to master too.

@kares

This comment has been minimized.

Copy link
Member

kares commented Apr 22, 2015

@headius yeah just noticed your update - sounds great if it does not break anything to start getting the "right" API in 1.7.x while the "old" still works ... 👍

@vietj

This comment has been minimized.

Copy link
Author

vietj commented Apr 22, 2015

so what am I supposed to do :-) ? use .java_class.java_method or test a jruby snapshot ?

@vietj

This comment has been minimized.

Copy link
Author

vietj commented Apr 22, 2015

with

    def self.create
      if !block_given?
        return ::Acme::MyInterface.new(Java::ComAcmePkg::MyInterface.java_class.java_method(:create, []).call())
      end
      raise ArgumentError, "Invalid arguments when calling create()"
    end

as advised per @kares I'm getting:

java.lang.AssertionError: org.jruby.embed.EvalFailedException: (TypeError) [] is not a string
    at org.jruby.embed.internal.EmbedEvalUnitImpl.run(EmbedEvalUnitImpl.java:133)
    at org.jruby.embed.jsr223.JRubyEngine.eval(JRubyEngine.java:90)
    at org.jruby.embed.jsr223.JRubyEngine.eval(JRubyEngine.java:142)
    at io.vertx.test.lang.jruby.RubyTestBase.runTest(RubyTestBase.java:17)
    at io.vertx.test.lang.jruby.ApiTest.runTest(ApiTest.java:510)
    at io.vertx.test.lang.jruby.ApiTest.testCustomModule(ApiTest.java:491)
Caused by: org.jruby.exceptions.RaiseException: (TypeError) [] is not a string
    at org.jruby.javasupport.JavaClass.for_name(org/jruby/javasupport/JavaClass.java:1286)
    at org.jruby.javasupport.JavaClass.java_method(org/jruby/javasupport/JavaClass.java:1626)
    at RUBY.create(/Users/julien/java/vertx-lang-ruby/target/test-classes/acme/my_interface.rb:21)
    at RUBY.test_custom_module(/Users/julien/java/vertx-lang-ruby/target/test-classes/api_test.rb:1025)
    at RUBY.(root)(<script>:1)
@vietj

This comment has been minimized.

Copy link
Author

vietj commented Apr 22, 2015

I will try test-ji-improvements branch

@vietj

This comment has been minimized.

Copy link
Author

vietj commented Apr 22, 2015

looks like it is test-jit-improvements

@kares

This comment has been minimized.

Copy link
Member

kares commented Apr 22, 2015

@vietj sorry for the confusion - was trying to get you working on 1.7.x missed that this is actually a static interface method ... that is something we need to figure out as it does not work (without some fixes as proposed above)

@vietj

This comment has been minimized.

Copy link
Author

vietj commented Apr 22, 2015

right, so now I'm trying test_jit_improvements branch with version 9.0.0.0-SNAPSHOT . And we need to figure out the best option for Vert.x

@vietj

This comment has been minimized.

Copy link
Author

vietj commented Apr 22, 2015

I confirm it works correctly and all tests pass with these changes (vert-x3/vertx-lang-ruby@bf8d083) .

@headius

This comment has been minimized.

Copy link
Member

headius commented Apr 22, 2015

@vietj Excellent! I will talk with @enebo about adding the feature to 1.7.20, and then the changes should work there too. Once we figure that out you can re-test with a 1.7-based JRuby as well.

As far as version lifetimes... we know JRuby 9k has taken a long time, so we've continued to maintain the 1.7 line. We will likely do so at least until the end of the year. JRuby 9k itself should be final in the next month, gods willing. It's up to you and vert.x project whether you want to depend on 9k or hold back with 1.7 for these changes.

@vietj

This comment has been minimized.

Copy link
Author

vietj commented Apr 22, 2015

@headius let me know when a jruby 1.7 branch is ready for testing . Do you have an ETA for such branch and for a 1.7.20 release . Concerning 1.7 / 9K, I think we should secure first something that works for Vert.x 3.0 and then we can decide (perhaps do some benchmarks)

headius added a commit that referenced this issue Apr 22, 2015

Add java_method, java_alias, and java_send to interface modules.
Part of improvements to support #2857. Specs pending.

headius added a commit that referenced this issue Apr 22, 2015

Add java_method, java_alias, and java_send to interface modules.
Part of improvements to support #2857. Specs pending.
@headius

This comment has been minimized.

Copy link
Member

headius commented Apr 22, 2015

I just merged the change directly into jruby-1_7 and it will go into 1.7.20, probably next week some time.

The change has also merged to master and I'll be deleting the branch.

@vietj

This comment has been minimized.

Copy link
Author

vietj commented Apr 22, 2015

ok, I'll confirm it works soon with 1.7 branch.

headius added a commit that referenced this issue Apr 22, 2015

@vietj

This comment has been minimized.

Copy link
Author

vietj commented Apr 23, 2015

I just tested with 1.7 branch and I'm seing a weird behavior that does not happen with 9.0.0.0.

A global scoped object magically becomes nil during the tests. That does not happen if tests are executed one by one and that does not happen when using java_class.declared_method.

Consequently the log is filled with:

java.lang.AssertionError: org.jruby.embed.EvalFailedException: (NoMethodError) undefined method `method_with_enum_return' for nil:NilClass
    at org.jruby.embed.internal.EmbedEvalUnitImpl.run(EmbedEvalUnitImpl.java:132)
    at org.jruby.embed.jsr223.JRubyEngine.eval(JRubyEngine.java:90)
    at org.jruby.embed.jsr223.JRubyEngine.eval(JRubyEngine.java:142)
    at io.vertx.test.lang.jruby.RubyTestBase.runTest(RubyTestBase.java:17)
    at io.vertx.test.lang.jruby.ApiTest.runTest(ApiTest.java:510)
    at io.vertx.test.lang.jruby.ApiTest.testEnumReturn(ApiTest.java:366)
Caused by: org.jruby.exceptions.RaiseException: (NoMethodError) undefined method `method_with_enum_return' for nil:NilClass
    at RUBY.test_enum_return(/Users/julien/java/vertx-lang-ruby/target/test-classes/api_test.rb:767)
    at RUBY.(root)(<script>:1)

So that makes me think that there a bug with java_method in 1.7.20-SNAPSHOT. Can someone look at the issue ?

To reproduce the branch https://github.com/vert-x3/vertx-lang-ruby/tree/with_java_method should be used with mvn test. (Note that the previous commit of this branch uses 9K and can be tested too for comparison).

@headius

This comment has been minimized.

Copy link
Member

headius commented Apr 23, 2015

I'll have a look. We've had no reported issues with java_method, so I wouldn't suspect that. If you're running tests in parallel, then the problem could be the isolation level for the ScriptingContainer you're using.

@headius

This comment has been minimized.

Copy link
Member

headius commented Apr 23, 2015

FYI we had some dependency changes last night so I pushed a new 1.7.20-SNAPSHOT.

@vietj

This comment has been minimized.

Copy link
Author

vietj commented Apr 23, 2015

we are not doing concurrent testing (and it works fine with 9K!!!)

@vietj

This comment has been minimized.

Copy link
Author

vietj commented Apr 24, 2015

hi, any news about this ?

@headius

This comment has been minimized.

Copy link
Member

headius commented Apr 27, 2015

I'm looking into the remaining issue today. It appears to be an issue with global variable isolation in the ScriptEngine impl.

@headius

This comment has been minimized.

Copy link
Member

headius commented Apr 28, 2015

I could not quite get the variable isolation to do what I wanted, but I recommended to use instance vars instead of global vars (@obj instead of $obj) which should work fine on any release of JRuby. Still an open question why global variable scoping did not line up right.

@headius headius added this to the JRuby 1.7.20 milestone Apr 28, 2015

@headius

This comment has been minimized.

Copy link
Member

headius commented Apr 29, 2015

I think we can call this fixed. JRuby 1.7.20 will be out late this week or early next, and I believe you'll use it for the next vert.x 3 milestone later this month (with the new Java integration code).

@headius headius closed this Apr 29, 2015

@vietj

This comment has been minimized.

Copy link
Author

vietj commented Apr 29, 2015

+1 thanks

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.