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

Single and Completable takeUntil() #3708

Closed
ZacSweers opened this issue Feb 14, 2016 · 10 comments
Closed

Single and Completable takeUntil() #3708

ZacSweers opened this issue Feb 14, 2016 · 10 comments
Labels

Comments

@ZacSweers
Copy link
Contributor

In reading the original PR for Single, I saw Ben was OK with adding a takeUntil(Single/Observable) operator for Single later. I was wondering if this was still the plan, and if you would be open to contributions on this front. Same with Completable.

One caveat that I've thought of is that there's a potentially conflicting contact with Single and takeUntil() in the sense that takeUntil() calls onCompleted() in observables, but Singles will actually go to onError() if onCompleted() is called before any events are emitted (and by extension onSuccess()). Not sure what the clear path would be, but it seems like Singles would have to only unsubscribe and not propagate any notifications, differing from Observables and likely Completables in this regard.

@akarnokd
Copy link
Member

Single.takeUntil still has to emit an onSuccess or onError; you need to emit a NoSuchElementException from the other Single.onSuccess. Unsubscribing the main and not emitting anything is bad because it keeps the subscribers hanging. I don't think it is worth it.

Completable.takeUntil is just completable.ambWith(other).

@ZacSweers
Copy link
Contributor Author

The completable makes sense. For Single, do you think there's a feasible way to do a sort of takeUntil-like behavior where it completes/unsubscribes on the emission of another observable/single?

I'm working on some lifecycle handling for android, and a use case that's worked well in the past is to emit lifecycle events via behaviorsubject and then just takeUntil(lifecycleSubject.filter(e -> e == DETACH)). The idea is you want it to unsubscribe when the detach happens, and perhaps out of convenience we've always been using the fact that takeUntil would call onComplete and eventually cause it to unsubscribe.

@ZacSweers
Copy link
Contributor Author

It seems like forcing an onSuccess or onError defeats the purpose of having a subscription that supports unsubscribe prior to that.

@akarnokd
Copy link
Member

The best you can do with takeUntil is to signal an error, like CancellationException similar to how Future does.

@ZacSweers
Copy link
Contributor Author

I see. Would that be something you guys would be open to as PR? Or at least consider making Single#lift() public since it's still experimental? Otherwise it doesn't seem like it's possible in the current API. Could catch existing NoSuchElementExceptions but there'd be no way to know if it was due to completion or cancellation.

Alternatively, would there be a simple way to potentially swallow that error upstream rather than force the subscriber to handle it?

@akarnokd
Copy link
Member

We are open for PRs.

@akarnokd
Copy link
Member

Closing via #3712

@ZacSweers
Copy link
Contributor Author

Actually this just reminded me, would we want Single and Observable overloads for Completable, or just use amb and call toCompletable on those when passing them in?

@akarnokd
Copy link
Member

Maybe, submit a PR and let's see the opinions.

@ZacSweers
Copy link
Contributor Author

Will do, though likely not until this weekend or next week

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

No branches or pull requests

2 participants