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() connect/subscribe/cancel deadlock #5975

Merged
merged 5 commits into from
Apr 29, 2018

Conversation

akarnokd
Copy link
Member

This PR fixes a deadlock issue with the refCount operator when a subscription leads to a blocking execution while the lock is being held, preventing other subscription or cancellation from executing on other threads.

The bug was discovered as the cause of a reported hang on the Google groups.

The code has been developed in the extensions project where the operator has more features. The overloads supporting these features can be added in a separate enhancement PR.

@akarnokd akarnokd added this to the 2.2 milestone Apr 28, 2018
public void blockingSourceAsnycCancel() throws Exception {
BehaviorProcessor<Integer> bp = BehaviorProcessor.createDefault(1);

Flowable<Integer> o = bp
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be f

@codecov
Copy link

codecov bot commented Apr 28, 2018

Codecov Report

Merging #5975 into 2.x will decrease coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##                2.x   #5975      +/-   ##
===========================================
- Coverage     98.27%   98.2%   -0.07%     
- Complexity     6019    6049      +30     
===========================================
  Files           656     656              
  Lines         44037   44074      +37     
  Branches       6100    6118      +18     
===========================================
+ Hits          43278   43285       +7     
- Misses          224     246      +22     
- Partials        535     543       +8
Impacted Files Coverage Δ Complexity Δ
.../internal/operators/flowable/FlowableRefCount.java 100% <100%> (ø) 22 <22> (+15) ⬆️
...ernal/operators/observable/ObservableRefCount.java 100% <100%> (ø) 22 <22> (+15) ⬆️
...l/operators/observable/ObservableFlatMapMaybe.java 89.54% <0%> (-4.58%) 2% <0%> (ø)
...rnal/operators/flowable/FlowableFlatMapSingle.java 92.93% <0%> (-3.81%) 2% <0%> (ø)
...a/io/reactivex/internal/util/QueueDrainHelper.java 95.83% <0%> (-2.78%) 55% <0%> (-2%)
...tivex/internal/schedulers/TrampolineScheduler.java 96.1% <0%> (-2.6%) 6% <0%> (ø)
...perators/single/SingleFlatMapIterableFlowable.java 95.83% <0%> (-2.5%) 2% <0%> (ø)
...ava/io/reactivex/processors/BehaviorProcessor.java 96.86% <0%> (-2.25%) 60% <0%> (ø)
.../io/reactivex/internal/schedulers/IoScheduler.java 89.24% <0%> (-2.16%) 9% <0%> (ø)
.../io/reactivex/disposables/CompositeDisposable.java 98.14% <0%> (-1.86%) 39% <0%> (-1%)
... and 23 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 be12fe2...53bddc0. Read the comment docs.

@akarnokd
Copy link
Member Author

/cc @davidmoten as you devised the previous refCount algorithm.

@akarnokd akarnokd merged commit 5b929c4 into ReactiveX:2.x Apr 29, 2018
@akarnokd akarnokd deleted the RefCountDeadlockFix branch April 29, 2018 08:39
@davidmoten
Copy link
Collaborator

👍 Great to see that bit of locking removed. I actually had launched into a non-blocking rewrite of the refCount operator a couple of months ago and had it working with current tests but started bringing in tests from 1.x and got a failure or two with some quite unusual tests, then jumped to something else leaving it on my to-do queue. I'll cast another look at the rewrite soon.

Copy link
Contributor

@artem-zinnatullin artem-zinnatullin left a comment

Choose a reason for hiding this comment

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

looks good, but comment :)

}

long c = conn.subscriberCount;
if (c == 0L && conn.timer != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like this condition is only actual for resubscribeBeforeTimeout test

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 strange to see logic and tests for something that user cannot use in RxJava

As you mentioned in PR description, let's maybe expose additional time related refCount api or try to shrink unused code?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done: #5986.

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.

4 participants