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 bounded replay() memory leak due to bad node retention #6371

Merged
merged 1 commit into from
Jan 17, 2019

Conversation

akarnokd
Copy link
Member

@akarnokd akarnokd commented Jan 16, 2019

This PR fixes a memory leak caused by node retention in the bounded replay() implementations. When the consumer disposed/canceled the flow, the index field was not cleared. In certain situations, such out-of-comission consumers were still reachable via thread local stack and through it, the node they were last refencing. Further items through it would then keep referencing an ever increasing number of linked nodes, causing OOME eventually.

The fix is to clear the index field upon dispose/cancellation. The subject/processor variants were working properly. Tests were added to verify them though as well.

Issue discovered on StackOverflow: https://stackoverflow.com/q/54191190/61158

@akarnokd akarnokd added this to the 2.2 backlog milestone Jan 16, 2019
@codecov
Copy link

codecov bot commented Jan 16, 2019

Codecov Report

Merging #6371 into 2.x will increase coverage by 0.03%.
The diff coverage is 80%.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #6371      +/-   ##
============================================
+ Coverage     98.24%   98.28%   +0.03%     
- Complexity     6282     6287       +5     
============================================
  Files           673      673              
  Lines         45023    45028       +5     
  Branches       6226     6226              
============================================
+ Hits          44235    44257      +22     
+ Misses          248      243       -5     
+ Partials        540      528      -12
Impacted Files Coverage Δ Complexity Δ
...nternal/operators/observable/ObservableReplay.java 98.63% <100%> (-0.55%) 20 <0> (ø)
...ex/internal/operators/flowable/FlowableReplay.java 94.09% <66.66%> (-0.18%) 20 <0> (ø)
.../operators/flowable/FlowableBlockingSubscribe.java 93.02% <0%> (-4.66%) 10% <0%> (-1%)
...ternal/operators/observable/ObservablePublish.java 93.8% <0%> (-4.43%) 10% <0%> (-1%)
...ava/io/reactivex/processors/BehaviorProcessor.java 96.86% <0%> (-2.25%) 60% <0%> (ø)
...rnal/operators/flowable/FlowableFlatMapSingle.java 94.56% <0%> (-2.18%) 2% <0%> (ø)
...nternal/operators/observable/ObservableWindow.java 98% <0%> (-2%) 3% <0%> (ø)
...ivex/internal/operators/maybe/MaybeMergeArray.java 96.62% <0%> (-1.13%) 6% <0%> (ø)
...x/internal/operators/flowable/FlowableGroupBy.java 95.51% <0%> (-0.57%) 3% <0%> (ø)
... and 24 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 e1b3838...f7d4d02. Read the comment docs.

@artur-jablonski
Copy link

Thank you for fixing this! You guys rock!

@akarnokd akarnokd merged commit 5106a20 into ReactiveX:2.x Jan 17, 2019
@akarnokd akarnokd deleted the ReplayRetentionFix branch January 17, 2019 14:02
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