-
Notifications
You must be signed in to change notification settings - Fork 504
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 netty ByteBuf reference counting #1711
Conversation
Netty was reporting "LEAK: ByteBuf.release() was not called", direct memory was growing unbounded. Remove unnecessary call to retain in h2 code. Fixes #1678
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really like to understand why this retain
isn't necessary (is the frame retained somewhere else on this call to ByteBufAsBuf
?), but if we're sure that this doesn't break anything, we should definitely merge it.
A better question is "why would this retain be necessary?". We're just moving a ByteBuf we already have a handle on into a new container. Or another question: "If this retain is appropriate, then were is the corresponding release?". |
Correct me if I'm wrong, but we think this leak is likely the cause of #1690, right? Can we try to verify that this fixes that issue? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this has been thoroughly tested, I approve.
The Finagle `ByteBufAsBuf` converter wraps a Netty 4 `ByteBuf`, but does not support reference counting (see [this comment](https://github.com/twitter/finagle/blob/82d00660373582d6bfe56df8155b52528df36691/finagle-netty4/src/main/scala/com/twitter/finagle/netty4/ByteBufAsBuf.scala#L58-L61)). Therefore, when releasing a data frame whose content `ByteBuf` we `retain()` before wrapping it as a `Buf`, we must `release()` the underlying `ByteBuf`. This fixes the memory leak observed in #1678 without removing the call to `retain()` which was removed in #1711, which introduced a data corruption issue (#1728). If we `release()` the underlying `ByteBuf` after releasing the data frame, we can once again `retain()` it before converting to `Buf` without leaking, thus avoiding the data corruption caused by removing the `retain()`. Thanks to @vadimi for helping with this fix! Fixes #1728.
Netty was reporting "LEAK: ByteBuf.release() was not called", direct
memory was growing unbounded.
Remove unnecessary call to retain in h2 code.
Verified netty leak messages go away with this fix.
Fixes #1678