From 6667b590c4fceba3f9712c564f97c9952ec332f6 Mon Sep 17 00:00:00 2001
From: Norman Maurer <norman_maurer@apple.com>
Date: Tue, 29 Oct 2019 19:48:43 +0100
Subject: [PATCH] =?UTF-8?q?HttpClientCodec=20need=20to=20keep=20request=20?=
 =?UTF-8?q?/=20response=20pairs=20in=20sync=20all=20the=E2=80=A6=20(#9721)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Motivation:

At the moment we miss to poll the method queue when we see an Informational response code. This can lead to out-of-sync of request / response pairs when later try to compare these.

Modifications:

Always poll the queue correctly

Result:

Always compare the correct request / response pairs
---
 .../handler/codec/http/HttpClientCodec.java   | 11 +++--
 .../codec/http/HttpClientCodecTest.java       | 43 +++++++++++++++++++
 2 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientCodec.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientCodec.java
index fb0e437d4b..990faf78da 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientCodec.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientCodec.java
@@ -222,6 +222,13 @@ public final class HttpClientCodec extends CombinedChannelDuplexHandler<HttpResp
 
         @Override
         protected boolean isContentAlwaysEmpty(HttpMessage msg) {
+            // Get the method of the HTTP request that corresponds to the
+            // current response.
+            //
+            // Even if we do not use the method to compare we still need to poll it to ensure we keep
+            // request / response pairs in sync.
+            HttpMethod method = queue.poll();
+
             final int statusCode = ((HttpResponse) msg).status().code();
             if (statusCode >= 100 && statusCode < 200) {
                 // An informational response should be excluded from paired comparison.
@@ -229,10 +236,6 @@ public final class HttpClientCodec extends CombinedChannelDuplexHandler<HttpResp
                 return super.isContentAlwaysEmpty(msg);
             }
 
-            // Get the getMethod of the HTTP request that corresponds to the
-            // current response.
-            HttpMethod method = queue.poll();
-
             // If the remote peer did for example send multiple responses for one request (which is not allowed per
             // spec but may still be possible) method will be null so guard against it.
             if (method != null) {
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpClientCodecTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpClientCodecTest.java
index fd04d8bb4a..3470161a1e 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpClientCodecTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpClientCodecTest.java
@@ -36,6 +36,7 @@ import io.netty.handler.codec.CodecException;
 import io.netty.handler.codec.PrematureChannelClosureException;
 import io.netty.util.CharsetUtil;
 import io.netty.util.NetUtil;
+import org.hamcrest.CoreMatchers;
 import org.junit.Test;
 
 import java.net.InetSocketAddress;
@@ -347,6 +348,48 @@ public class HttpClientCodecTest {
         assertThat(ch.finish(), is(false));
     }
 
+    @Test
+    public void testInformationalResponseKeepsPairsInSync() {
+        byte[] data = ("HTTP/1.1 102 Processing\r\n" +
+                "Status-URI: Status-URI:http://status.com; 404\r\n" +
+                "\r\n").getBytes();
+        byte[] data2 = ("HTTP/1.1 200 OK\r\n" +
+                "Content-Length: 8\r\n" +
+                "\r\n" +
+                "12345678").getBytes();
+        EmbeddedChannel ch = new EmbeddedChannel(new HttpClientCodec());
+        assertTrue(ch.writeOutbound(new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.HEAD, "/")));
+        ByteBuf buffer = ch.readOutbound();
+        buffer.release();
+        assertNull(ch.readOutbound());
+        assertTrue(ch.writeInbound(Unpooled.wrappedBuffer(data)));
+        HttpResponse res = ch.readInbound();
+        assertThat(res.protocolVersion(), sameInstance(HttpVersion.HTTP_1_1));
+        assertThat(res.status(), is(HttpResponseStatus.PROCESSING));
+        HttpContent content = ch.readInbound();
+        // HTTP 102 is not allowed to have content.
+        assertThat(content.content().readableBytes(), is(0));
+        assertThat(content, CoreMatchers.<HttpContent>instanceOf(LastHttpContent.class));
+        content.release();
+
+        assertTrue(ch.writeOutbound(new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/")));
+        buffer = ch.readOutbound();
+        buffer.release();
+        assertNull(ch.readOutbound());
+        assertTrue(ch.writeInbound(Unpooled.wrappedBuffer(data2)));
+
+        res = ch.readInbound();
+        assertThat(res.protocolVersion(), sameInstance(HttpVersion.HTTP_1_1));
+        assertThat(res.status(), is(HttpResponseStatus.OK));
+        content = ch.readInbound();
+        // HTTP 200 has content.
+        assertThat(content.content().readableBytes(), is(8));
+        assertThat(content, CoreMatchers.<HttpContent>instanceOf(LastHttpContent.class));
+        content.release();
+
+        assertThat(ch.finish(), is(false));
+    }
+
     @Test
     public void testMultipleResponses() {
         String response = "HTTP/1.1 200 OK\r\n" +