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: Make observeOn not let worker.dispose() called prematurely #6167

Merged
merged 2 commits into from
Aug 22, 2018

Conversation

akarnokd
Copy link
Member

Some operators may call dispose()/cancel from onError/onComplete which could trigger a permature call to worker.dispose() that was about to happen anyway. This PR prevents this by moving the operator into its disposed/cancelled state before signaling the terminal event, thus a downstream cancel()/dispose() call won't trigger this premature cleanup.

Such premature cleanups may cause unwanted Schedulers.io() reuse in some scenarios.

Related: #6146

@akarnokd
Copy link
Member Author

There are 2 x 2 tests that were relying on a race that the previous behavior always allowed. I've updated those tests to avoid the inherent termination-cancel race in them.

@codecov
Copy link

codecov bot commented Aug 22, 2018

Codecov Report

Merging #6167 into 2.x will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #6167      +/-   ##
============================================
- Coverage     98.23%   98.21%   -0.03%     
- Complexity     6198     6199       +1     
============================================
  Files           667      667              
  Lines         44860    44879      +19     
  Branches       6214     6214              
============================================
+ Hits          44070    44076       +6     
- Misses          249      259      +10     
- Partials        541      544       +3
Impacted Files Coverage Δ Complexity Δ
...internal/operators/flowable/FlowableObserveOn.java 96.65% <100%> (-0.46%) 3 <0> (ø)
...operators/completable/CompletableFromCallable.java 100% <100%> (ø) 4 <0> (ø) ⬇️
...rnal/operators/observable/ObservableObserveOn.java 100% <100%> (ø) 3 <0> (ø) ⬇️
.../operators/flowable/FlowableBlockingSubscribe.java 93.02% <0%> (-4.66%) 10% <0%> (-1%)
...ernal/operators/flowable/FlowableFlatMapMaybe.java 90.82% <0%> (-4.35%) 2% <0%> (ø)
...l/operators/observable/ObservableFlatMapMaybe.java 89.54% <0%> (-2.62%) 2% <0%> (ø)
...tivex/internal/schedulers/TrampolineScheduler.java 96.1% <0%> (-2.6%) 6% <0%> (ø)
...ernal/operators/flowable/FlowableFromIterable.java 95.72% <0%> (-1.61%) 5% <0%> (ø)
...ternal/operators/completable/CompletableMerge.java 96.42% <0%> (-1.2%) 2% <0%> (ø)
.../operators/mixed/FlowableConcatMapCompletable.java 99.14% <0%> (-0.86%) 2% <0%> (ø)
... and 20 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 592b991...be80d4d. Read the comment docs.

@akarnokd akarnokd merged commit ab64982 into ReactiveX:2.x Aug 22, 2018
@akarnokd akarnokd deleted the ObserveOnCancelHandling branch August 22, 2018 14:58
@davidmoten
Copy link
Collaborator

For me the situation in #6146 is an anti-pattern (having more than one blocking subscribe in a Flowable chain) so I hope to avoid it entirely. Having said that I understand that this PR helps the situation described in #6146. @akarnokd Would you say the window of opportunity for unwanted Schedulers.io() reuse has been narrowed or completely closed with the changes in this PR?

@akarnokd
Copy link
Member Author

akarnokd commented Aug 23, 2018

It is definitely narrowed but I can't prove it is eliminated or that we aren't back to state where excess io creation happens. It is better to confine blocking to its own io scheduler via subscribeOn so that such onComplete blocking doesn't elongate the scheduler's life and also doesn't keep the previous operator's stack.

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