HTTP/2: Prevent memory leak when trying to create new streams on a connection that received a GOAWAY. (#9674)
Motivation: In https://github.com/netty/netty/issues/8692, `Http2FrameCodec` was updated to keep track of all "being initialized" streams, allocating memory before initialization begins, and releasing memory after initialization completes successfully. In some instances where stream initialization fails (e.g. because this connection has received a GOAWAY frame), this memory is never released. Modifications: This change updates the `Http2FrameCodec` to use a separate promise for monitoring the success of sending HTTP2 headers. When sending of headers fails, we now make sure to release memory allocated for stream initialization. Result: After this change, failures in writing HTTP2 Headers (e.g. because this connection has received a GOAWAY frame) will no longer leak memory.
This commit is contained in:
parent
c462ec960b
commit
d36a2a65e1
@ -199,6 +199,15 @@ public class Http2FrameCodec extends Http2ConnectionHandler {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Retrieve the number of streams currently in the process of being initialized.
|
||||||
|
*
|
||||||
|
* This is package-private for testing only.
|
||||||
|
*/
|
||||||
|
int numInitializingStreams() {
|
||||||
|
return frameStreamToInitializeMap.size();
|
||||||
|
}
|
||||||
|
|
||||||
@Override
|
@Override
|
||||||
public final void handlerAdded(ChannelHandlerContext ctx) throws Exception {
|
public final void handlerAdded(ChannelHandlerContext ctx) throws Exception {
|
||||||
this.ctx = ctx;
|
this.ctx = ctx;
|
||||||
@ -403,8 +412,16 @@ public class Http2FrameCodec extends Http2ConnectionHandler {
|
|||||||
// We should not re-use ids.
|
// We should not re-use ids.
|
||||||
assert old == null;
|
assert old == null;
|
||||||
|
|
||||||
|
// Clean up the stream being initialized if writing the headers fails.
|
||||||
|
promise.addListener(channelFuture -> {
|
||||||
|
if (!channelFuture.isSuccess()) {
|
||||||
|
frameStreamToInitializeMap.remove(streamId);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
encoder().writeHeaders(ctx, streamId, headersFrame.headers(), headersFrame.padding(),
|
encoder().writeHeaders(ctx, streamId, headersFrame.headers(), headersFrame.padding(),
|
||||||
headersFrame.isEndStream(), promise);
|
headersFrame.isEndStream(), promise);
|
||||||
|
|
||||||
if (!promise.isDone()) {
|
if (!promise.isDone()) {
|
||||||
numBufferedStreams++;
|
numBufferedStreams++;
|
||||||
promise.addListener(bufferedStreamsListener);
|
promise.addListener(bufferedStreamsListener);
|
||||||
@ -423,15 +440,14 @@ public class Http2FrameCodec extends Http2ConnectionHandler {
|
|||||||
}
|
}
|
||||||
|
|
||||||
private final class ConnectionListener extends Http2ConnectionAdapter {
|
private final class ConnectionListener extends Http2ConnectionAdapter {
|
||||||
|
|
||||||
@Override
|
@Override
|
||||||
public void onStreamAdded(Http2Stream stream) {
|
public void onStreamAdded(Http2Stream stream) {
|
||||||
DefaultHttp2FrameStream frameStream = frameStreamToInitializeMap.remove(stream.id());
|
DefaultHttp2FrameStream frameStream = frameStreamToInitializeMap.remove(stream.id());
|
||||||
|
|
||||||
if (frameStream != null) {
|
if (frameStream != null) {
|
||||||
frameStream.setStreamAndProperty(streamKey, stream);
|
frameStream.setStreamAndProperty(streamKey, stream);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@Override
|
@Override
|
||||||
public void onStreamActive(Http2Stream stream) {
|
public void onStreamActive(Http2Stream stream) {
|
||||||
|
@ -689,6 +689,33 @@ public class Http2FrameCodecTest {
|
|||||||
assertFalse(channel.finishAndReleaseAll());
|
assertFalse(channel.finishAndReleaseAll());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
public void doNotLeakOnFailedInitializationForChannels() throws Exception {
|
||||||
|
setUp(Http2FrameCodecBuilder.forServer(), new Http2Settings().maxConcurrentStreams(2));
|
||||||
|
|
||||||
|
Http2FrameStream stream1 = frameCodec.newStream();
|
||||||
|
Http2FrameStream stream2 = frameCodec.newStream();
|
||||||
|
|
||||||
|
ChannelPromise stream1HeaderPromise = channel.newPromise();
|
||||||
|
ChannelPromise stream2HeaderPromise = channel.newPromise();
|
||||||
|
|
||||||
|
channel.writeAndFlush(new DefaultHttp2HeadersFrame(new DefaultHttp2Headers()).stream(stream1),
|
||||||
|
stream1HeaderPromise);
|
||||||
|
channel.runPendingTasks();
|
||||||
|
|
||||||
|
frameInboundWriter.writeInboundGoAway(stream1.id(), 0L, Unpooled.EMPTY_BUFFER);
|
||||||
|
|
||||||
|
channel.writeAndFlush(new DefaultHttp2HeadersFrame(new DefaultHttp2Headers()).stream(stream2),
|
||||||
|
stream2HeaderPromise);
|
||||||
|
channel.runPendingTasks();
|
||||||
|
|
||||||
|
assertTrue(stream1HeaderPromise.syncUninterruptibly().isSuccess());
|
||||||
|
assertTrue(stream2HeaderPromise.isDone());
|
||||||
|
|
||||||
|
assertEquals(0, frameCodec.numInitializingStreams());
|
||||||
|
assertFalse(channel.finishAndReleaseAll());
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
public void streamIdentifiersExhausted() throws Http2Exception {
|
public void streamIdentifiersExhausted() throws Http2Exception {
|
||||||
int maxServerStreamId = Integer.MAX_VALUE - 1;
|
int maxServerStreamId = Integer.MAX_VALUE - 1;
|
||||||
|
Loading…
Reference in New Issue
Block a user