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

Make defer() wait until request > 0 before calling Func0 #3816

Closed
JakeWharton opened this issue Mar 31, 2016 · 6 comments
Closed

Make defer() wait until request > 0 before calling Func0 #3816

JakeWharton opened this issue Mar 31, 2016 · 6 comments

Comments

@JakeWharton
Copy link
Contributor

Currently the defer operator calls its supplied Func0 immediately upon subscription as documented. Because this operator is usually used to delay work of some kind, this can prematurely trigger that work before the downstream consumer actually requests a non-zero amount.

A failing test, to illustrate:

AtomicBoolean b = new AtomicBoolean();
Observable<Boolean> bo = Observable.defer(() -> Observable.just(b.get()));

TestSubscriber s = new TestSubscriber(0);
bo.subscribe(s);
s.assertNoValues();

b.set(true);
s.requestMore(1);
s.assertValues(true);

In this test, the "work" (aka b.get()) ran before the downstream consumer actually wanted a value produced. If you think of b.get() as, say, an HTTP request or something based on time, etc. the problem becomes more clear.

I haven't looked, but I would guess that defer's behavior was implemented prior to the backpressure concept being introduced.

I'll leave it up to the library maintainers to determine the risk of changing the behavior, but I want to again note one thing which started this conversation (from comments in #3780): there currently doesn't exist an easy, sane, stable API for deferring work that is backpressure aware as described above. So if it's determined to be too risky to alter defer, fast-tracking fromCallable to stable and things like SyncOnSubscribe would be useful. This is of great concern to library developers, not so much application developers, where only stable APIs can safely be used.

@akarnokd
Copy link
Member

Defer has nothing to do with backpressure because it just routes the subscriber to a generated Observable. Deferring the creation of the actual Observable until the first non-zero request requires completely different internals.

Besides, there are standard, stable API tools for your case:

defer(() -> just("whatever").map(v -> b.get()));

@JakeWharton
Copy link
Contributor Author

I'm not saying it can't be done with the stable APIs, just that it's a bit more convoluted than should be necessary.

Defer has nothing to do with backpressure because it just routes the subscriber to a generated Observable.

But would you write that same behavior if implementing defer today? Or would you have it wait until request(1)?

@akarnokd
Copy link
Member

I've written defer() twice for different projects now and would do it again without interfering with requests.

In addition, if defer() would react to request only, that would expose you to request patterns of the downstream operators. For example, given such a deferUntilRequested operator, if you apply observeOn it, the prefetch will immediately trigger the source before the end-Subscriber requested anything at all.

@JakeWharton
Copy link
Contributor Author

So does that mean you'd support this change to defer() as a bugfix, or do you think it's a breaking change in behavior such that we're stuck with what we've got / need a new API?

I'm perfectly happy if the answer is "we can't change this" and the focus should be on fromCallable and SyncOnSubscribe instead.

In general I'm 👎 on a new operator since it would have to go through stabalization behind both fromCallable and SyncOnSubscribe and it's not clear that anyone wants that operator over those two alternatives if they were stable.

@akarnokd
Copy link
Member

I don't support changing defer and perhaps fromCallable should be fast tracked to stable version.

@JakeWharton
Copy link
Contributor Author

Sounds good 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants