ReactiveX / RxJava

RxJava – Reactive Extensions for the JVM – a library for composing asynchronous and event-based programs using observable sequences for the Java VM.
Apache License 2.0
47.92k stars 7.61k forks source link

Idiomatic Scala Support #336

Closed benjchristensen closed 11 years ago

benjchristensen commented 11 years ago

As of version 0.11.0 Scala support is provided through the use of implicits. Conversations on Twitter are bringing up other possible improvements. Let's use this issue to discuss.

jmhofer commented 11 years ago

A few comments from my point of view (since I first looked at this project, I always wanted to use it from Scala):

Now that the core is typesafe, imho we have the first workable Scala integration (via implicits). From here on, everthing depends on how much effort we want to put into this vs how convenient it will be for the users.

What we already have is that the most important method names match (map, flatMap, filter). Matt also made Scala for-comprehensions work with observables.

There's a lot of room for improvements, though. It seems that due to Scala-Java interop, there are problems with type inference (I even ran into a Scala compiler crash somewhere), and sometimes asJava collection conversions are necessary. Also, Scala/Akka futures are not yet addressed at all. An Akka-specific scheduler would probably be awesome to have, too.

For full Scala user convenience, we'd probably have to completely wrap rx.Observable into its own rx.scala.Observable. This would mean a lot more maintenance effort for the Scala integration, of course (I don't think we can do this automatically via code generation or macros, but I'm no expert of all that). But still, I think it's well worth it to create a dedicated Scala API.

xeno-by commented 11 years ago

1) Implicit-based integration with Java-based observables has a serious flaw as it doesn't support parameter type inference for lambdas. The situation isn't going to change until https://issues.scala-lang.org/browse/SI-6221 is fixed. It's definitely fixed in 2.11, and at EPFL we have a compiler plugin that backports the fix to 2.10. Probably Typesafe folks can be convinced to do the backport in one of 2.10.x releases (/cc @adriaanm @gkossakowski @retronym @JamesIry).

2) Some features that might be desirable in rx.scala.Observable: a) native function types, b) native collections, c) method names familiar to folks that use native collections, d) making parameter of defer by-name, e) creation of Observables via apply rather than via from.

3) I also don't think that macros can help with rx.scala.Observable, because what you're probably after isn't just a copy/paste of a Java API, but rather a redesign that takes Scala features into account. But if there are some things that can be autogenerated, I'll be happy to answer questions about how macros work and how they can be used.

aloiscochard commented 11 years ago

IMHO, after looking at the code, using implicits convertions is not a good idea.

I think you have better to start with a complete wrapper of the API, then you'll be able to apply scala idiom (basically writing a DSL) instead of having a Java API with some facilities for converting type.

And that way you don't have to wait for https://issues.scala-lang.org/browse/SI-6221 ... which sounds anyway like a clumsy integration of Java API to me.

benjchristensen commented 11 years ago

Here is some background on why the current design was chosen rather than having each language with a separate version of Observable:


The approach of having language specific packages/classes was pursued but did not work well because Rx is a composable library. It means that every time an Observable is used it needs to be re-wrapped or un-wrapped by whichever language is using it.

For example ...

From Java a library is exposed that has a method like this:

rx.Observable getData()

From Groovy a library is exposed with a method like:

rx.groovy.GroovyObservable getOtherData()

Then from Scala you need to wrap them again:

rx.scala.ScalaObservable.from(getOtherData())

This means we have an rx.Observable wrapped as rx.groovy.GroovyObservable wrapped as rx.scala.ScalaObservable.

To compose the two we would have:

rx.scala.ScalaObservable.zip(rx.scala.ScalaObservable.from(
      getOtherData()), 
      rx.scala.ScalaObservable.from(getData()),
       ... scala closure here ...);

Now what does ScalaObservable return from its operators? ScalaObservable or Observable?

Should the above zip operator return rx.scala.ScalaObservable or rx.Observable? What happens if this library is consumed from another language?

If Observable each step along the way it must be wrapped yet again. If ScalaObservable it has now changed all of the return types of rx.Observable to a subtype.

In short, for interop between languages it very quickly becomes a mess and our primary polyglot goal was that rx.Observable was usable across all libraries as the single type and because the whole point of Rx is chained composition it's not as simple as just a single decoration at the beginning. It affects every single method in an API and step of the chaining.

For this reason we chose the current language-adaptor model so rx.Observable can remain the sole public interface across languages.

xeno-by commented 11 years ago

Thanks for providing the design notes!

I wonder though how often people tend to mix languages in their projects. Usability improvements in switching from a Java-based least common denominator to an idiomatic API might be significant. Does being polyglot overweigh these improvements?

jmhofer commented 11 years ago

I'm not sure if wrapping Observable for Scala (not in a subclass though) would really hurt so much. Imho it's worth a try to find out how it feels. There will be a lot of wrapping and unwrapping behind the scene though, I'm afraid.

benjchristensen commented 11 years ago

At Netflix we are using Clojure, Groovy, Java and Scala and I know of apps running code from at least 3 of those 4 in the same JVM instance. I imagine it's not common in most environment for this type of diversity, but it is something we have wanted to support as seamlessly as possible. This is because we have wanted the rx.Observable to act as the interface we can expose across module boundaries as it naturally ends up at the edge of the API in methods such as Observable<T> getDataFromService(args). This is part of what drove us to target the JVM with RxJava and not any specific language.

That said, an idiomatic solution that works best for pure Scala apps is more important. If we can find a solution that can retain the use of rx.Observable without a completely separate wrapper then great, otherwise let's have a ScalaObservable for pure Scala apps and a way of going back and forth across language boundaries when it's needed. Perhaps the implicits that exist right now can solve the immediate interop needs for easy interaction from Scala to Java, but the option to convert to ScalaObservable would also be there.

Another piece of information to guide this ... the Rx.Net version in C# is defined by simple interfaces for Observer and Observable without any of the operator interfaces on them. All of the operator methods (static and instance level) are added via extension methods. It's quite clean and makes the interfaces more flexible (anyone can easily implement them and the extension methods are 'just there' without inheritance involved).

The reason RxJava has Observable as a concrete class is because extension methods don't exist in Java so we don't have a choice but to have them as concrete methods to enable the fluent chaining pattern.

In Scala however we do have extension methods (implicits), macros etc that theoretically can allow the rx.Observable to be made into whatever it needs to be for idiomatic Scala usage, similar to how C# implements Rx.Net using extension methods. I imagine a possible issue is if existing methods on rx.Observable are in the way and cause problems in achieving idiomatic Scala functionality, if that's the case I'd like to understand what those issues are and if they can be resolved and if not if they really are deal breakers.

At this point I become not so helpful as I am not skilled enough in Scala to have a valid opinion or guide the conversation much further. I do have some questions though for the Scala experts here:

1) What would make rx.Observable idiomatic for Scala?

I'd like to see unit tests or sample code demonstrating expected behavior and usage so that we're all working towards the same goal and know when we've achieved success.

2) What about the current rx.Observable + Scala implicits is not working well?

3) What requirements of (1) can not be achieved with implicits and macros and requires a separate concrete class as a wrapper?

Thank you everyone for your involvement, I really do want us to find the ideal solution for Scala and then as a secondary priority make it work well across the JVM for polyglot applications.

xeno-by commented 11 years ago

Thank you for your swift feedback! It's a pleasure to help.

1) I'm not an Rx expert, and in fact this is the first time I see any Rx code apart from random snippets in web articles, so please don't treat this list as definitive. Comprehensive analysis would require quite some time, and I'm not sure I have it right now.

a) Observable.from could be replaced by Observable.apply, so that one can write Observable(1, 2, 3) instead of Observable.from(1, 2, 3).

b) It shouldn't be necessary to write asJava, e.g. as in https://github.com/Netflix/RxJava/blob/master/language-adaptors/rxjava-scala/src/main/scala/rx/lang/scala/RxImplicits.scala#L349, and asScala (not sure whether it's necessary now).

c) Use of mutable collections in situations like https://github.com/Netflix/RxJava/blob/master/language-adaptors/rxjava-scala/src/main/scala/rx/lang/scala/RxImplicits.scala#L350 is really inconsistent with Scala's pursuit of immutability. I wonder whether it'd be possible to write such code in a functional way.

d) Instead of taking a no-arg function, Observable.defer could take a by-name parameter, which would obviate the need of creating an explicit closure: https://github.com/Netflix/RxJava/blob/master/language-adaptors/rxjava-scala/src/main/scala/rx/lang/scala/RxImplicits.scala#L232.

