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

skipUntil doesn't unsubscribe notifier after first event #1886

Closed
matthewwithanm opened this issue Aug 18, 2016 · 5 comments
Closed

skipUntil doesn't unsubscribe notifier after first event #1886

matthewwithanm opened this issue Aug 18, 2016 · 5 comments
Labels
bug Confirmed bug

Comments

@matthewwithanm
Copy link

RxJS version: 5.0.0-beta11

Code to reproduce:

Observable.never().skipUntil(Observable.interval(100).do(x => console.log(x))).subscribe()

Expected behavior:

0 will be logged

Actual behavior:

Lots of numbers are logged.

Additional information:

This works as expected in v4.

@trxcllnt
Copy link
Member

Looks like skipUntil doesn't close its subscription to the notifier unless the source completes, which isn't happening in this case. And while I'm thinking about it, skip/takeUntil should both be refactored to subscribe to the notifier after their source subscription has occurred, so they don't superfluously subscribe to the notifier if the source completes synchronously.

@benlesh
Copy link
Member

benlesh commented Aug 18, 2016

@trxcllnt I know this is horrifying... But you could have a synchronous source observable with a takeUntil notifier that's a subject, and side effect next into that subject during the execution of the synchronous source observable. So you probably don't want to wait

@benlesh benlesh added the bug Confirmed bug label Aug 18, 2016
@trxcllnt
Copy link
Member

@Blesh I mean you could do anything, but that's not how it's supposed to work.

@benlesh
Copy link
Member

benlesh commented Aug 18, 2016

I guess what I'm disagreeing with is here:

skip/takeUntil should both be refactored to subscribe to the notifier after their source subscription has occurred

The notifier should probably be subscribed to first. That way if it notifies synchronously it effects the source. In case the source firehoses synchronously.

benlesh added a commit to benlesh/rxjs that referenced this issue Apr 2, 2018
- No longer waits until notifier is complete to complete resulting observable
- Unsubs from notifier after first notification
- Updates tests to be correct
- Corrects some grammar in test descriptions

fixes ReactiveX#1886
@benlesh benlesh closed this as completed in 889f84a Apr 2, 2018
@lock
Copy link

lock bot commented Jun 5, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Confirmed bug
Projects
None yet
Development

No branches or pull requests

3 participants