From bbc34d0eda38efb4a6364561e9ac1d2319682694 Mon Sep 17 00:00:00 2001
From: Matthew Miller <millem@amazon.com>
Date: Wed, 16 Oct 2019 19:32:45 -0700
Subject: [PATCH] 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.
---
 .../handler/codec/http2/Http2FrameCodec.java  | 29 +++++++++++++++----
 .../codec/http2/Http2FrameCodecTest.java      | 27 +++++++++++++++++
 2 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodec.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodec.java
index 548ef9fc50..6a2f37832c 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodec.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodec.java
@@ -207,6 +207,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
     public final void handlerAdded(ChannelHandlerContext ctx) throws Exception {
         this.ctx = ctx;
@@ -411,8 +420,19 @@ public class Http2FrameCodec extends Http2ConnectionHandler {
             // We should not re-use ids.
             assert old == null;
 
+            // Clean up the stream being initialized if writing the headers fails.
+            promise.addListener(new ChannelFutureListener() {
+                @Override
+                public void operationComplete(ChannelFuture channelFuture) {
+                    if (!channelFuture.isSuccess()) {
+                        frameStreamToInitializeMap.remove(streamId);
+                    }
+                }
+            });
+
             encoder().writeHeaders(ctx, streamId, headersFrame.headers(), headersFrame.padding(),
                     headersFrame.isEndStream(), promise);
+
             if (!promise.isDone()) {
                 numBufferedStreams++;
                 promise.addListener(bufferedStreamsListener);
@@ -431,15 +451,14 @@ public class Http2FrameCodec extends Http2ConnectionHandler {
     }
 
     private final class ConnectionListener extends Http2ConnectionAdapter {
-
         @Override
         public void onStreamAdded(Http2Stream stream) {
             DefaultHttp2FrameStream frameStream = frameStreamToInitializeMap.remove(stream.id());
 
-             if (frameStream != null) {
-                 frameStream.setStreamAndProperty(streamKey, stream);
-             }
-         }
+            if (frameStream != null) {
+                frameStream.setStreamAndProperty(streamKey, stream);
+            }
+        }
 
         @Override
         public void onStreamActive(Http2Stream stream) {
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameCodecTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameCodecTest.java
index 01a6032404..0d1fb5b416 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameCodecTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameCodecTest.java
@@ -665,6 +665,33 @@ public class Http2FrameCodecTest {
         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
     public void streamIdentifiersExhausted() throws Http2Exception {
         int maxServerStreamId = Integer.MAX_VALUE - 1;