e) For a former C# developer like me, method names like lastOrDefault sound familiar, but for the Scala croud out there - not so much. How about changing the names and maybe even signatures, so that they mirror method names of native collections? E.g. lastOrDefault could become lastOption (also note the change from default values to options).

f) I'm also not sure whether extension methods are even necessary. From what I would guess, in C# they are forced to use them, because interfaces can't define methods with implementations (at least, that was the story with IEnumerable and Enumerable). In Scala, we can do that, so why not just put all the combinators in Observable?

g) mapMany in https://github.com/Netflix/RxJava/blob/master/language-adaptors/rxjava-scala/src/main/scala/rx/lang/scala/RxImplicits.scala#L351 is essentially flatMap, right?

h) No idea how this name could be stated more succinctly, but toBlockingObservable feels a bit verbose. When I moved from C# to Scala, in people's code I felt an overall tendency to compress everything, including names. This is kind of a vague observation, so feel free to ignore it.

2) Function literals don't support inference for parameter types unless they are used in a context that requires a Scala native function type. In particular, inference won't work if function literals are supposed to be converted to something implicitly.

E.g. in https://github.com/Netflix/RxJava/blob/master/language-adaptors/rxjava-scala/src/main/scala/rx/lang/scala/RxImplicits.scala#L341 one can't drop type annotations for lambda parameters. I tried removing them and got a compilation error. (Btw why are you using 2.10.1, when 2.10.2 is already available for quite long? Did you have any problems with it?)

That's an instance of https://issues.scala-lang.org/browse/SI-6221, which has been fixed in 2.11 and probably could be backported to 2.10. If you're in need of an immediate solution, we at EPFL have a compiler plugin for 2.10 that provides a backport.

vjovanov commented 11 years ago

Excellent observations about the API @xeno-by. Currently, we are preparing exercises for the Coursera class Principles of Reactive Programming. For this course we would like to make an idiomatic Scala API. @samuelgruetter is working on that.

The cross language compatibility pointed out by @benjchristensen is a reasonable concern. However, I think there is a lot of monolingual projects out there that would greatly benefit from the idiomatic Scala library. It seems to me that adding a Scala wrapper can be only a plus in those cases.

For polyglot projects we could also take the Java rx.Observable as a common ground (being a return type of each operation and in API signatures). Then all of the Scala support can be added by an implicit conversion (rx.Observable => rx.ScalaObservable).

As a long-shot maybe the interface for multi and mono lingual projects can be unified so that we do not repeat our selves. IMHO it is worth a try.

jmhofer commented 11 years ago

Thanks for all the observations! I'll try to address some of the ones from @xeno-by.

1a), 1e) and 1g) are just aliasing problems, imho. We already have flatMap as alias for mapMany, and the other aliases could easily be added, either directly in rx.Observable, or in a Scala wrapper, if the namespace gets too big otherwise (not my preferred solution). Methods returning options will have to live in a Scala wrapper, however.

I didn't take a look at 1b) and collection conversions yet. This really shouldn't be necessary often, if at all.

1c) You're looking at test code here which tests the callback "at the end of the world". This is not really normal usage of the API. The whole RxJava codebase is refreshingly (though not completely) free of mutability, if you ask me. Also, the Rx API is actually there to allow you to handle all your events in a functional, immutable way. - In short, I don't see a problem here.

1d) is Scala-specific. By-name parameters are a great feature for methods like defer, I agree. We should make use of them in the Scala wrapper.

I'm not worried about 1f) either.

1h) I like the verbose name in this specific case. Imho, it's the same as asInstanceOf in Scala: Maybe I'm evil, but I like to punish people who do things they probably shouldn't do. :) Ok, there will of course be valid use cases for blocking observables, but there shouldn't be too many of them, hopefully.

2) is quite a big problem, if you ask me. A backport of the fix to Scala 2.10 would be great. We can probably get around this with a dedicated Scala wrapper, though.

And the reason for 2.10.1 instead of 2.10.2 is currently just a technical problem with the build system. I, too, hope that this will be fixed soon.

abersnaze commented 11 years ago

1c) Even that can be made functional if you replace the subscribe() with .toList().toBlockingObservable().single();

jmhofer commented 11 years ago

@abersnaze You're right, good point.

I started a little experiment with a more idiomatic Scala wrapper. It's just humble beginnings currently, though. You can find it here.

