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

Fix direct memory leak #421

Merged
merged 7 commits into from
May 3, 2023

Conversation

hamadodene
Copy link
Contributor

@hamadodene hamadodene commented Apr 27, 2023

In the current implementation we use PooledByteBufAllocator. PooledByteBufAllocator allows you to have a ByteBuf cache for each thread. So if release is called on a ByteBuf, this is put back in the cache so that it can be reused without having to allocate a new one.

Since it is for Threads, if there are threads that are no longer used and new ones are created this inevitably leads to an OOM. Because unused threads will continue to maintain their ByteBuf cache. We've tried enabling the -Dio.netty.allocator.cacheTrimIntervalMillis option but it doesn't seem to help in any way.

In the way we use the caffeine cache it is convenient to switch to UnpolledByteBuf only for the carapace cache.

The fix consists of:

  • Create a UnPooledByteBufAllocator used only for data that needs to be cached
  • Each time a chunk is received, the chunk data is copied to a new ByteBuf allocated in UnpooledByteBufAllocator.
  • When an entry in the cache is invalidated, the reference Bytebuf is also released so that it can be cleaned by GC.
  • Release the original chunk so that the memory portion can be cleaned by the GC.
  • Introduce a sys prop cache.allocator.usepooledbytebufallocator=true/false (default false - Unpooled) to use pooled version

To fetch an object from the cache, the logic doesn't change.
It's called retainduplicate() of the chunk.
No need to call release dul duplicate as it will be released already by reactor netty.

Note: reactor netty HttpClient and HttpServer continue use PooledByteBufAllocator. This fix is only to use UnpooledByteBufAllocator for cachable data in caffeine cache.

Copy link
Contributor

@pv3ntur1 pv3ntur1 left a comment

Choose a reason for hiding this comment

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

Just a comment

@hamadodene hamadodene requested a review from pv3ntur1 April 27, 2023 08:09
@hamadodene hamadodene force-pushed the fix/direct-memory-leak branch from d0f699f to e0bb575 Compare April 27, 2023 13:20
@hamadodene hamadodene force-pushed the fix/direct-memory-leak branch 2 times, most recently from fca8d7b to 576acd8 Compare April 28, 2023 11:20
@hamadodene hamadodene force-pushed the fix/direct-memory-leak branch 3 times, most recently from 315bfcc to 4baae49 Compare April 28, 2023 15:15
@hamadodene hamadodene force-pushed the fix/direct-memory-leak branch from 4baae49 to b677aab Compare April 28, 2023 15:41
@hamadodene hamadodene force-pushed the fix/direct-memory-leak branch from 5d28108 to defa7ff Compare April 30, 2023 06:07
@hamadodene hamadodene force-pushed the fix/direct-memory-leak branch from defa7ff to 2b490f7 Compare April 30, 2023 06:07
@hamadodene
Copy link
Contributor Author

hamadodene commented Apr 30, 2023

@pv3nturi Upgrade reactor netty for potential memory leak in netty 4.1.81.Final polled allocator #424

@hamadodene hamadodene force-pushed the fix/direct-memory-leak branch 7 times, most recently from d7d2bcb to ab0694c Compare May 2, 2023 12:18
@hamadodene hamadodene marked this pull request as ready for review May 2, 2023 12:45
Copy link
Contributor

@pv3ntur1 pv3ntur1 left a comment

Choose a reason for hiding this comment

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

LGTM

@pv3ntur1 pv3ntur1 requested a review from dmercuriali May 3, 2023 12:47
@hamadodene hamadodene force-pushed the fix/direct-memory-leak branch from ab0694c to 6499066 Compare May 3, 2023 13:28
@dmercuriali dmercuriali merged commit 868d50f into diennea:master May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants