-
Notifications
You must be signed in to change notification settings - Fork 505
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
finagle/netty4-7: LEAK: ByteBuf.release() was not called before it's garbage-collected #1678
Milestone
Comments
Quick update for folks watching this issue. We have reproduced a leak and are actively working on a fix. To confirm the leak you are seeing is the same one we have identified, have a look at your |
@zackangelo does your |
Quick update: we're digging into this issue this week, stay tuned. |
siggy
added a commit
that referenced
this issue
Nov 21, 2017
Netty was reporting "LEAK: ByteBuf.release() was not called", direct memory was growing unbounded. Remove unnecessary call to retain in h2 code. Fixes #1678
siggy
added a commit
that referenced
this issue
Nov 28, 2017
Netty was reporting "LEAK: ByteBuf.release() was not called", direct memory was growing unbounded. Remove unnecessary call to retain in h2 code. Fixes #1678
hawkw
added a commit
that referenced
this issue
Dec 14, 2017
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.
Tim-Brooks
pushed a commit
to Tim-Brooks/linkerd
that referenced
this issue
Dec 20, 2018
Signed-off-by: Kevin Lingerfelt <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I'm running the latest linkerd master (6148117) in Kubernetes 1.7.8, and have started seeing warnings in the linkerd logs:
In my environment, I'm running the gob app, with a sidecar h2 router configuration that looks like this:
The leaks are being reported for requests from the
web
service to thegen
service, which uses gRPC streaming. Here's the full leak report from the log:The text was updated successfully, but these errors were encountered: