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: Fix flatMap inner fused poll crash not cancelling the upstream #5792

Merged
merged 2 commits into from
Jan 5, 2018

Conversation

akarnokd
Copy link
Member

@akarnokd akarnokd commented Jan 5, 2018

This PR fixes the lack of upstream cancel() call when an inner, fused source's queue.poll() crashes in a non-delayed error mode.

Unit tests were added to verify Observable.flatMap, Flowable.flatMapIterable and Observable.flatMapIterable as well.

Fixes #5791

@codecov
Copy link

codecov bot commented Jan 5, 2018

Codecov Report

Merging #5792 into 2.x will decrease coverage by 0.03%.
The diff coverage is 90%.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #5792      +/-   ##
============================================
- Coverage     96.29%   96.26%   -0.04%     
+ Complexity     5809     5806       -3     
============================================
  Files           634      634              
  Lines         41607    41609       +2     
  Branches       5770     5771       +1     
============================================
- Hits          40066    40055      -11     
+ Misses          614      612       -2     
- Partials        927      942      +15
Impacted Files Coverage Δ Complexity Δ
...x/internal/operators/flowable/FlowableFlatMap.java 89.47% <90%> (+1.11%) 4 <0> (ø) ⬇️
...in/java/io/reactivex/subjects/BehaviorSubject.java 86.24% <0%> (-5.83%) 54% <0%> (ø)
.../operators/flowable/FlowableBlockingSubscribe.java 91.89% <0%> (-5.41%) 9% <0%> (-1%)
...nternal/operators/parallel/ParallelReduceFull.java 91.17% <0%> (-3.93%) 2% <0%> (ø)
...ternal/operators/observable/ObservablePublish.java 92.1% <0%> (-3.51%) 10% <0%> (ø)
.../io/reactivex/internal/schedulers/IoScheduler.java 89.24% <0%> (-3.23%) 9% <0%> (ø)
.../operators/observable/ObservableFlatMapSingle.java 88.8% <0%> (-2.99%) 2% <0%> (ø)
.../internal/operators/flowable/FlowableInterval.java 94.44% <0%> (-2.78%) 3% <0%> (ø)
...va/io/reactivex/internal/queue/SpscArrayQueue.java 97.61% <0%> (-2.39%) 22% <0%> (-1%)
.../main/java/io/reactivex/subjects/MaybeSubject.java 97.75% <0%> (-2.25%) 45% <0%> (-1%)
... and 21 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 709ccd6...0b70b05. Read the comment docs.

@@ -482,6 +482,9 @@ void drainLoop() {
Exceptions.throwIfFatal(ex);
is.dispose();
errs.addThrowable(ex);
if (!delayErrors) {
upstream.cancel();
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is bug fix, but what's the reason for renaming s to upstream?

It looks not important to revert but I guess minimizing fix PRs saves time for those who review them while going through changelog :)

Copy link
Member Author

Choose a reason for hiding this comment

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

It reads nicer now.

@akarnokd akarnokd merged commit d91ee5a into ReactiveX:2.x Jan 5, 2018
@akarnokd akarnokd deleted the FlatMapFusedCrashCancelFix branch January 5, 2018 23:26
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.

2.x: part of the stream continues after error in Flowable but not Observable.
3 participants