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

JRuby examples don't work (JRuby picks the wrong overloaded method) #320

Closed
iconara opened this issue Aug 23, 2013 · 20 comments
Closed

JRuby examples don't work (JRuby picks the wrong overloaded method) #320

iconara opened this issue Aug 23, 2013 · 20 comments

Comments

@iconara
Copy link

iconara commented Aug 23, 2013

The JRuby adapter doesn't work because JRuby can't figure out which overloaded Observable#subscribe method to pick. It ends up picking Observable#subscribe(Map<String, Object>), which raises an error because the argument doesn't respond right:

require 'rxjava-core-0.10.0.jar'
require 'rxjava-jruby-0.10.0.jar'

observable = Java::Rx::Observable.toObservable(['one', 'two', 'three'])
observable.take(2).subscribe(lambda { |x| puts x })

the code above prints the following errors in JRuby 1.7.4:

% ruby rxjava.rb
rxjava.rb:6 warning: ambiguous Java methods found, using subscribe(java.util.Map)
onNext
Observable.java:278:in `subscribe': java.lang.RuntimeException: 'onNext' key must contain an implementation
    from NativeMethodAccessorImpl.java:-2:in `invoke0'
    from NativeMethodAccessorImpl.java:57:in `invoke'
    from DelegatingMethodAccessorImpl.java:43:in `invoke'
    from Method.java:601:in `invoke'
    from JavaMethod.java:455:in `invokeDirectWithExceptionHandling'
    from JavaMethod.java:316:in `invokeDirect'
    from InstanceMethodInvoker.java:61:in `call'
    from MethodHandle.java:599:in `invokeWithArguments'
    from InvocationLinker.java:153:in `invocationFallback'
    from rxjava.rb:5:in `__file__'
    from rxjava.rb:-1:in `load'
    from Ruby.java:807:in `runScript'
    from Ruby.java:800:in `runScript'
    from Ruby.java:669:in `runNormally'
    from Ruby.java:518:in `runFromMain'
    from Main.java:390:in `doRunFromMain'
    from Main.java:279:in `internalRun'
    from Main.java:221:in `run'
    from Main.java:201:in `main'

in JRuby 1.6.8 it prints a less verbose version of the same error.

Notice the line which reads "onNext". That's actually the puts from lambda { |x| puts x } in action. JRuby wraps the lambda in something that looks like a Map, and then when RxJava calls get on that map JRuby calls call on the lambda.

So if you modify the example code to read lambda { |x| lambda { |y| puts y } } this is what happens:

% ruby rxjava.rb
rxjava.rb:6 warning: ambiguous Java methods found, using subscribe(java.util.Map)
RxJava => Could not find function language adaptor: Groovy with path: rx.lang.groovy.GroovyAdaptor
RxJava => Successfully loaded function language adaptor: JRuby with path: rx.lang.jruby.JRubyAdaptor
RxJava => Could not find function language adaptor: Clojure with path: rx.lang.clojure.ClojureAdaptor
RxJava => Could not find function language adaptor: Scala with path: rx.lang.scala.ScalaAdaptor
one
two

Which kind of works, but it's not as smooth as the ideal API would be:

observable.subscribe { |x| puts x }

But that would probably require a real JRuby native extension (if I get the time I'll send you a pull request with one).

@benjchristensen
Copy link
Member

Glad to have you involved, we haven't had much use of JRuby (that I'm aware of) so I'm not surprised we have some issues. I would appreciate your involvement.

