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: BehaviorProcessor & Subject terminate-subscribe race #5281

Merged
merged 2 commits into from
Apr 12, 2017

Conversation

akarnokd
Copy link
Member

This PR fixes the race condition in BehaviorProcessor and BehaviorSubject when onComplete() or onError() is called concurrently with subscribe and the consumer throws a NullPointerException instead of relaying the terminal event.

The fix involves having a separate terminalEvent atomic field, CAS-ing in the actual or marker Throwable and reading that field in subscribe.


TestHelper.race(r1, r2);

ts.assertError(TestException.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

assertFailure()? it also checks that no completion events were emitted

@codecov
Copy link

codecov bot commented Apr 12, 2017

Codecov Report

Merging #5281 into 2.x will decrease coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #5281      +/-   ##
============================================
- Coverage     96.12%   96.01%   -0.11%     
+ Complexity     5755     5745      -10     
============================================
  Files           628      628              
  Lines         41079    41077       -2     
  Branches       5699     5699              
============================================
- Hits          39486    39441      -45     
- Misses          631      653      +22     
- Partials        962      983      +21
Impacted Files Coverage Δ Complexity Δ
...in/java/io/reactivex/subjects/BehaviorSubject.java 84.89% <100%> (+0.95%) 56 <5> (+1) ⬆️
...ava/io/reactivex/processors/BehaviorProcessor.java 87.61% <100%> (-0.06%) 61 <5> (ø)
...al/operators/observable/ObservableSampleTimed.java 88.33% <0%> (-10%) 3% <0%> (ø)
...a/io/reactivex/internal/util/QueueDrainHelper.java 58.86% <0%> (-5.68%) 31% <0%> (-4%)
...reactivex/internal/operators/single/SingleAmb.java 96.36% <0%> (-3.64%) 9% <0%> (-1%)
...internal/operators/completable/CompletableAmb.java 94.91% <0%> (-3.39%) 10% <0%> (-1%)
...ernal/operators/flowable/FlowableFromIterable.java 91.97% <0%> (-3.21%) 5% <0%> (ø)
...x/internal/operators/maybe/MaybeSwitchIfEmpty.java 97.22% <0%> (-2.78%) 2% <0%> (ø)
...rnal/operators/flowable/FlowableTakeLastTimed.java 96.29% <0%> (-2.78%) 2% <0%> (ø)
...activex/internal/observers/QueueDrainObserver.java 61.53% <0%> (-2.57%) 12% <0%> (-1%)
... and 36 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 8a78c74...fa780f4. Read the comment docs.

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.

UnicastSubject as well as SerializedSubject still have the boolean done. Should those be changed for consistency too?

@akarnokd
Copy link
Member Author

No need for that.

@akarnokd akarnokd merged commit 85e0ea5 into ReactiveX:2.x Apr 12, 2017
@akarnokd akarnokd deleted the BehaviorProcessorFix branch April 12, 2017 13:24
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.

3 participants