These are chat archives for ReactiveX/RxJava

8th
Feb 2018
Yannick Lecaillez
@ylecaillez
Feb 08 2018 11:14
Hi guys !
Just got a weird issues on my project. The source of it lies in BaseEmitter.complete() which invokes the serial.dipose().
This has the side effect of invoking the callback registered with setCancelled(). This is quite surprising and looks like a bug to me ?
David Karnok
@akarnokd
Feb 08 2018 12:21
@ylecaillez So you want your resource to leak?
Yannick Lecaillez
@ylecaillez
Feb 08 2018 13:31
Well, at least i would have expected setCancelled() to behave differently then setDisposable())
when response.cancel() is invoked i want to send a CANCEL request. But surely i don't want to send a CANCEL request when the response is completed.
i manage to solve that using .doOnCancel(), but i found the method naming setCancelled() to be a bit misleading
David Karnok
@akarnokd
Feb 08 2018 13:33
There is no such method setCancelled.
Yannick Lecaillez
@ylecaillez
Feb 08 2018 13:34
Sorry, it is on FlowableEmitter
and it is named setCancellable
David Karnok
@akarnokd
Feb 08 2018 13:35
Disposable and Cancellable are logically equivalent. Cancellable.cancel() allows the implementor to throw a checked exception. They are there to allow resource cleanup when the emitter is terminated or cancelled.
Why don't want you to send a CANCEL to whatever source when your generator decides to end the sequence?
Yannick Lecaillez
@ylecaillez
Feb 08 2018 13:41
I have a RequestState which can be shared amongst multiple request (see it like a transaction). this request state manage the cancellation of the whole transaction.
If RequestState.isCancelled() return true, subsequent request sharing the same RequestState will signal onError(CancelException) on the response Flowable.
I've used setCancellable() to plug the RxJava cancel() to this RequestState.
Not sure this is clear. But my bug was that on the first request completed, the RequestState.cancel() was invoked. The subsequent request were always cancelled without "reason".
the FlowableEmitter.setCancellable() looks more like a Flowable.doOnTerminate() to me
FloableEmitter.doOnTerminate()
David Karnok
@akarnokd
Feb 08 2018 13:44
It acts like doFinally actually. It is on purpose because most use cases involving a resource would leak that resource when the generator terminates. It sounds like you used the wrong abstraction or didn't properly anticipate the sharing of that request state.
Yannick Lecaillez
@ylecaillez
Feb 08 2018 13:45
What do you mean by "used the wrong abstraction" ?
RequestStare are designed to be shared. Everything works fine if i bridge the RequestState using the doOnCancel()
Flowable.create(...).doOnCancel(requestState::cancel) worked while Flowable.create(emitter -> emitter.setCancellable(requestState::cancel)) didn't
My point is just that setCancellable() is a misleading method name given that it is invoked when flowable is cancel() but also when onComplete() or onError().
David Karnok
@akarnokd
Feb 08 2018 13:49
Why would you complete for some consumers in the create() while not for others?
Yannick Lecaillez
@ylecaillez
Feb 08 2018 13:51
Sorry i don't understand your question. Each requests shared by the RequestState have its own Flowable.create()
because each requests is producing a Flowable<Response>
once a request is onComplete(), a Flowable.concat(), subscribe to the next one. If the RequestSTate is cancelled, then the processing will throw and stop subsequent request to be subscribed. Otherwise, the request will produce result, then onComplete(), then the Flowable.concat() will subscribe to the next one. And so on
David Karnok
@akarnokd
Feb 08 2018 13:53
So is setDisposable. The behavior then is provided by the Emitter. I don't see any case for misleading naming, just misunderstanding. The main use case for setCancellable is to release resources created within the emitter callback which are scoped for the duration of the emission. If you have external resources that are shared, you'll need other means to release them considering how a solution would interact with it.
Yannick Lecaillez
@ylecaillez
Feb 08 2018 13:55
But why "Cancellable" then ? Why not "setFinally" or "setFinalizer" or whatever ? I mean cancel() has a special meaning in Flowable. This is the thing i found misleading
But of course YMMV, this is just a remark from a RxJava user :)
I should not have used setCancellable() for my use case, this is an error on my side. BUT i would probably not used it at the first place if it was named "setFinalizer" or sthg like that.
Yannick Lecaillez
@ylecaillez
Feb 08 2018 14:05
Note that the javadoc of FlowbableEmitter.isCancelled() is also misleading: Returns true if the downstream cancelled the sequence.
It'll also returns true if the stream is terminated (onComplete/onError)
David Karnok
@akarnokd
Feb 08 2018 14:06

The governing rule here is 1.6:

"If a Publisher signals either onError or onComplete on a Subscriber, that Subscriber’s Subscription MUST be considered cancelled.
The intent of this rule is to make sure that a Subscription is treated the same no matter if it was cancelled, the Publisher signalled onError or onComplete."

People coming from different backgrounds tend to get confused by different naming. There is often discussion how to name components so that it doesn't imply too much but doesn't go under the radar either. setCancellable follows the typical bean-setter naming indicating you set some object on this emitter.

Yannick Lecaillez
@ylecaillez
Feb 08 2018 14:08
Regarding 1.6: i agree but there is a difference on considering the Subscription cancelled and invoking cancel() in onError(), onComplete().
I agree that the naming makes sense, i just fall in a trap :)
David Karnok
@akarnokd
Feb 08 2018 14:10
I've posted ReactiveX/RxJava#5844 to clarify the properties of the Emitter interfaces.
Yannick Lecaillez
@ylecaillez
Feb 08 2018 14:13
That's absolutely perfect !
Thank you so much :)