BoundedReplayBuffer no longer leaks memory #5282
Conversation
Codecov Report
@@ Coverage Diff @@
## 2.x #5282 +/- ##
============================================
- Coverage 96.13% 95.99% -0.14%
+ Complexity 5748 5746 -2
============================================
Files 628 628
Lines 41077 41088 +11
Branches 5699 5703 +4
============================================
- Hits 39488 39443 -45
- Misses 621 656 +35
- Partials 968 989 +21
Continue to review full report at Codecov.
|
| @@ -751,6 +757,26 @@ void truncateFinal() { | |||
| } | |||
| } | |||
| } | |||
| /* test */ final void collectObjectsInMemory(Collection<? super T> output) { | |||
There was a problem hiding this comment.
There is a collect(Collection) method just above this addition, what's wrong with that?
There was a problem hiding this comment.
collect(Collection) intentionally ignores the value in head.
| setFirst(next); | ||
| Node newHead = new Node(null); | ||
| Node newNext = next.get(); | ||
| newHead.set(newNext); |
There was a problem hiding this comment.
Wouldn't lazySet() cause bugs here?
Problem: When `BoundedReplayBuffer` evicts an item to make room for a new item it still contains a strong reference to the evicted reference. This diff adds a new unit test to verify this behavior. Given this unit test needs to look under the hood of `BoundedReplayBuffer` in order to ensure no memory leaks exist, writing this test requires a new package private method. This diff fixes the behavior.
Why? Do these have similar problems? |
I took a look at merging the `collect()` and `collectValuesInMemory()` methods into a single method at the prompting of akarnokd. This made me realize the following: 1) There are more memory leaks. I updated the setNext() method to fix some of these. 2) We can't avoid all these memory leaks without introducing schduling because of the TimedBuffer. I don't believe engineers should have any expectation that the TimedBuffer values will be eagerly cleaned up after they timeout. So I left the seperate `collect()` and `collectValuesInMemory()` methods. This allows us to test memory for memory leaks only when we think it is appropriate.
| @@ -617,23 +617,20 @@ final void removeFirst() { | |||
| // can't just move the head because it would retain the very first value | |||
| // can't null out the head's value because of late replayers would see null | |||
| setFirst(next); | |||
| } | |||
There was a problem hiding this comment.
When testing you can just call removeFirst() repeatedly. No need to introduce a test-only method. Plus, doing this reduces some memory leak false positives.
|
Updated my change so that it fixes leaks due to size truncation in both This pull request makes no attempt to fix leaks that occur due to timeouts. Fully fixing such leaks would require scheduling. Therefore, eagerly fixing these leaks is impractical. This is fine since Rx consumers won't expect timeouts to perform eager cleanup. |
|
I've thought about this and the problem with the change is that now there are two Node allocations happening per item instead of one. I would accept such change if it happened when the terminal events are received; the head no longer references +1 item for an unknown duration. Why is this extra 1 item causing you trouble?
There are a bunch of operators having the same replay like behavior implemented with the same node logic:
|
Doesn't sound like a significant improvement. I can't think of any improvement ideas for this. Can you?
While Rx is designed with immutable value objects in mind, it is useful for asynchronously emitting other types as well. For example, passing around an Optimizations like this have a large affect on application startup time. However, if something in Rx leaks
Interesting. I see this was true with Rx1 as well. So I guess this leak isn't a huge deal. It hasn't caused many issues for us in the last two years. My takeaway is: never put objects you don't want leaked into rx caches because the cache may live longer than expected. |
|
There was a previous report when the user actually wanted 1 item cached but due to the head reference, there was 2 object retained. The workaround for that case is the cacheLast extension operator.
I think caching Activities is by itself bad practice. I'm not willing to pay this extra per item allocation in the standard operator, however, the following are acceptable changes:
|
Neat. I'll take a look at that. Looks like it only supports
Okay. I'll think about this a bit. Thanks for the help and promptness! |
The Issue: When
BoundedReplayBufferevicts an item to make room for a new item it still contains a strong reference to the evicted item.New Test: This diff adds a new unit test to verify this behavior. Given this unit test needs to look under the hood of
BoundedReplayBufferin order to ensure no memory leaks exist, writing this test requires a new package private method.Change: This diff fixes misbehavior.