Comments and forks are welcome, of course. I started from the super-extends pull request in order to check how this whole thing feels in Scala.

benjchristensen commented 11 years ago

Scala 2.10.2 support was added in version 0.11.2 (https://github.com/Netflix/RxJava/releases/tag/0.11.2).

adriaanm commented 11 years ago

1) Implicit-based integration with Java-based observables has a serious flaw as it doesn't support parameter type inference for lambdas. The situation isn't going to change until https://issues.scala-lang.org/browse/SI-6221 is fixed. It's definitely fixed in 2.11, and at EPFL we have a compiler plugin that backports the fix to 2.10. Probably Typesafe folks can be convinced to do the backport in one of 2.10.x releases (/cc @adriaanm @gkossakowski @retronym @JamesIry).

Sorry, we don't change type inference in minor versions unless there's a critical bug. The risk of regression in source compatibility is too high.

benjchristensen commented 11 years ago

The cross language compatibility pointed out by @benjchristensen is a reasonable concern. However, I think there is a lot of monolingual projects out there that would greatly benefit from the idiomatic Scala library. It seems to me that adding a Scala wrapper can be only a plus in those cases.

I completely agree if a wrapper is the only way to achieve idiomatic support. Monolingual projects should take first priority as that is the common case.

samuelgruetter commented 11 years ago

We believe that the following solution would be best:

I've started to implement such an adapter and will post some code soon.

Note that I've not (yet) addressed the following points:

jmhofer commented 11 years ago

@samuelgruetter Sounds awesome! Very much looking forward to your sharing the code.

samuelgruetter commented 11 years ago

You can see what I'm doing here. Note that it's still very incomplete and not yet tested, but work is in progress.

jmhofer commented 11 years ago

Great, thanks! I'll play around with it, too.

mattrjacobs commented 11 years ago

@samuelgruetter All of that sounds great. I'll take a look next week and offer feedback.

samuelgruetter commented 11 years ago

There's an interesting problem with map: If I add an explicit conversion like ScalaObservable(numbers).map(...), it works, but without, i.e. numbers.map(...) gives an error... I'll investigate on this. You can see the corresponding test here

jmhofer commented 11 years ago

Maybe there's a 2nd implicit somewhere, adding map?

aloiscochard commented 11 years ago

@samuelgruetter what is the error? if @jmhofer is right you should have an amibgous implicit resolution error.

benjchristensen commented 11 years ago

If anything about the core Java library is causing issues let me know.

samuelgruetter commented 11 years ago

There's no 2nd implicit, but there's a second map: The map in rx.Observable. And scalac wants to use this one and does not convert to ScalaObservable. However, with reduce, it works...

jmhofer commented 11 years ago

Just another guess, but then it's maybe due to the type parameter of map (your reduce doesn't have one; you renamed the one with type parameter to fold which luckily avoids the problem).

benjchristensen commented 11 years ago

Covariant support has been merged into master. This also changes the type used with Observable.create.

benjchristensen commented 11 years ago

I believe all of the structural changes are now merged to master. Are there any other changes that should be made before we release 0.12 so as to better support Scala integration?

samuelgruetter commented 11 years ago

I've isolated the implicit conversion problem here. It's a problem with the Scala compiler. @benjchristensen looking forward to a jar with covariant observables!

aloiscochard commented 11 years ago

@samuelgruetter I'm pretty sure no one could make the scala compiler be able to solve an ambiguity like that one.

If you where the compiler, which method you think should be choosed?

You guys should seriously consider implementing a full wrapped scala version first, and keep multi-lang support as a cherry on cake.

IMHO it's a mistake to do the opposite, maybe I'm wrong but I think most scala user will want to use it in a full scala project.

And for the course, it would be good to have an idiomatic API.

retronym commented 11 years ago

scalac is indeed inconsistent, it goes the extra mile to try an implicit view when the originally tried method is monomorphic, but doesn't do the same if it is polymorphic.

That's unlikely to change soon, even in 2.11.

But, one change that is likely in the 2.11 timeframe is first class support of SAM types, which would let you write an anonymous function that would be translated to rx.Func1.

BTW, it's really great to see this discussion and cooperation happening!

jmhofer commented 11 years ago

So, to make polymorphic methods like map work, we'd have to return ScalaObservables, not (Java)Observables everywhere, I guess (which doesn't pose a problem anyway as far as I can see)?

