Skip to content

Commit

Permalink
Data frames release the underlying ByteBuf, not the wrapped Buf (link…
Browse files Browse the repository at this point in the history
…erd#1751)

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 linkerd#1678 without removing the call to `retain()` which was removed in linkerd#1711, which introduced a data corruption issue (linkerd#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 linkerd#1728.
  • Loading branch information
hawkw authored Dec 14, 2017
1 parent 2092665 commit 544060c
Showing 1 changed file with 12 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,19 @@ private[h2] object Netty4Message {

def apply(f: Http2DataFrame, updateWindow: Int => Future[Unit]): Frame.Data = {
val sz = f.content.readableBytes + f.padding
val buf = ByteBufAsBuf(f.content)
val buf = ByteBufAsBuf(f.content.retain())
val releaser: () => Future[Unit] =
if (sz > 0) () => updateWindow(sz)
else () => Future.Unit
() =>
{
// When releasing the frame, call `updateWindow` with the frame's
// size to release the frame's window capacity.
val res = if (sz > 0) updateWindow(sz) else Future.Unit
// Because `ByteBufAsBuf` does not support reference counting,
// it's necessary to call `.release()` on the underlying `ByteBuf`
// when releasing the data frame.
f.content.release()
res
}
Frame.Data(buf, f.isEndStream, releaser)
}
}
Expand Down

0 comments on commit 544060c

Please sign in to comment.