HttpClientCodec need to keep request / response pairs in sync all the… (#9721)
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
This commit is contained in:
parent
8e57c00e1c
commit
6667b590c4
|
@ -222,6 +222,13 @@ public final class HttpClientCodec extends CombinedChannelDuplexHandler<HttpResp
|
||||||
|
|
||||||
@Override
|
@Override
|
||||||
protected boolean isContentAlwaysEmpty(HttpMessage msg) {
|
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();
|
final int statusCode = ((HttpResponse) msg).status().code();
|
||||||
if (statusCode >= 100 && statusCode < 200) {
|
if (statusCode >= 100 && statusCode < 200) {
|
||||||
// An informational response should be excluded from paired comparison.
|
// An informational response should be excluded from paired comparison.
|
||||||
|
@ -229,10 +236,6 @@ public final class HttpClientCodec extends CombinedChannelDuplexHandler<HttpResp
|
||||||
return super.isContentAlwaysEmpty(msg);
|
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
|
// 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.
|
// spec but may still be possible) method will be null so guard against it.
|
||||||
if (method != null) {
|
if (method != null) {
|
||||||
|
|
|
@ -36,6 +36,7 @@ import io.netty.handler.codec.CodecException;
|
||||||
import io.netty.handler.codec.PrematureChannelClosureException;
|
import io.netty.handler.codec.PrematureChannelClosureException;
|
||||||
import io.netty.util.CharsetUtil;
|
import io.netty.util.CharsetUtil;
|
||||||
import io.netty.util.NetUtil;
|
import io.netty.util.NetUtil;
|
||||||
|
import org.hamcrest.CoreMatchers;
|
||||||
import org.junit.Test;
|
import org.junit.Test;
|
||||||
|
|
||||||
import java.net.InetSocketAddress;
|
import java.net.InetSocketAddress;
|
||||||
|
@ -347,6 +348,48 @@ public class HttpClientCodecTest {
|
||||||
assertThat(ch.finish(), is(false));
|
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
|
@Test
|
||||||
public void testMultipleResponses() {
|
public void testMultipleResponses() {
|
||||||
String response = "HTTP/1.1 200 OK\r\n" +
|
String response = "HTTP/1.1 200 OK\r\n" +
|
||||||
|
|
Loading…
Reference in New Issue
Block a user