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 function wrapping support #422

Merged
merged 8 commits into from
Oct 9, 2013
Merged

Conversation

ragalie
Copy link

@ragalie ragalie commented Oct 7, 2013

This PR fixes #320 by implementing JRuby wrappers for RxJava-specific function interfaces. Given a set of method signatures for a Java method, JRuby will select the signature that best matches the Ruby arguments provided and wrap each argument in a proxy that implements the correct Java interface.

Occasionally JRuby will be unable to unambiguously select one method signature over another, and will either a) select the correct signature, but report that there was ambiguity in the method signatures or b) select the wrong signature and fail.

By explicitly wrapping Proc arguments into wrappers that implement the correct RxJava interface, JRuby will always select the correct method signature and will not need to wrap the argument in a proxy, increasing both correctness and performance.

Mike Ragalie added 6 commits October 6, 2013 17:14
Whenever a RubyProc is passed into an Observable method, wrap it in
a Java class that implements the correct RxJava interfaces. This avoids
ambiguous method errors and costly proxy instantiations by JRuby's
default method delegation logic.
OnSubscribeFunc#onSubscribe expects to return a Subscription. I am
unable to successfully cast the return value of a RubyProc, even if
that value _is_ an object that implements the Subscription interface,
into a Subscription in Java-land (Java reports that ConcreteJavaProxy
cannot be cast). Instead I allow JRuby to handle OnSubscribeFunc
arguments through its default proxy logic, which works correctly.
@cloudbees-pull-request-builder

RxJava-pull-requests #330 FAILURE
Looks like there's a problem with this pull request

@cloudbees-pull-request-builder

RxJava-pull-requests #331 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

RxJava-pull-requests #332 SUCCESS
This pull request looks good

@benjchristensen
Copy link
Member

Structure of files, build etc looks good. Code looks good as far as I can tell (I'm not a Ruby expert).

Great contribution @ragalie ! I'm merging for the next release. Continue submitting fixes/improvements as needed.

benjchristensen added a commit that referenced this pull request Oct 9, 2013
JRuby function wrapping support
@benjchristensen benjchristensen merged commit abc8bec into ReactiveX:master Oct 9, 2013
@benjchristensen
Copy link
Member

Curious, is it this class require "rx/lang/jruby/interop" that triggers all of the logic for converting from Proc to Action/Function?

Is that idiomatic in JRuby to have an interop import like this, or is something like rx/lang/jruby/Observable also appropriate?

I have no idea, so it's out of curiosity and want of understanding that I ask.

@ragalie
Copy link
Author

ragalie commented Oct 9, 2013

Hi @benjchristensen, that is the line that triggers the wrapper logic.

I'm not sure what's idiomatic in JRuby, I'm pretty new to it as well. I considered rewriting the Ruby interop code in Java so that I could include it as a static block somewhere, but it would've taken much longer to get this out and I wasn't sure of the ultimate feasibility.

The best delivery method for rxjava-jruby would probably be through a gem that depends on the relevant jars (using jbundler, for instance), requires the jars and then does the interop require. But again, I'm relatively new to the JRuby ecosystem and I don't know what exists for building gems via gradle/maven (or if you are even interested in maintaining a gem for RxJava specifically).

Once this gets released I will do the requisite require in porcupine so that anyone using Porcupine for Hystrix in JRuby will benefit at least.

@benjchristensen
Copy link
Member

Is it worth reaching out to others in the JRuby community before releasing this to get some input on whether there is an expected approach to importing this?

if you are even interested in maintaining a gem for RxJava specifically

I don't think so, but honestly have no idea what that even entails.

Thanks again for the contribution. I'm waiting on confirming a few other things but likely will release in the next day or two.

@ragalie
Copy link
Author

ragalie commented Oct 9, 2013

@iconara, what do you think? Is it too much of an imposition to ask people to require an interop file in addition to the jar file? The BasicLibraryService approach you suggested seemed promising, but seemed potentially difficult in practice since it needs a particular filename and directory structure to work correctly.

@iconara
Copy link

iconara commented Oct 9, 2013

As you say, the BasicLibraryService approach depends on the JAR being named correctly, which is difficult since you often want version numbers and that kind of thing in the name.

I think that the interop code should require the JARs, you shouldn't have to require those first. The interop code should know where they are, and could just use a glob in place of the version so that it wouldn't have to be changed for each version (e.g. require Dir['path/to/jars/rxjava-core-*.jar'].first).

In the end, without a gem release I don't really see any adoption in the JRuby community. Maybe releasing a JAR that automatically runs the interop code would work too, with JBundler that would at least mean that dependency management could work. If you released an official RxJava gem that bundled the core and JRuby JARs that would be much better. I'm not yet convinced by JBundler, but releasing the interop code as a gem and depending on the RxJava JARs like Porcupine depends on Hystrix could work. You'd constantly get bug reports about the JARs missing, or about making it work without JBundler, but apart from that I think it would work.

@ragalie
Copy link
Author

ragalie commented Oct 9, 2013

The difficulty is that the interop code is currently inside the rxjava-jruby.jar, so it can't be required until the jar is required first.

To be honest, my interest is primarily in Hystrix, so to me having instructions in the README and doing the correct requires in porcupine seems like a good stopping point. If there's independent interest in an RxJava gem then I would be happy to help get that going, however.

@iconara
Copy link

iconara commented Oct 9, 2013

You can probably require the interop code only, even if it's in the JAR.
Try using "path/to.jar!path/inside/jar.rb". I think it should work. On the
other hand, telling people to do that is probably even worse :)

On Wed, Oct 9, 2013 at 9:19 PM, Mike Ragalie notifications@github.comwrote:

The difficulty is that the interop code is currently inside the
rxjava-jruby.jar, so it can't be required until the jar is required first.

To be honest, my interest is primarily in Hystrix, so to me having
instructions in the README and doing the correct requires in porcupine
seems like a good stopping point. If there's independent interest in an
RxJava gem then I would be happy to help get that going, however.


Reply to this email directly or view it on GitHubhttps://github.com//pull/422#issuecomment-26000136
.

rickbw pushed a commit to rickbw/RxJava that referenced this pull request Jan 9, 2014
JRuby function wrapping support
jihoonson pushed a commit to jihoonson/RxJava that referenced this pull request Mar 6, 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

Successfully merging this pull request may close these issues.

4 participants