From 6d485b2777b116ca4e09672ac8db8c4eadd46297 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Tue, 6 Sep 2016 12:01:24 +0200 Subject: [PATCH] Fixes #902 - Expect: 100-Continue does not work with HTTP/2. Improved handling of the 100 status code in both client and server. --- .../client/http/HttpReceiverOverHTTP2.java | 5 +- .../http2/server/HttpTransportOverHTTP2.java | 18 ++--- .../jetty/http/client/AbstractTest.java | 2 +- .../http}/client/HttpClientContinueTest.java | 74 ++++++++----------- 4 files changed, 43 insertions(+), 56 deletions(-) rename {jetty-client/src/test/java/org/eclipse/jetty => tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http}/client/HttpClientContinueTest.java (91%) diff --git a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpReceiverOverHTTP2.java b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpReceiverOverHTTP2.java index 5a3d01e867a..8420752c1ca 100644 --- a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpReceiverOverHTTP2.java +++ b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpReceiverOverHTTP2.java @@ -30,6 +30,7 @@ import org.eclipse.jetty.client.HttpReceiver; import org.eclipse.jetty.client.HttpResponse; import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpFields; +import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.MetaData; import org.eclipse.jetty.http2.ErrorCode; import org.eclipse.jetty.http2.api.Stream; @@ -79,7 +80,9 @@ public class HttpReceiverOverHTTP2 extends HttpReceiver implements Stream.Listen if (responseHeaders(exchange)) { - if (frame.isEndStream()) + int status = metaData.getStatus(); + boolean informational = HttpStatus.isInformational(status) && status != HttpStatus.SWITCHING_PROTOCOLS_101; + if (frame.isEndStream() || informational) responseSuccess(exchange); } } diff --git a/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java b/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java index 7ca4974ceb8..d8b53fa8f64 100644 --- a/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java +++ b/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java @@ -21,6 +21,7 @@ package org.eclipse.jetty.http2.server; import java.nio.ByteBuffer; import java.util.concurrent.atomic.AtomicBoolean; +import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http.MetaData; import org.eclipse.jetty.http2.ErrorCode; @@ -83,20 +84,17 @@ public class HttpTransportOverHTTP2 implements HttpTransport @Override public void send(MetaData.Response info, boolean isHeadRequest, ByteBuffer content, boolean lastContent, Callback callback) { - // info != null | content != 0 | last = true => commit + send/end - // info != null | content != 0 | last = false => commit + send - // info != null | content == 0 | last = true => commit/end - // info != null | content == 0 | last = false => commit - // info == null | content != 0 | last = true => send/end - // info == null | content != 0 | last = false => send - // info == null | content == 0 | last = true => send/end - // info == null | content == 0 | last = false => noop - boolean hasContent = BufferUtil.hasContent(content) && !isHeadRequest; if (info != null) { - if (commit.compareAndSet(false, true)) + int status = info.getStatus(); + boolean informational = HttpStatus.isInformational(status) && status != HttpStatus.SWITCHING_PROTOCOLS_101; + boolean committed = false; + if (!informational) + committed = commit.compareAndSet(false, true); + + if (committed || informational) { if (hasContent) { diff --git a/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/AbstractTest.java b/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/AbstractTest.java index 99f79cbc90d..493f801a8f5 100644 --- a/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/AbstractTest.java +++ b/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/AbstractTest.java @@ -104,7 +104,7 @@ public abstract class AbstractTest return new ServerConnector(server, provideServerConnectionFactory(transport)); } - private void startClient() throws Exception + protected void startClient() throws Exception { QueuedThreadPool clientThreads = new QueuedThreadPool(); clientThreads.setName("client"); diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientContinueTest.java b/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/HttpClientContinueTest.java similarity index 91% rename from jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientContinueTest.java rename to tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/HttpClientContinueTest.java index d654ba03e51..3961c029987 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientContinueTest.java +++ b/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/HttpClientContinueTest.java @@ -16,7 +16,7 @@ // ======================================================================== // -package org.eclipse.jetty.client; +package org.eclipse.jetty.http.client; import java.io.ByteArrayOutputStream; import java.io.IOException; @@ -29,11 +29,13 @@ import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; + import javax.servlet.ServletException; import javax.servlet.ServletInputStream; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import org.eclipse.jetty.client.ContinueProtocolHandler; import org.eclipse.jetty.client.api.ContentResponse; import org.eclipse.jetty.client.api.Response; import org.eclipse.jetty.client.api.Result; @@ -47,15 +49,17 @@ import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.handler.AbstractHandler; import org.eclipse.jetty.toolchain.test.annotation.Slow; import org.eclipse.jetty.util.IO; -import org.eclipse.jetty.util.ssl.SslContextFactory; +import org.hamcrest.Matchers; import org.junit.Assert; +import org.junit.Assume; import org.junit.Test; -public class HttpClientContinueTest extends AbstractHttpClientServerTest +public class HttpClientContinueTest extends AbstractTest { - public HttpClientContinueTest(SslContextFactory sslContextFactory) + public HttpClientContinueTest(Transport transport) { - super(sslContextFactory); + // Skip FCGI for now. + super(transport == Transport.FCGI ? null : transport); } @Test @@ -83,8 +87,7 @@ public class HttpClientContinueTest extends AbstractHttpClientServerTest } }); - ContentResponse response = client.newRequest("localhost", connector.getLocalPort()) - .scheme(scheme) + ContentResponse response = client.newRequest(newURI()) .header(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE.asString()) .content(new BytesContentProvider(contents)) .timeout(5, TimeUnit.SECONDS) @@ -123,8 +126,7 @@ public class HttpClientContinueTest extends AbstractHttpClientServerTest byte[] content1 = new byte[10240]; byte[] content2 = new byte[16384]; - ContentResponse response = client.newRequest("localhost", connector.getLocalPort()) - .scheme(scheme) + ContentResponse response = client.newRequest(newURI()) .header(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE.asString()) .content(new BytesContentProvider(content1, content2) { @@ -175,8 +177,7 @@ public class HttpClientContinueTest extends AbstractHttpClientServerTest byte[] content1 = new byte[10240]; byte[] content2 = new byte[16384]; final CountDownLatch latch = new CountDownLatch(1); - client.newRequest("localhost", connector.getLocalPort()) - .scheme(scheme) + client.newRequest(newURI()) .header(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE.asString()) .content(new BytesContentProvider(content1, content2)) .send(new BufferingResponseListener() @@ -224,8 +225,7 @@ public class HttpClientContinueTest extends AbstractHttpClientServerTest byte[] content = new byte[10240]; final CountDownLatch latch = new CountDownLatch(1); - client.newRequest("localhost", connector.getLocalPort()) - .scheme(scheme) + client.newRequest(newURI()) .method(HttpMethod.POST) .path("/continue") .header(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE.asString()) @@ -273,8 +273,7 @@ public class HttpClientContinueTest extends AbstractHttpClientServerTest byte[] content = new byte[10240]; final CountDownLatch latch = new CountDownLatch(1); - client.newRequest("localhost", connector.getLocalPort()) - .scheme(scheme) + client.newRequest(newURI()) .method(HttpMethod.POST) .path("/redirect") .header(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE.asString()) @@ -321,8 +320,7 @@ public class HttpClientContinueTest extends AbstractHttpClientServerTest byte[] content = new byte[1024]; final CountDownLatch latch = new CountDownLatch(1); - client.newRequest("localhost", connector.getLocalPort()) - .scheme(scheme) + client.newRequest(newURI()) .header(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE.asString()) .content(new BytesContentProvider(content)) .send(new BufferingResponseListener() @@ -368,8 +366,7 @@ public class HttpClientContinueTest extends AbstractHttpClientServerTest byte[] content = new byte[1024]; final CountDownLatch latch = new CountDownLatch(1); - client.newRequest("localhost", connector.getLocalPort()) - .scheme(scheme) + client.newRequest(newURI()) .header(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE.asString()) .content(new BytesContentProvider(content)) .send(new BufferingResponseListener() @@ -427,8 +424,7 @@ public class HttpClientContinueTest extends AbstractHttpClientServerTest byte[] content = new byte[1024]; final CountDownLatch latch = new CountDownLatch(1); - client.newRequest("localhost", connector.getLocalPort()) - .scheme(scheme) + client.newRequest(newURI()) .header(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE.asString()) .content(new BytesContentProvider(content)) .send(new BufferingResponseListener() @@ -469,8 +465,7 @@ public class HttpClientContinueTest extends AbstractHttpClientServerTest final CountDownLatch latch = new CountDownLatch(1); DeferredContentProvider content = new DeferredContentProvider(); - client.newRequest("localhost", connector.getLocalPort()) - .scheme(scheme) + client.newRequest(newURI()) .header(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE.asString()) .content(content) .send(new BufferingResponseListener() @@ -518,8 +513,7 @@ public class HttpClientContinueTest extends AbstractHttpClientServerTest final CountDownLatch latch = new CountDownLatch(1); DeferredContentProvider content = new DeferredContentProvider(ByteBuffer.wrap(chunk1)); - client.newRequest("localhost", connector.getLocalPort()) - .scheme(scheme) + client.newRequest(newURI()) .header(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE.asString()) .content(content) .send(new BufferingResponseListener() @@ -558,17 +552,12 @@ public class HttpClientContinueTest extends AbstractHttpClientServerTest final DeferredContentProvider content = new DeferredContentProvider(); final CountDownLatch latch = new CountDownLatch(1); - client.newRequest("localhost", connector.getLocalPort()) - .scheme(scheme) + client.newRequest(newURI()) .header(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE.asString()) - .onRequestHeaders(new org.eclipse.jetty.client.api.Request.HeadersListener() + .onRequestHeaders(request -> { - @Override - public void onHeaders(org.eclipse.jetty.client.api.Request request) - { - content.offer(ByteBuffer.wrap(data)); - content.close(); - } + content.offer(ByteBuffer.wrap(data)); + content.close(); }) .content(content) .send(new BufferingResponseListener() @@ -625,8 +614,7 @@ public class HttpClientContinueTest extends AbstractHttpClientServerTest }); final CountDownLatch latch = new CountDownLatch(1); - client.newRequest("localhost", connector.getLocalPort()) - .scheme(scheme) + client.newRequest(newURI()) .header(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE.asString()) .content(content) .send(new BufferingResponseListener() @@ -645,6 +633,8 @@ public class HttpClientContinueTest extends AbstractHttpClientServerTest @Test public void test_Expect100Continue_WithTwoResponsesInOneRead() throws Exception { + Assume.assumeThat(transport, Matchers.isOneOf(Transport.HTTP, Transport.HTTPS)); + // There is a chance that the server replies with the 100 Continue response // and immediately after with the "normal" response, say a 200 OK. // These may be read by the client in a single read, and must be handled correctly. @@ -659,15 +649,11 @@ public class HttpClientContinueTest extends AbstractHttpClientServerTest client.newRequest("localhost", server.getLocalPort()) .header(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE.asString()) .content(new BytesContentProvider(new byte[]{0})) - .send(new Response.CompleteListener() + .send(result -> { - @Override - public void onComplete(Result result) - { - Assert.assertTrue(result.toString(), result.isSucceeded()); - Assert.assertEquals(200, result.getResponse().getStatus()); - latch.countDown(); - } + Assert.assertTrue(result.toString(), result.isSucceeded()); + Assert.assertEquals(200, result.getResponse().getStatus()); + latch.countDown(); }); try (Socket socket = server.accept())