Skip to content

Commit

Permalink
fix memory leak on failure to decode header
Browse files Browse the repository at this point in the history
Summary:
Since December 16th we've been noticing a gradual increase in memory usage on one of our services https://fburl.com/ods/rkhrh9ri.

Following investigation the root cause appears to be related to an exception coming from decoding message headers which for some reason are missing the expected magic [bytes](https://fburl.com/code/pg51fcpv) (to follow up with the thrift team on root cause) - [logs](https://fburl.com/logarithm/ikbyqt4t). The message buffers are not freed when this exception or any others occur whilst decoding which results in memory leaks.

The fix is to release resources when thrift header decoding fails. Netty [documentation](https://netty.io/wiki/reference-counted-objects.html#who-destroys-it) suggests a rule of thumb that whoever accesses a refcounted object last is responsible for freeing it. If there are exceptions in HeaderTransportCodec before the message is passed down it's now freed. If there are exceptions down the callstack it's assumed that the last consumer where the exception was raised is responsible for freeing as is done by the [thrift client handler](https://fburl.com/code/dvzn65f7).

Reviewed By: avalonalex

Differential Revision: D52330700

fbshipit-source-id: b8b115c5e065a674927747bf4dfe924a4e4a4dde
  • Loading branch information
Rikin Shah authored and facebook-github-bot committed Dec 29, 2023
1 parent 13464b1 commit 628d648
Showing 1 changed file with 14 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import io.netty.channel.ChannelDuplexHandler;
import io.netty.channel.ChannelHandlerContext;
import io.netty.channel.ChannelPromise;
import io.netty.util.ReferenceCountUtil;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
Expand Down Expand Up @@ -74,11 +75,22 @@ public HeaderTransportCodec(boolean isServer) {
public void channelRead(ChannelHandlerContext context, Object message) {
if (message instanceof ByteBuf) {
ByteBuf request = (ByteBuf) message;
if (request.isReadable()) {
context.fireChannelRead(decodeFrame(context.alloc(), request));
ThriftFrame frame = null;
try {
if (request.isReadable()) {
frame = decodeFrame(context.alloc(), request);
}
} catch (Throwable t) {
ReferenceCountUtil.safeRelease(request);
context.fireExceptionCaught(t);
return;
}
if (frame != null) {
context.fireChannelRead(frame);
return;
}
}

context.fireChannelRead(message);
}

Expand Down

0 comments on commit 628d648

Please sign in to comment.