Removing onSetProducer from the API

[abersnaze]

No description provided.

[cloudbees-pull-request-builder]

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

[davidmoten]

This simplification looks good. I'd be interested to see the discussion behind this. I take it occurred offline but can you give a summary or copy it through?

[abersnaze]

It started because I lobbed this #1452 at @benjchristensen yesterday. I'm not 100% sold on keeping onStart() because anything in there could be done on the Subscriber before it is returned by the Operator.call(). Ben convinced me that it is more in line with reactive-stream onSubscribe(Subscription). He did like the removal onSetProducer(). While we were at it we made request(long n) protected.

[benjchristensen]

• The onSetProducer method always felt ugly and wrong, but was added to solve a problem and I left it there but never liked it.
• The onStart was added to be more explicit rather than requiring either (1) an overloaded constructor (which is what I first did) or (2) requiring users to define the Subscriber and then call request on it which prevent inline declarations.

@abersnaze pushed me to clean these up and we spent time discussing them and came to the following conclusions:

• The onSetProducer can be removed, but at the risk of a very nuanced behavior (the problem I mentioned above) if someone then calls Subscriber.request, since it will no longer do anything if setProducer is over-written.
• To prevent this non-obvious behavior, Subscriber.request was made protected so it can only be called by the class implementing the setProducer so they can make the right decision, and because request doesn't really make sense any longer on a Subscriber that is decoupling the setProducer chain.
• We kept onStart because making request protected effectively forces us to have a hook inside the class for invoking request (it can no longer be done externally even if we wanted to go that route) and we still don't like the constructor.

If I wasn't trying to make these changes additive (and was willing to do massive breaking changes), I would likely do some things differently, but the constraint we have is to not fundamentally break RxJava 0.x while getting to 1.0.

[rtyley]

As Subscriber.request is now protected, does this mean users of RxJava should only use the request() method as outlined the backpressure wiki doc, ie only from the onStart() and onNext() methods within Subscriber?

What if there is some other external event that means I would like to request items from the producer?

(my case is an Android scrolling view, where I would only like items calculated on an Observable.from(expensiveIterable) if the user has scrolled near to the bottom of the list of so-far calculated data)

[zsxwing]

(my case is an Android scrolling view, where I would only like items calculated on an Observable.from(expensiveIterable) if the user has scrolled near to the bottom of the list of so-far calculated data)

If I understand correctly, it's triggered by a UI event, which is hot. It cannot work well with request as description in Wiki:

Cold Observables are ideal subjects for the reactive pull model of backpressure described below. Hot observables are typically not designed to cope well with a reactive pull model, and are better candidates for some of the other flow control strategies discussed on this page, such as the use of the onBackpressureBuffer or onBackpressureDrop operators, throttling, buffers, or windows.

[benjchristensen]

If you want to call request externally you can choose to expose a method to do so with another method than then calls request(n). For example: https://github.com/ReactiveX/RxJava/blob/1.x/src/main/java/rx/observers/TestSubscriber.java#L142

It was made protected because it is generally intended for inner use only.

[benjchristensen]

What if there is some other external event that means I would like to request items from the producer?

I generally try to compose all events via combinatorial operators such as zip and combineLatest so the Subscriber receives an onNext and can make the decision to request more based on composed events.