JVM concat throws if one observable is a single

[jconti]

user> (def numbers (rx/from-coll (range 10)))
#'user/numbers
user> (rx/on-value numbers println)
0
1
2
3
4
5
6
7
8
9
#object[beicon.core$wrap_disposable$reify__264 0x79bd924e "beicon.core$wrap_disposable$reify__264@79bd924e"]
user> (rx/on-value (rx/reduce + 0 numbers) println)
45
#object[beicon.core$wrap_disposable$reify__264 0x2de5e902 "beicon.core$wrap_disposable$reify__264@2de5e902"]
user> (rx/on-value (rx/concat numbers (rx/reduce + 0 numbers)) println)
ExceptionInfo Invalid arguments.  clojure.core/ex-info (core.clj:4617)
user> (rx/observable? numbers)
true
user> (rx/observable? (rx/reduce + 0 numbers))
false
user> (rx/single? (rx/reduce + 0 numbers))
true

I am running beicon 4.1.0 with Clojure 8.0 and JVM 8. I plan to send a pull request.

[jconti]

#31 Installed support for Flowable. But it turns out that Singles are not Observables, which ends up being a similar situation. Such is the unfortunate design of RxJava2. So glad we have beicon to deal with it!

[niwinz]

yeah, is clearly a bug

[jconti]

I have had the problem with both flowables and observables. A fix is complicated by that issue. I am wondering if the resulting fix would touch concat, observable?, and flowable?. And if so, what other destabilizing issues I might induce.

I say this off the top of my head. My thought is that if every param to concat is observable? or an observable single then concat can be applied to those arguments, with the the singles coerced with .toObservable. I think the same type of logic would apply to the flowable.

This is not the behavior of observable? and flowable? today. The change I am proposing would be that the return value of (rx/reduce + 0 numbers) would have both single? and observable? return true on it (since it is an Observable Single). The analogous change would be made to flowable?.

At that point the use of every? in concat would continue to work, but a second step of coercing the singles to the appropriate choice of Observable or Flowable would need to be done. At that point I have to wonder what other functions may need that to handle singles (flat-map likely). In which case does it then make sense to have a coercer used throughout the code base like as-observable to convert singles or other classes to observables. There would be an as-flowable as well to do the same for those.

Reflecting on that idea I think: as nice as having Flowable and Observable is for the implementers of RxJava, I think it is broken to have two separate concrete instantiations of the reactive interfaces (Flowable and Observable) in one implementation. It breaks the polymorphic access to the interfaces, forcing the user to deal with classes specific to this implementation (ObservableSingle and FlowableSingle). Seems like RxJava2 needed to focus on Flowable, and have it be a no-op if backpressure was not implemented (and throw the appropriate exception if observables were over-run).

But again, that is thinking off the top of my head. The only reason I mention that is to provoke enough review of this issue prior to implementing a fix for concat. I do not want to change the meaning of those predicate functions (flowable? and observable?) and break a bunch of stuff that is working. At the same time, to have random beicon functions fail on Singles in not a nice thing either. So it would be good to have a comprehensive way to handle these cases, because they are a result of the way RxJava is implemented.

[niwinz]

I have pushed the commit 1fa9736 to approach this issue, let me know that you are think about it :D

[jconti]

My only comment is I liked the coercions being public. It made using other parts of rxjava easy and smooth. I do not see a reason to make them private. Their functionality is well defined, and matches rxjava nicely.

(defn group-by-ob
  [f ob]
  (.groupBy ob (rx/as-function f)))

[niwinz]

ok, i will make them public again.

[jconti]

thanks!

[jconti]

I think change does a good job of keeping the current approach intact of not trying to wrap the rx-java library into another set of abstractions which would just add a ton of weight and no value. It is a hassle when these concrete class leak out, but it is the lesser of the evils IMO. Thanks for the help!