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

2.x: Don't dispose the winner of {Single|Maybe|Completable}.amb() #6375

Merged
merged 2 commits into from
Jan 17, 2019

Conversation

akarnokd
Copy link
Member

This PR fixes the Single.amb, Maybe.amb and Completable.amb operators to not dispose the winner source, causing potential interruptions as a side-effect on the current thread.

The fix is to delete the winner from the composite before disposing the rest to avoid the interruption race.

Unit tests were added to verify this behavior on all amb implementations. Flowable and Observable were already working correctly.

Fixes: #6373

@akarnokd akarnokd added this to the 2.2 backlog milestone Jan 17, 2019
Copy link
Collaborator

@vanniktech vanniktech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL: about delete()

@codecov
Copy link

codecov bot commented Jan 17, 2019

Codecov Report

Merging #6375 into 2.x will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #6375      +/-   ##
============================================
+ Coverage     98.24%   98.27%   +0.02%     
- Complexity     6282     6288       +6     
============================================
  Files           673      673              
  Lines         45023    45035      +12     
  Branches       6226     6226              
============================================
+ Hits          44235    44259      +24     
+ Misses          248      243       -5     
+ Partials        540      533       -7
Impacted Files Coverage Δ Complexity Δ
...internal/operators/completable/CompletableAmb.java 100% <100%> (ø) 11 <0> (ø) ⬇️
...o/reactivex/internal/operators/maybe/MaybeAmb.java 100% <100%> (ø) 11 <0> (+1) ⬆️
...reactivex/internal/operators/single/SingleAmb.java 100% <100%> (ø) 10 <0> (ø) ⬇️
.../operators/flowable/FlowableBlockingSubscribe.java 93.02% <0%> (-4.66%) 10% <0%> (-1%)
...rnal/operators/flowable/FlowableFlatMapSingle.java 92.93% <0%> (-3.81%) 2% <0%> (ø)
...l/operators/observable/ObservableFlatMapMaybe.java 90.19% <0%> (-3.27%) 2% <0%> (ø)
...ava/io/reactivex/processors/BehaviorProcessor.java 96.86% <0%> (-2.25%) 60% <0%> (ø)
...perators/single/SingleFlatMapIterableFlowable.java 95.83% <0%> (-1.67%) 2% <0%> (ø)
...java/io/reactivex/processors/UnicastProcessor.java 98.8% <0%> (-1.2%) 67% <0%> (-1%)
...perators/observable/ObservableMergeWithSingle.java 99.06% <0%> (-0.94%) 2% <0%> (ø)
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1b3838...15f5dfe. Read the comment docs.

@akarnokd akarnokd merged commit d40f923 into ReactiveX:2.x Jan 17, 2019
@akarnokd akarnokd deleted the AmbNoWinnerDispose branch January 17, 2019 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Single.observeOn wrongly interrupt thread
2 participants