Skip to content

Commit

Permalink
Fixes #902 - Expect: 100-Continue does not work with HTTP/2.
Browse files Browse the repository at this point in the history 
Improved handling of the 100 status code in both client and server.
  • Loading branch information
@sbordet
sbordet committed Sep 6, 2016
1 parent 9d72e0d commit 6d485b2
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 56 deletions.
5 changes: 4 additions & 1 deletion ...nt-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpReceiverOverHTTP2.java
View file
Expand Up @@ -30,6 +30,7 @@
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;
Expand Down Expand Up @@ -79,7 +80,9 @@ public void onHeaders(Stream stream, HeadersFrame frame)

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);
}
}
Expand Down
18 changes: 8 additions & 10 deletions ...tp2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java
View file
Expand Up @@ -21,6 +21,7 @@
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;
Expand Down Expand Up @@ -83,20 +84,17 @@ public void recycle()
@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)
{
Expand Down
2 changes: 1 addition & 1 deletion .../test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/AbstractTest.java
View file
Expand Up @@ -104,7 +104,7 @@ protected ServerConnector newServerConnector(Server server)
return new ServerConnector(server, provideServerConnectionFactory(transport));
}

private void startClient() throws Exception
protected void startClient() throws Exception
{
QueuedThreadPool clientThreads = new QueuedThreadPool();
clientThreads.setName("client");
Expand Down
74 changes: 30 additions & 44 deletions .../jetty/client/HttpClientContinueTest.java → ...y/http/client/HttpClientContinueTest.java
View file
Expand Up @@ -16,7 +16,7 @@
// ========================================================================
//

package org.eclipse.jetty.client;
package org.eclipse.jetty.http.client;

import java.io.ByteArrayOutputStream;
import java.io.IOException;
Expand All @@ -29,11 +29,13 @@
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;
Expand All @@ -47,15 +49,17 @@
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
Expand Down Expand Up @@ -83,8 +87,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
}
});

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)
Expand Down Expand Up @@ -123,8 +126,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques

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)
{
Expand Down Expand Up @@ -175,8 +177,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
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()
Expand Down Expand Up @@ -224,8 +225,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques

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())
Expand Down Expand Up @@ -273,8 +273,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques

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())
Expand Down Expand Up @@ -321,8 +320,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques

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()
Expand Down Expand Up @@ -368,8 +366,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques

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()
Expand Down Expand Up @@ -427,8 +424,7 @@ public void onFailure(Response response, Throwable failure)

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()
Expand Down Expand Up @@ -469,8 +465,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques

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()
Expand Down Expand Up @@ -518,8 +513,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques

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()
Expand Down Expand Up @@ -558,17 +552,12 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
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()
Expand Down Expand Up @@ -625,8 +614,7 @@ public void onHeaders(Response response)
});

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()
Expand All @@ -645,6 +633,8 @@ public void onComplete(Result result)
@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.
Expand All @@ -659,15 +649,11 @@ public void test_Expect100Continue_WithTwoResponsesInOneRead() throws Exception
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())
Expand Down

0 comments on commit 6d485b2

Please sign in to comment.