@aloiscochard: Is that what you mean by "a full wrapped scala version"? - Because I thought that @samuelgruetter's goal was already to create a full wrapped scala version.

aloiscochard commented 11 years ago

@jmhofer yeah it's exactly what I meant :-)

By doing that you'll be able to provide the best possible syntax for RX API, looking forward next enhancement!

benjchristensen commented 11 years ago

@samuelgruetter is there something we can do in the short-term to work around this issue? Is there anything about the core library that if changed would simplify this effort?

@aloiscochard What about the approach being pursued (if the compiler bugs didn't exist) is a mistake?

jmhofer commented 11 years ago

@benjchristensen I don't see any problem with switching to the approach where we stay with intermediate ScalaObservables throughout the wrapper. As ScalaObservable is a value class, it shouldn't even impact performance at all.

samuelgruetter commented 11 years ago

I agree with @jmhofer. I've started such a Scala Observable which always returns Scala Observable, see here

aloiscochard commented 11 years ago

@benjchristensen I would say it's kind of bad design, a form of overloading abuse. I would go the type-class way (on a similar Wrapper than @jmhofer one) to make a map function polymorphic.

Anyway, the current approach took by @samuelgruetter looks good! As @jmhofer said there should be no perf impact as long as you keep a value class, and from what I've seen there should be no reason to need more than one value in the wrapper.

I have some remarks but I'l make them directly on @samuelgruetter commit as I did before.

samuelgruetter commented 11 years ago

@benjchristensen I'm confused about the jar releases. I thought they have contained @jmhofer 's changes for proper covariance/contravariance handling since 0.11.2, but apparently they don't:

In rxjava-core-0.11.3.jar, flatMap looks like this:

public <R> Observable<R> flatMap(Func1<T, Observable<R>> func) {
    return mapMany(func);
}

On github master, flatMap looks like this (that's what I would like to have in the jar):

public <R> Observable<R> flatMap(Func1<? super T, ? extends Observable<? extends R>> func) {
    return mapMany(func);
}
jmhofer commented 11 years ago

@samuelgruetter If I'm not mistaken, there was no release yet after the merge of the co-/contavariance branch. 0.11.3 was right before that. I'm afraid that you'll have to wait for 0.11.4 (or 0.12, more likely), or you'll have to work directly on the repo build of RxJava.

benjchristensen commented 11 years ago

Those changes will be in 0.12 which has not been released yet. It was merged to master yesterday.

Before releasing I wanted to determine if this thread of discussion required any further changes to core, and I want to get in https://github.com/Netflix/RxJava/pull/349 as well since it generalizes some objects in rx.util which hopefully gets confirmed today.

Is there anything else that if done in core for release 0.12 would improve Scala support?

samuelgruetter commented 11 years ago

I can't currently think of anything that if done in core for release 0.12 would improve Scala support. But probably I'll find such things just after 0.12 is released ;-)

benjchristensen commented 11 years ago

Then once I wrap up those other things (hopefully today) I will release 0.12. I fully expect we'll find more to change ... hence us not being at 1.0 yet :-)

benjchristensen commented 11 years ago

Due to the 2 bugs, are we unable to pursue the route where rx.Observable is the return type at this time?

Also, since I'm unfortunately not yet very experienced with Scala, in your examples would you mind demonstrating a use case where a ScalaObservable is exposed back to Java for interop? I'm interested in understanding how the solution being worked on would function in polyglot environments (while understanding this is a secondary priority).

aloiscochard commented 11 years ago

if you call .wrapped on a ScalaObservable you get back the original Observable which can be used with the Java API.

Maybe we could find a better name that wrapped, not easy though ;-)

jmhofer commented 11 years ago

I'd prefer calling .wrapped .asJava instead (like the collection JavaConverters do).

benjchristensen commented 11 years ago

Version 0.12 has been released.

samuelgruetter commented 11 years ago

I've forked the RxJava repo and I'm now working here.

aloiscochard commented 11 years ago

@samuelgruetter great stuff!

jmhofer commented 11 years ago

@samuelgruetter Good idea!

And: Yay, finally a covariant observable! Awesome! I've been working towards this all the time...

samuelgruetter commented 11 years ago

Well now I have a Scala compiler bug... It's here. I don't know yet what to do about that.