2.x: Fix cancel/dispose upon upstream switch for some operators #6258
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## 2.x #6258 +/- ##
============================================
+ Coverage 98.22% 98.28% +0.05%
- Complexity 6203 6210 +7
============================================
Files 667 667
Lines 44889 44898 +9
Branches 6216 6219 +3
============================================
+ Hits 44093 44126 +33
+ Misses 254 242 -12
+ Partials 542 530 -12
Continue to review full report at Codecov.
|
Contributor
|
David, can you please allow 2-3 days window for review on this one? |
Member
Author
|
Sure Artem. |
vanniktech
approved these changes
Oct 26, 2018
isfaaghyth
approved these changes
Oct 27, 2018
artem-zinnatullin
approved these changes
Oct 27, 2018
| child.onNext(t); | ||
| public void onNext(Object t) { | ||
| Subscription s = get(); | ||
| if (s != SubscriptionHelper.CANCELLED) { |
Contributor
There was a problem hiding this comment.
offtopic: I'm wondering if we should drop SubscriptionHelper.isCancelled()
Member
Author
There was a problem hiding this comment.
If you want to inline all of them.
artem-zinnatullin
approved these changes
Oct 27, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR extends the
SubscriptionArbiterto optionally allow or disallow cancelling the currentSubscriptionif it is replaced by a new one. Some operators do not need to cancel the currentSubscription:concat,concatMap,repeat,repeatWhen,retryandretryWhen.In addition
repeatWhenandretryWhenwere cancelling when the handler sequence itself terminated. The code has been updated to disconnect the upstream upon the completion/failure but before signaling the handler.The Reactive Streams specification also disallows synchronous cancellation after the terminal event anyway.
Others may actually need to cancel, such as
Timeout.Observables don't have a specific arbiter, they use theDisposableHelpermethods and the relevant ones were changed toreplace()instead of the disposingsetcall.Some tests actually checking if the dispose/cancel happens and had to be updated.
The
Flowable.delaySubscription(Publisher)also usedSubscriptionArbiterbut it was unnecessary. The code has been replaced with a more apt deferred requesting scheme as the downstream requests need to be delayed until the main subscription happens, the other publisher is always consumed unbounded.Resolves: #6259