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 Observable.switchMap main onError not disposing the current inner source #5833

Merged
merged 2 commits into from
Feb 2, 2018

Conversation

akarnokd
Copy link
Member

@akarnokd akarnokd commented Feb 1, 2018

The Observable.switchMap had bad logic in its main onError handler which didn't dispose the current inner Observable.

The Flowable version didn't have this logic error. Both variants received an unit test to verify the correct behavior.

Fixes #5832.

@akarnokd akarnokd added this to the 2.2 milestone Feb 1, 2018
@akarnokd akarnokd changed the title 2.x: Fix Obs.switchMap main onError not disposing the current inner src 2.x: Fix Observable.switchMap main onError not disposing the current inner source Feb 1, 2018
@codecov
Copy link

codecov bot commented Feb 1, 2018

Codecov Report

Merging #5833 into 2.x will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #5833      +/-   ##
============================================
+ Coverage     96.42%   96.44%   +0.01%     
+ Complexity     5816     5815       -1     
============================================
  Files           634      634              
  Lines         41761    41760       -1     
  Branches       5796     5796              
============================================
+ Hits          40268    40275       +7     
+ Misses          578      573       -5     
+ Partials        915      912       -3
Impacted Files Coverage Δ Complexity Δ
...rnal/operators/observable/ObservableSwitchMap.java 90.62% <100%> (+1.8%) 3 <0> (ø) ⬇️
.../operators/completable/CompletableConcatArray.java 93.75% <0%> (-6.25%) 2% <0%> (ø)
.../operators/flowable/FlowableBlockingSubscribe.java 91.89% <0%> (-5.41%) 9% <0%> (-1%)
.../internal/operators/flowable/FlowableInterval.java 94.44% <0%> (-2.78%) 3% <0%> (ø)
...activex/internal/observers/QueueDrainObserver.java 61.53% <0%> (-2.57%) 12% <0%> (-1%)
...va/io/reactivex/internal/queue/SpscArrayQueue.java 97.61% <0%> (-2.39%) 22% <0%> (-1%)
...l/operators/observable/ObservableFlatMapMaybe.java 93.46% <0%> (-1.97%) 2% <0%> (ø)
...x/internal/operators/flowable/FlowablePublish.java 93.83% <0%> (-1.77%) 10% <0%> (ø)
...a/io/reactivex/internal/util/QueueDrainHelper.java 61.8% <0%> (-1.39%) 35% <0%> (-1%)
...ternal/operators/observable/ObservablePublish.java 95.61% <0%> (-0.88%) 10% <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 1fbc44f...cfa17ae. Read the comment docs.

@@ -131,15 +131,15 @@ public void onNext(T t) {

@Override
public void onError(Throwable t) {
if (done || !errors.addThrowable(t)) {
if (!done && errors.addThrowable(t)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Codecov indicates that some of possible variants are not tested, do you want to cover them in this PR?

screen shot 2018-02-01 at 4 48 30 pm

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not that I think code in the patch is broken, but that if we had test that was trying to go into onError when it's already done == true, we would have a hint that original code had some problems

Copy link
Member Author

Choose a reason for hiding this comment

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

That line is tested but parallelism on the CI is flaky and may not produce the necessary interleaving there.

The upstream may onError after a crash in the mapper thus we have to route that into the global error handler.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we test it just by breaking RS protocol with main.onComplete(); main.onError()?

I might miss something, reviewing on mobile

Copy link
Member Author

Choose a reason for hiding this comment

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

This and this tests should cover that line. Apparently, the test did not involve ps2 so the inner source was not involved. Updated the unit tests accordingly.

@akarnokd akarnokd merged commit e76a476 into ReactiveX:2.x Feb 2, 2018
@akarnokd akarnokd deleted the ObservableSwitchMapFix branch February 2, 2018 09:56
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