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

fix(race): unsubscribe raced observables with immediate scheduler #2158

Merged
merged 1 commit into from
Dec 12, 2016

Conversation

ktkiiski
Copy link
Contributor

Description:

This fixes a bug with the race operator: whenever a raced Observable emitted a value immediately when subscribed (with the default scheduler), any following Observables were subscribed but never unsubscribed.

Now, if any raced Observable emits a value immediately, any latter Observables are ignored, because for them "the race is already lost".

Also, it is now ensured that the immediately emitting Observable is still added to the RaceSubscriber and will be unsubscribed once the outer Observable is unsubscribed.

Fixed the bug where racing an Observable that emits immediately,
with default scheduler, leaves other raced Observables unsubscribed
forever. Now immediate emit makes the race ignore following Observables.
@coveralls
Copy link

coveralls commented Nov 29, 2016

Coverage Status

Coverage remained the same at 97.686% when pulling cb98652 on ktkiiski:fix-race-unsubscription into f51b8f9 on ReactiveX:master.

@kwonoj
Copy link
Member

kwonoj commented Nov 29, 2016

Looks good to me.

@trxcllnt
Copy link
Member

trxcllnt commented Nov 29, 2016

LGTM, but can the unit tests be implemented as marbles like the rest of them?

@kwonoj
Copy link
Member

kwonoj commented Nov 29, 2016

haven't tried it yet but seems some of the test may able to use marbles. @ktkiiski, what do you think?

@ktkiiski
Copy link
Contributor Author

I actually first wrote the specs with marbles, but they did not bust the original bug. This is probably because the marble tests are based on VirtualTimeScheduler (?), and it seems that the bug only occurred with the default (immediate) scheduler, when the value was immediately emitted on subscription.

@kwonoj
Copy link
Member

kwonoj commented Nov 29, 2016

Ahhhh forgot that. VirtualTimeScheduler is kind of AsyncScheduler, so if test aims for default scheduler it may not work. @trxcllnt, I think PR's good as-is. any suggestions?

@trxcllnt
Copy link
Member

After reviewing the source, it seems the test cold Observables are missing the ability to synchronously emit values on subscription. I was expecting something like cold('(x)--|') to emit x upon subscription. Don't want to block this PR until that gets implemented, so LGTM.

@benlesh
Copy link
Member

benlesh commented Dec 12, 2016

LGTM as well.

@benlesh benlesh merged commit 7dd533b into ReactiveX:master Dec 12, 2016
@lock
Copy link

lock bot commented Jun 6, 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 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants