Skip to content

Commit ada0999

Browse files
chrisvestchrisvest
authored
Merge commit from fork
Motivation: In HTTP, the INFORMATIONAL responses are not final, and further responses are _always_ expected. Therefor, INFORMATIONAL responses must be skipped for request/response pairing. Modification: Move the request queue poll below the INFORMATIONAL check. Update tests to match and explicitly call out this behavior. Result: Fixed request/response pairing in HttpClientCodec.
1 parent b4051e2 commit ada0999

2 files changed

Lines changed: 67 additions & 13 deletions

File tree

‎codec-http/src/main/java/io/netty/handler/codec/http/HttpClientCodec.java‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -340,13 +340,6 @@ private void decrement(Object msg) {
340340

341341
@Override
342342
protected boolean isContentAlwaysEmpty(HttpMessage msg) {
343-
// Get the method of the HTTP request that corresponds to the
344-
// current response.
345-
//
346-
// Even if we do not use the method to compare we still need to poll it to ensure we keep
347-
// request / response pairs in sync.
348-
HttpMethod method = queue.poll();
349-
350343
final HttpResponseStatus status = ((HttpResponse) msg).status();
351344
final HttpStatusClass statusClass = status.codeClass();
352345
final int statusCode = status.code();
@@ -356,6 +349,10 @@ protected boolean isContentAlwaysEmpty(HttpMessage msg) {
356349
return super.isContentAlwaysEmpty(msg);
357350
}
358351

352+
// Get the method of the HTTP request that corresponds to the
353+
// current response.
354+
HttpMethod method = queue.poll();
355+
359356
// If the remote peer did for example send multiple responses for one request (which is not allowed per
360357
// spec but may still be possible) method will be null so guard against it.
361358
if (method != null) {

‎codec-http/src/test/java/io/netty/handler/codec/http/HttpClientCodecTest.java‎

Lines changed: 63 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -354,19 +354,22 @@ public void testWebDavResponse() {
354354

355355
@Test
356356
public void testInformationalResponseKeepsPairsInSync() {
357-
byte[] data = ("HTTP/1.1 102 Processing\r\n" +
357+
String data = "HTTP/1.1 102 Processing\r\n" +
358358
"Status-URI: Status-URI:http://status.com; 404\r\n" +
359-
"\r\n").getBytes();
360-
byte[] data2 = ("HTTP/1.1 200 OK\r\n" +
359+
"\r\n" +
360+
"HTTP/1.1 200 OK\r\n" +
361+
"Content-Length: 5\r\n" +
362+
"\r\n"; // No contents; we're responding to a HEAD request.
363+
String data2 = "HTTP/1.1 200 OK\r\n" +
361364
"Content-Length: 8\r\n" +
362365
"\r\n" +
363-
"12345678").getBytes();
366+
"12345678";
364367
EmbeddedChannel ch = new EmbeddedChannel(new HttpClientCodec());
365368
assertTrue(ch.writeOutbound(new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.HEAD, "/")));
366369
ByteBuf buffer = ch.readOutbound();
367370
buffer.release();
368371
assertNull(ch.readOutbound());
369-
assertTrue(ch.writeInbound(Unpooled.wrappedBuffer(data)));
372+
assertTrue(ch.writeInbound(Unpooled.wrappedBuffer(data.getBytes(CharsetUtil.ISO_8859_1))));
370373
HttpResponse res = ch.readInbound();
371374
assertSame(HttpVersion.HTTP_1_1, res.protocolVersion());
372375
assertEquals(HttpResponseStatus.PROCESSING, res.status());
@@ -375,12 +378,21 @@ public void testInformationalResponseKeepsPairsInSync() {
375378
assertEquals(0, content.content().readableBytes());
376379
assertInstanceOf(LastHttpContent.class, content);
377380
content.release();
381+
res = ch.readInbound();
382+
assertSame(HttpVersion.HTTP_1_1, res.protocolVersion());
383+
assertEquals(HttpResponseStatus.OK, res.status());
384+
// If it had not been a HEAD request, server *would* have sent 5 bytes of contents...
385+
assertEquals(5, res.headers().getInt(HttpHeaderNames.CONTENT_LENGTH));
386+
content = ch.readInbound();
387+
// ... but it is a HEAD request, so we get zero bytes.
388+
assertEquals(0, content.content().readableBytes());
389+
content.release();
378390

379391
assertTrue(ch.writeOutbound(new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/")));
380392
buffer = ch.readOutbound();
381393
buffer.release();
382394
assertNull(ch.readOutbound());
383-
assertTrue(ch.writeInbound(Unpooled.wrappedBuffer(data2)));
395+
assertTrue(ch.writeInbound(Unpooled.wrappedBuffer(data2.getBytes(CharsetUtil.ISO_8859_1))));
384396

385397
res = ch.readInbound();
386398
assertSame(HttpVersion.HTTP_1_1, res.protocolVersion());
@@ -394,6 +406,51 @@ public void testInformationalResponseKeepsPairsInSync() {
394406
assertFalse(ch.finish());
395407
}
396408

409+
@Test
410+
public void testInformationalFollowedByResponse() {
411+
EmbeddedChannel channel = new EmbeddedChannel(new HttpClientCodec());
412+
413+
assertTrue(channel.writeOutbound(new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/1")));
414+
ByteBuf request = channel.readOutbound();
415+
request.release();
416+
assertNull(channel.readOutbound());
417+
418+
assertTrue(channel.writeOutbound(new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.HEAD, "/2")));
419+
request = channel.readOutbound();
420+
request.release();
421+
assertNull(channel.readOutbound());
422+
423+
String responseStr =
424+
"HTTP/1.1 103 Early Hints\r\n\r\n" + // Early response to first GET request
425+
"HTTP/1.1 200 OK\r\nContent-Length: 5\r\n\r\nhello" + // Actual response to first GET request
426+
"HTTP/1.1 200 OK\r\n\r\n"; // Body-less response to second HEAD request
427+
assertTrue(channel.writeInbound(Unpooled.copiedBuffer(responseStr, CharsetUtil.US_ASCII)));
428+
429+
// Response 1: Early hints to first GET request.
430+
HttpResponse response = channel.readInbound();
431+
assertEquals(HttpResponseStatus.EARLY_HINTS, response.status());
432+
LastHttpContent last = channel.readInbound();
433+
assertEquals(0, last.content().readableBytes());
434+
last.release();
435+
436+
// Response 2: Actual response, with contents, to first GET request.
437+
response = channel.readInbound();
438+
assertEquals(HttpResponseStatus.OK, response.status());
439+
assertEquals(5, response.headers().getInt(HttpHeaderNames.CONTENT_LENGTH));
440+
last = channel.readInbound();
441+
assertEquals(5, last.content().readableBytes());
442+
last.release();
443+
444+
// Response 3: Actual response, with no contents, to second HEAD request.
445+
response = channel.readInbound();
446+
assertEquals(HttpResponseStatus.OK, response.status());
447+
last = channel.readInbound();
448+
assertEquals(0, last.content().readableBytes());
449+
last.release();
450+
451+
assertFalse(channel.finish());
452+
}
453+
397454
@Test
398455
public void testMultipleResponses() {
399456
String response = "HTTP/1.1 200 OK\r\n" +

0 commit comments

Comments
 (0)