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

3.x: Fix window(time) possible interrupts while terminating #6674

Merged
merged 2 commits into from
Oct 17, 2019

Conversation

akarnokd
Copy link
Member

@akarnokd akarnokd commented Oct 17, 2019

Fix the case in window(time) variants where the timer thread is busy with window emission and the upstream terminates on some other thread, the window emission is interrupted.

So instead of disposing the timer/worker right after the upstream termination, a DISPOSE message is queued up. Thus any ongoing drain loop from the timer thread can cleanup gracefully.

After some additional considerations, there is no need for DISPOSE. The drain loop will take care of disposing the timer and the main downstream can simply be terminated.

The 2.x fix will be in a separate PR shortly.

Fixes #6672

@akarnokd akarnokd added this to the 3.0 milestone Oct 17, 2019
@codecov
Copy link

codecov bot commented Oct 17, 2019

Codecov Report

Merging #6674 into 3.x will increase coverage by 0.01%.
The diff coverage is 88.88%.

Impacted file tree graph

@@             Coverage Diff              @@
##                3.x    #6674      +/-   ##
============================================
+ Coverage     98.06%   98.08%   +0.01%     
+ Complexity     6189     6188       -1     
============================================
  Files           677      677              
  Lines         44682    44658      -24     
  Branches       6169     6169              
============================================
- Hits          43819    43802      -17     
+ Misses          314      308       -6     
+ Partials        549      548       -1
Impacted Files Coverage Δ Complexity Δ
...al/operators/observable/ObservableWindowTimed.java 91.26% <87.5%> (+0.53%) 4 <0> (ø) ⬇️
...ternal/operators/flowable/FlowableWindowTimed.java 92.79% <90%> (+0.93%) 4 <0> (ø) ⬇️
...ernal/operators/flowable/FlowableFlatMapMaybe.java 89.58% <0%> (-3.13%) 2% <0%> (ø)
...rxjava3/internal/observers/QueueDrainObserver.java 97.43% <0%> (-2.57%) 21% <0%> (-1%)
...nternal/operators/observable/ObservableWindow.java 98% <0%> (-2%) 3% <0%> (ø)
...tivex/rxjava3/disposables/CompositeDisposable.java 98.14% <0%> (-1.86%) 39% <0%> (-1%)
...ernal/operators/flowable/FlowableFromIterable.java 96.25% <0%> (-1.61%) 5% <0%> (ø)
...va3/internal/operators/flowable/FlowableCache.java 98.48% <0%> (-1.52%) 38% <0%> (-1%)
...internal/operators/flowable/FlowableSwitchMap.java 94.39% <0%> (-1.41%) 3% <0%> (ø)
...java3/internal/operators/flowable/FlowableZip.java 97.39% <0%> (-1.05%) 6% <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 e6406b3...040b15c. Read the comment docs.

@akarnokd
Copy link
Member Author

The coverage complains about the new additions because it is very difficult to establish an interleaving that would skip over the done part the right time and find done == false and o == DISPOSE. I.e., executing the entire onComplete while the drain loop is between reading done and polling the queue.

@vanniktech
Copy link
Collaborator

Let me know once you're done so I can review.

@akarnokd
Copy link
Member Author

That's it. Can't make the coverage better.

@akarnokd akarnokd merged commit c1a919c into ReactiveX:3.x Oct 17, 2019
@akarnokd akarnokd deleted the WindowTimedInterruptFix branch October 17, 2019 16:23
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.

Question: Why a running computation thread got interrupted while window is finished
2 participants