From e3795330bf84aa84a81e51cf646910acb76bb374 Mon Sep 17 00:00:00 2001 From: nmittler Date: Tue, 22 Jul 2014 08:36:17 -0700 Subject: [PATCH] Fixing bug in HTTP/2 ping handling. Motivation: The current HTTP/2 ping handling replies with an ack using the same buffer as the received ping. It doesn't call retain(), however, which causes a ReferenceCountException since the buffer ends up getting released twice (once by the write and once by the decoder). Modifications: Modified AbstractHttp2ConnectionHandler to retain() the buffer. Added a ping to Http2ConnectionRoundtripTest.stressTest() to verify the problem and that this fixes it. Result: Ping should no longer cause an exception. --- .../handler/codec/http2/AbstractHttp2ConnectionHandler.java | 3 ++- .../handler/codec/http2/Http2ConnectionRoundtripTest.java | 6 +++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/AbstractHttp2ConnectionHandler.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/AbstractHttp2ConnectionHandler.java index 43e50ceb79..92f43b5d65 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/AbstractHttp2ConnectionHandler.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/AbstractHttp2ConnectionHandler.java @@ -968,7 +968,8 @@ public abstract class AbstractHttp2ConnectionHandler extends ByteToMessageDecode verifyPrefaceReceived(); // Send an ack back to the remote client. - frameWriter.writePing(ctx, ctx.newPromise(), true, data); + // Need to retain the buffer here since it will be released after the write completes. + frameWriter.writePing(ctx, ctx.newPromise(), true, data.retain()); AbstractHttp2ConnectionHandler.this.onPingRead(ctx, data); } diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionRoundtripTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionRoundtripTest.java index 9163b735e5..7ffd9e5ef9 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionRoundtripTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionRoundtripTest.java @@ -60,7 +60,7 @@ public class Http2ConnectionRoundtripTest { private Channel serverChannel; private Channel clientChannel; private static final int NUM_STREAMS = 1000; - private final CountDownLatch requestLatch = new CountDownLatch(NUM_STREAMS * 2); + private final CountDownLatch requestLatch = new CountDownLatch(NUM_STREAMS * 3); @Before public void setup() throws Exception { @@ -111,12 +111,14 @@ public class Http2ConnectionRoundtripTest { new DefaultHttp2Headers.Builder().method("GET").scheme("https") .authority("example.org").path("/some/path/resource2").build(); final String text = "hello world"; + final String pingMsg = "12345678"; runInChannel(clientChannel, new Http2Runnable() { @Override public void run() { for (int i = 0, nextStream = 3; i < NUM_STREAMS; ++i, nextStream += 2) { http2Client.writeHeaders( ctx(), newPromise(), nextStream, headers, 0, (short) 16, false, 0, false, false); + http2Client.writePing(ctx(), newPromise(), Unpooled.copiedBuffer(pingMsg.getBytes())); http2Client.writeData( ctx(), newPromise(), nextStream, Unpooled.copiedBuffer(text.getBytes()), 0, true, true, false); @@ -128,6 +130,8 @@ public class Http2ConnectionRoundtripTest { verify(serverObserver, times(NUM_STREAMS)).onHeadersRead(any(ChannelHandlerContext.class), anyInt(), eq(headers), eq(0), eq((short) 16), eq(false), eq(0), eq(false), eq(false)); + verify(serverObserver, times(NUM_STREAMS)).onPingRead(any(ChannelHandlerContext.class), + eq(Unpooled.copiedBuffer(pingMsg.getBytes()))); verify(serverObserver, times(NUM_STREAMS)).onDataRead(any(ChannelHandlerContext.class), anyInt(), eq(Unpooled.copiedBuffer(text.getBytes())), eq(0), eq(true), eq(true)); }