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 refCount() not resetting when cross-canceled #6629

Merged
merged 2 commits into from
Aug 22, 2019

Conversation

akarnokd
Copy link
Member

This PR fixes the issue with refCount not resetting the connection when the termination triggers cross-cancellation over it.

Fixes #6608

The fix is more involved than #6609 because how 2.x uses two publish() implementation internally due to bugfix #6505. The old/classic implementation does not fail #6608 but the newer implementation fails #6608. If the fix is applied unconditionally, the old/classic implementation fails an older unit test verifying an error allows reconnection. Therefore, the PR checks and applies the new code path only if refCount isn't talking to the classic publish implementation.

As a reminder #6609 for 3.x has a redesigned Connectable with a much more clearer reset semantics and thus the restructuring of the termination handling had no trouble passing the aforementioned error-allows-reconnect unit test.

@codecov
Copy link

codecov bot commented Aug 22, 2019

Codecov Report

Merging #6629 into 2.x will decrease coverage by 0.04%.
The diff coverage is 95%.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #6629      +/-   ##
============================================
- Coverage     98.22%   98.18%   -0.05%     
- Complexity     6336     6342       +6     
============================================
  Files           677      677              
  Lines         45523    45545      +22     
  Branches       6330     6332       +2     
============================================
+ Hits          44717    44719       +2     
- Misses          257      261       +4     
- Partials        549      565      +16
Impacted Files Coverage Δ Complexity Δ
...ernal/operators/observable/ObservableRefCount.java 99.12% <95%> (-0.88%) 32 <5> (+4)
.../internal/operators/flowable/FlowableRefCount.java 99.13% <95%> (-0.87%) 32 <5> (+4)
...l/operators/observable/ObservableFlatMapMaybe.java 84.96% <0%> (-5.89%) 2% <0%> (ø)
...va/io/reactivex/internal/queue/SpscArrayQueue.java 97.61% <0%> (-2.39%) 22% <0%> (-1%)
...nternal/operators/parallel/ParallelSortedJoin.java 92.7% <0%> (-2.19%) 2% <0%> (ø)
...ernal/operators/flowable/FlowableFromIterable.java 94.11% <0%> (-2.14%) 5% <0%> (ø)
...nal/operators/observable/ObservablePublishAlt.java 96.26% <0%> (-1.87%) 15% <0%> (-1%)
...perators/mixed/ObservableConcatMapCompletable.java 98.49% <0%> (-1.51%) 3% <0%> (ø)
...a/io/reactivex/internal/util/QueueDrainHelper.java 98.61% <0%> (-1.39%) 57% <0%> (-1%)
...perators/mixed/ObservableSwitchMapCompletable.java 98.94% <0%> (-1.06%) 3% <0%> (ø)
... 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 a9df239...2973758. Read the comment docs.

@akarnokd akarnokd merged commit fa406d1 into ReactiveX:2.x Aug 22, 2019
@akarnokd akarnokd deleted the RefCountCancelFix2xa branch August 22, 2019 20:08
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.

None yet

2 participants