[fix][ml] Fix memory leak due to duplicated RangeCache value retain operations

[BewareMyPower]

Motivation

#23903 introduces a memory leak issue in RangeCache#removeEntry.

-        Value value = entryWrapper.getValue(key);
+        Value value = getValueMatchingEntry(entry);

Unlike entryWrapper.getValue, getValueMatchingEntry will increase the reference count of entry's value.

Modifications

• Remove the duplicated retain operation in RangeCache#removeEntry. Add some API notes for the private methods that might increase the reference count of the value
• Apply the reference count validation for eviction on a RangeCache (RangeCacheTest.customTimeExtraction).

Documentation

• doc
• doc-required
• doc-not-needed
• doc-complete

Matching PR in forked repository

PR in forked repository:

[BewareMyPower]

Though I think catching the IllegalReferenceCountException from value.retain() is not a good practice and error prone, let's just keep the fix simple.

[lhotari]

Thanks for finding the problem area. However, this isn't currently the correct fix to the problem. Explained in #23955 (comment) . There's also comments in the original code on line 389 and 430 where the extra reference is removed.

[lhotari]

Oh sorry, reviewed again and noticed that it's fine.

[lhotari]

The removeEntry logic could be simplified further. The logic in this method should have been improved while adding the EntryWrapper in #22814. With EntryWrapper, it's possible to remove the exact entry from the map, which wasn't possible before adding EntryWrapper.

[lhotari]

Though I think catching the IllegalReferenceCountException from value.retain() is not a good practice and error prone, let's just keep the fix simple.

This is necessary in this case due to the racy usage of the cache without synchronization.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 80.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 74.23%. Comparing base (bbc6224) to head (9a9370a).
Report is 894 commits behind head on master.

Files with missing lines	Patch %	Lines
...org/apache/bookkeeper/mledger/util/RangeCache.java	80.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #23955      +/-   ##
============================================
+ Coverage     73.57%   74.23%   +0.66%     
+ Complexity    32624    31919     -705     
============================================
  Files          1877     1853      -24     
  Lines        139502   143828    +4326     
  Branches      15299    16344    +1045     
============================================
+ Hits         102638   106773    +4135     
+ Misses        28908    28674     -234     
- Partials       7956     8381     +425     

Flag	Coverage Δ
inttests	26.67% <53.33%> (+2.08%)	⬆️
systests	23.25% <53.33%> (-1.08%)	⬇️
unittests	73.77% <80.00%> (+0.93%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...org/apache/bookkeeper/mledger/util/RangeCache.java	80.79% <80.00%> (-14.62%)	⬇️

... and 1041 files with indirect coverage changes

[lhotari]

I'll merge this as soon as it completes, since I'm creating a PR to prevent future regressions by enabling Netty Leak detector in Pulsar CI so that any detected leak will make the build fail.

[lhotari]

I have created #23956 (WIP, draft) to address future memory leak regressions. It seems that there are quite a few tests where leaks happen currently and some of them are hard to fix. I have added a solution where it's possible to ignore specific test classes.