Can you check out code from this pull request (#319), build the rxjava-jruby jar and try it? We are working on a very different approach in this pull request for language adaptors that is statically typed.

As for the subscribe(Map) overload ... I'm not thrilled by that one and it has caused other issues. I've considered getting rid of it before we hit 1.0, but I know it's used in a variety of places so haven't pulled that trigger.

@iconara
Copy link
Author

iconara commented Aug 24, 2013

It works better, but not perfect. JRuby seems to pick a working overload, but it still warns about there being multiple overloads (which is hard to fix without adding JRuby-metadata to the Observable class).

An upside is that you can skip the lambda { ... } and use the more Ruby-ish: obs.subscribe { |x| puts x } (i.e. pass a block to #subscribe).

Since there will be a version generated to work specially for JRuby, how much can the code generation things add? Ideally it would add JRuby metadata to the class to make it work as a JRuby native extension. Can you give me any hints on how to dig into that? Where should I start to look in the code to understand the code generation?

@iconara
Copy link
Author

iconara commented Aug 24, 2013

If the generated Observable could be something like this (but lots more, obviously), it could integrate with JRuby even better:

@JRubyClass(name="Rx::Observable")
public class Observable<T> extends RubyObject {
    @JRubyMethod(meta = true, rest = true)
    public static <T> Observable<T> from(ThreadContext ctx, IRubyObject receiver, IRubyObject[] args) {
        // ...
    }

    @JRubyMethod(name = "subscribe")
    public IRubyObject subscribeRuby(ThreadContext ctx, Block block) {
        // ...
    }

There would be some more code needed that registered the class with the runtime, and created the Rx module, and some more things, but that wouldn't need to be generated.

@benjchristensen
Copy link
Member

Can you take a look at pull request #323 and suggest how to re-add JRuby support correctly? We have decided against doing byte-code generation.

We can not add anything (such as the annotations in your example) to rxjava-core, it needs to be in a separate submodule, similar to Groovy, Scala and Clojure. So this means using something like extension methods, implicits, or other such tools if JRuby supports them.

@iconara
Copy link
Author

iconara commented Aug 30, 2013

I will have a look.

@iconara
Copy link
Author

iconara commented Aug 30, 2013

With some small patches at least some of the basics work:

require 'rxjava-core/build/libs/rxjava-0.10.1-SNAPSHOT.jar'

class Java::Rx::Observable
  java_alias :subscribe, :subscribe, [Java::RxUtilFunctions::Action1.java_class]
end

module Rx
  include_package 'rx'
end

o = Rx::Observable.from([1, 2, 3])
o.map { |n| n * 2 }.subscribe { |n| puts(n) }

I haven't tested any more than that, but it looks like it could work.

@jmhofer
Copy link
Contributor

jmhofer commented Aug 30, 2013

#323 should be 0.11.0-SNAPSHOT afaik, not 0.10.1-SNAPSHOT. Are you sure you're using the correct version of RxJava?

@iconara
Copy link
Author

iconara commented Aug 30, 2013

Good catch, I'm not used to gradle, or even having to compile stuff, so I didn't clean up from the last build I did so the old jars were still around.

The Ruby patch works with 0.11.0 too (which means that those small patches would have make the old version work too, so that's good to know). Without the patch it works as with #319, the code runs but it prints a warning about not finding the right overloaded method.

The boring part of doing it this way is that each method on Observable, and any other classes needed for interoperability will need to be annotated with java_alias. On the other hand I don't see any other way of doing it which wouldn't involve writing even more code.

@benjchristensen
Copy link
Member

Does JRuby have a way to apply these aliases programmatically? For example, in Groovy we use reflection to determine all methods that need extensions and then programmatically create all of the MetaMethod implementations to bridge Groovy and Java. You can see an example here: https://github.com/mattrjacobs/RxJava/blob/8c87c29bea5e076bdc4202af5626d474ab1c7117/language-adaptors/rxjava-groovy/src/main/java/rx/lang/groovy/RxGroovyExtensionModule.java#L103

Otherwise every time a new method is added to core someone will have to maintain the JRuby java_alias mappings.

@iconara
Copy link
Author

iconara commented Aug 31, 2013

Maybe, I'm not sure, I'll have to look into more about how JRuby's native extensions work. So far I've only done it by creating classes and adding metadata, but I guess that it should be possible to do it on existing classes to somehow.

If I understand the code you linked to correctly it's looking through the Observable and BlockingObservable classes for methods that take subclasses of Function and somehow makes the Groovy runtime prefer those over any other overloads. That's what would be needed for JRuby too, and since the Groovy version just uses Java reflection to do it I think it should be possible in JRuby too.

All that the java_alias thing does is that it looks up a Java method by reflection and adds that method into a cache that JRuby will look in before it looks at the Java class, which is kind of similar to what the Groovy extension thing does, I think.

So, yes, probably, maybe, hopefully, but I'm not sure exactly how right now.

@ragalie
Copy link

ragalie commented Sep 23, 2013

This isn't complete yet, but here's a first pass at an implementation I think will work. @benjchristensen can you take a look and let me know if this is the direction you're looking for? It's largely cribbed from the Groovy implementation.

ragalie@fc0a706

@iconara
Copy link
Author

iconara commented Sep 23, 2013

I'm getting warnings about overloads.

This is my code:

require 'rxjava-core/build/libs/rxjava-core-0.14.2-SNAPSHOT.jar'
require 'language-adaptors/build/libs/language-adaptors-0.14.2-SNAPSHOT-sources.jar'

module Rx
  include_package 'rx'
end

o = Rx::Observable.from([1, 2, 3])
o.map { |n| n * 2 }.subscribe { |n| puts(n) }

and this is the output (JRuby 1.7.4):

rxjava.rb:14 warning: ambiguous Java methods found, using subscribe(rx.util.functions.Action1)
2
4
6

@ragalie
Copy link

ragalie commented Sep 23, 2013

Just to be clear, the commit I posted doesn't work yet. I just want to make sure that the approach is sane before investing more time in it.

@iconara
Copy link
Author

iconara commented Sep 24, 2013

Ok. Yeah, it might work.

Not sure how well the generated methods will work, though: they are added on the classes, but call super to call the original method, but I'm not sure that super is actually what you think it is (it might be, I assume you've run the code, and I have not, so I could be completely wrong). In other words, you're adding a method on Observable, and then you call super in that method to call the original method on Observable, but that should call a method on the superclass of Observable, not the original method -- unless JRuby tries to be clever and figures out that your added method is on the eigenclass and super should refer to Observable, but I don't think it does.

And either way, once you call super JRuby still has to look up the right Java method to call, and it doesn't have any more type information to do it, so it will pick one at random and print out the "ambiguous Java methods found" warning. Even if you've wrapped the proc in an Action I'm not sure that JRuby will pick the right overloaded method. In my experience it tries, but it doesn't always succeed in picking the right method.

I think a better way to solve the problem is to use java_alias as in one of my examples above. That way JRuby will always pick the right overload. I'm not sure all the wrapper classes would be needed in that case, because JRuby will wrap a proc in a proxy class that implements whatever interface is required (and since Function and Action interfaces only really have one method, that also just happens to map straight to interface of proc, I think letting JRuby do it makes sense). There might be a slight performance loss from letting JRuby generate proxies instead of having Java classes, but I think it's greatly outweighted by skipping a large part of the work that JRuby otherwise would have done in finding the right overload.

@ragalie
Copy link

ragalie commented Sep 24, 2013

You're right, the super approach won't work.

I tried the java_alias approach first and the problem I ran into is that it didn't seem to work correctly when a method had multiple signatures with the same number of arguments. For instance, I tested it with RxJava 0.10 which has subscribe(Hash) and subscribe(Action1) (or something like that). I tried just running java_alias on the methods that had Function arguments and it threw an error when I tried to use the Hash argument. I tried running java_alias on all methods, but running it first on the non-function methods and second on the function methods and that didn't work either (threw an error with a Hash argument).

I'm a relative newbie at JRuby, so totally possible I'm doing something wrong. But from my basic understanding I don't understand how java_alias will be a solution if there are two signatures with the same number of arguments and we want to allow access to both of them.

@ragalie
Copy link

ragalie commented Sep 25, 2013

I spent some more time researching this evening and I think the easiest path forward is to leverage the JRuby method dispatching as much as possible. Outside of the Proc => Action/OnSubscribeFunc/Function casts, which we can reliably do but which the JRuby dispatch logic is having trouble reliably doing, the default dispatch logic is likely to be far better at sussing out the method signature we want to invoke than we would be.

The first thing the dispatch logic checks for when trying to find a method signature match is whether the Java class of the argument provided is an exact match with the parameter type specified in the method signature (see: https://github.com/jruby/jruby/blob/master/core/src/main/java/org/jruby/java/dispatch/CallableSelector.java#L321). While it isn't guaranteed that this will always be true (JRuby could change the dispatch logic), I think it's a safe bet that it will continue to preference exact class matches when selecting overloads. I think that means that as long as we pass in arguments that implement the exact interfaces JRuby should have a much easier time finding the matching method signature.

Assuming that's true, I think we should implement an algorithm like the following on load:

  • Select the methods that have any signatures that contain a parameter type that is a (sub)interface of rx.util.functions.Function
  • Determine the (sub)interfaces of Function that are possibilities for each argument position for a given method.
  • If any given argument position has more than one possibility (e.g. Action1 or Action0), ignore.
  • If any given argument position has only one possibility, then note that we should convert any Proc we see in that argument position into the Function subclass we've identified.

Then I think we should implement the following to occur at runtime:

  • Upon invoking a method where we've noted that we should convert Procs, replace the Proc arguments with wrappers that implements the noted interfaces, if applicable.
  • Call the original Ruby method with the modified arguments.

We should be able to do this using alias_method to copy the original subscribe, for instance, to subscribe_without_argument_wrapping and then redefine subscribe to modify the arguments and call subscribe_without_argument_wrapping with the updated arguments.

Under this scenario most of the hard dispatch logic remains in Java: the only additional runtime things we're doing in Ruby are a) a check to see if the argument is a Proc, b) a Hash lookup to see whether we can replace the Proc with a wrapper and c) instantiating the Java wrapper. So I don't think it should significantly slow things down.

It's clear you've thought about this quite a bit @iconara; anything stick out to you as suspect here?

@ragalie
Copy link

ragalie commented Sep 26, 2013

It isn't the cleanest thing in the world, but ragalie@957af11 seems to be working correctly.

This code no longer causes an ambiguous method warning:

require 'rxjava-core/build/libs/rxjava-core-0.14.2-SNAPSHOT.jar'
require 'language-adaptors/rxjava-jruby/build/libs/rxjava-jruby-0.14.2-SNAPSHOT.jar'
require 'language-adaptors/rxjava-jruby/src/main/ruby/rx/lang/jruby/interop'

o = Java::Rx::Observable.from([1, 2, 3])
o.map { |n| n * 2 }.subscribe { |n| puts(n) }

I'm going to clean it up as best as I can then open a PR. I'm pretty new to JRuby, though, and I'm sure there are some ways to simplify what I've done, so hopefully someone can help me out with that. In particular I'd love to be able to leverage the built-in JRuby proxying (telling it which interface to proxy) instead of the clunky ones in the commit.

I also don't know what to do with the Ruby code. Should that stay in the JAR and just need to be required manually? Or should it be pulled out to a gem? I'm not sure what's idiomatic.

Thanks!

@iconara
Copy link
Author

iconara commented Sep 26, 2013

@ragalie I'm sure it can be done more elegantly, but it would take a lot of time, and I don't know very much more about the details of JRuby's Java integration to say for sure how to do it. It's better to get something that works and improve it later than trying to find the optimal solution now.

There's a way to package a JAR that makes JRuby run code when it is require'd from Ruby code. You need to stick a special class at the root of the JAR (here's an example: https://github.com/iconara/msgpack-jruby/blob/master/ext/java/MsgpackJrubyService.java). It's used to load JRuby native extensions (i.e. Java code that creates JRuby modules and classes), but in this case it could be used to automatically run the interop code (which can be loaded from within the JAR).

Another option would be to make the the interop code the main entrypoint for Ruby, and for it to load rxjava-core.jar and rxjava-jruby.jar. Come to think of it, that would be the better solution.

The benefit of the former solution would be that you could ship it all as just the JAR, but the latter is simpler to maintain and is how many JRuby wrappers for Java libraries work.

If it's ever going to get any kind of adoption in the Ruby world the library must be packaged as a gem.

@ragalie
Copy link

ragalie commented Oct 8, 2013

I implemented JRuby support in #422. Let me know what you all think and if there's anything that doesn't seem to be working correctly. Thanks!

@benjchristensen
Copy link
Member

Awesome @ragalie! I'm taking a look now and will merge it into master or iterate with you on it if there are changes needed.

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