From 41532edf0993e1c5933dc3856912c5b7b2298b90 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Fri, 27 May 2022 09:48:48 +1000 Subject: [PATCH] Implement #8057 103 Early Hint updates from review --- .../org/eclipse/jetty/http/HttpGenerator.java | 2 +- .../http2/server/HttpTransportOverHTTP2.java | 2 +- .../internal/HttpTransportOverHTTP3.java | 2 +- .../org/eclipse/jetty/server/HttpChannel.java | 2 +- .../org/eclipse/jetty/server/Response.java | 15 ++-- ...st.java => InformationalResponseTest.java} | 77 ++----------------- 6 files changed, 17 insertions(+), 83 deletions(-) rename tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/{IntermediateResponseTest.java => InformationalResponseTest.java} (75%) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java index f3676ca7e1db..16f608357f17 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java @@ -385,7 +385,7 @@ public Result generateResponse(MetaData.Response info, boolean head, ByteBuffer // Handle 1xx and no content responses int status = info.getStatus(); - if (status >= 100 && status < 200) + if (HttpStatus.isInformational(status)) { _noContentResponse = true; switch (status) 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 bda79fc8e33c..f62eb6ed280f 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 @@ -98,7 +98,7 @@ public void sendHeaders(MetaData.Request request, MetaData.Response response, By boolean isHeadRequest = HttpMethod.HEAD.is(request.getMethod()); boolean hasContent = BufferUtil.hasContent(content) && !isHeadRequest; int status = response.getStatus(); - boolean interimResponse = status >= HttpStatus.CONTINUE_100 && status < HttpStatus.OK_200 && status != HttpStatus.SWITCHING_PROTOCOLS_101; + boolean interimResponse = HttpStatus.isInformational(status) && status != HttpStatus.SWITCHING_PROTOCOLS_101; if (interimResponse) { // Must not commit interim responses. diff --git a/jetty-http3/http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpTransportOverHTTP3.java b/jetty-http3/http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpTransportOverHTTP3.java index 80c50cadd1ba..bb01b68e7e49 100644 --- a/jetty-http3/http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpTransportOverHTTP3.java +++ b/jetty-http3/http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpTransportOverHTTP3.java @@ -71,7 +71,7 @@ private void sendHeaders(MetaData.Request request, MetaData.Response response, B boolean isHeadRequest = HttpMethod.HEAD.is(request.getMethod()); boolean hasContent = BufferUtil.hasContent(content) && !isHeadRequest; int status = response.getStatus(); - boolean interimResponse = status >= HttpStatus.CONTINUE_100 && status < HttpStatus.OK_200 && status != HttpStatus.SWITCHING_PROTOCOLS_101; + boolean interimResponse = HttpStatus.isInformational(status) && status != HttpStatus.SWITCHING_PROTOCOLS_101; if (interimResponse) { // Must not commit interim responses. diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java index 6e13f885faf1..f8a4b1cf4676 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java @@ -1046,7 +1046,7 @@ protected boolean sendResponse(MetaData.Response response, ByteBuffer content, b // wrap callback to process 100 responses final int status = response.getStatus(); - final Callback committed = (status < HttpStatus.OK_200 && status >= HttpStatus.CONTINUE_100) + final Callback committed = HttpStatus.isInformational(status) ? new Send100Callback(callback) : new SendCallback(callback, content, true, complete); diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java index 21748ad6f5c1..620b319ef28f 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java @@ -470,6 +470,7 @@ public void sendError(int sc) throws IOException *

In addition to the servlet standard handling, this method supports some additional codes:

*
*
102
Send a partial PROCESSING response and allow additional responses
+ *
103
Send a partial EARLY_HINT response as per RFC8297
*
-1
Abort the HttpChannel and close the connection/stream
*
* @param code The error code @@ -501,9 +502,8 @@ public void sendError(int code, String message) throws IOException /** * Sends a 102-Processing response. - * If the connection is an HTTP connection, the version is 1.1 and the - * request has a Expect header starting with 102, then a 102 response is - * sent. This indicates that the request still be processed and real response + * If the request had an Expect header starting with 102, then + * a 102 response is sent. This indicates that the request still be processed and real response * can still be sent. This method is called by sendError if it is passed 102. * * @throws IOException if unable to send the 102 response @@ -518,11 +518,10 @@ public void sendProcessing() throws IOException } /** - * Sends a 102-Processing response. - * If the connection is an HTTP connection, the version is 1.1 and the - * request has a Expect header starting with 102, then a 102 response is - * sent. This indicates that the request still be processed and real response - * can still be sent. This method is called by sendError if it is passed 102. + * Sends a 103 Early Hint response. + * + * Send a 103 response as per RFC8297 + * This method is called by sendError if it is passed 103. * * @throws IOException if unable to send the 102 response * @see javax.servlet.http.HttpServletResponse#sendError(int) diff --git a/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/IntermediateResponseTest.java b/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/InformationalResponseTest.java similarity index 75% rename from tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/IntermediateResponseTest.java rename to tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/InformationalResponseTest.java index e6e597ec586d..55879e408dae 100644 --- a/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/IntermediateResponseTest.java +++ b/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/InformationalResponseTest.java @@ -23,23 +23,18 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; -import org.eclipse.jetty.client.ContinueProtocolHandler; import org.eclipse.jetty.client.HttpConversation; import org.eclipse.jetty.client.HttpExchange; import org.eclipse.jetty.client.HttpRequest; import org.eclipse.jetty.client.ProtocolHandler; import org.eclipse.jetty.client.api.Response; import org.eclipse.jetty.client.api.Result; -import org.eclipse.jetty.client.util.AsyncRequestContent; import org.eclipse.jetty.client.util.BufferingResponseListener; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpHeaderValue; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.handler.AbstractHandler; -import org.eclipse.jetty.util.BufferUtil; -import org.eclipse.jetty.util.Callback; -import org.eclipse.jetty.util.IO; import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ArgumentsSource; @@ -50,7 +45,7 @@ import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertTrue; -public class IntermediateResponseTest extends AbstractTest +public class InformationalResponseTest extends AbstractTest { @Override public void init(Transport transport) throws IOException @@ -60,65 +55,6 @@ public void init(Transport transport) throws IOException setScenario(new TransportScenario(transport)); } - @ParameterizedTest - @ArgumentsSource(TransportProvider.class) - public void test100Continue(Transport transport) throws Exception - { - init(transport); - scenario.start(new AbstractHandler() - { - @Override - public void handle(String target, Request jettyRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException - { - jettyRequest.setHandled(true); - String body = IO.toString(request.getInputStream()); - response.getOutputStream().print("read " + body.length()); - } - }); - long idleTimeout = 10000; - scenario.setRequestIdleTimeout(idleTimeout); - - AsyncRequestContent content = new AsyncRequestContent() - { - @Override - public void demand() - { - super.demand(); - } - }; - - scenario.client.getProtocolHandlers().put(new ContinueProtocolHandler() - { - @Override - protected void onContinue(org.eclipse.jetty.client.api.Request request) - { - super.onContinue(request); - content.offer(BufferUtil.toBuffer("Some content!"), Callback.from(content::close)); - } - }); - - CountDownLatch complete = new CountDownLatch(1); - AtomicReference response = new AtomicReference<>(); - BufferingResponseListener listener = new BufferingResponseListener() - { - @Override - public void onComplete(Result result) - { - response.set(result.getResponse()); - complete.countDown(); - } - }; - scenario.client.POST(scenario.newURI()) - .headers(headers -> headers.put(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE)) - .body(content) - .timeout(10, TimeUnit.SECONDS) - .send(listener); - - assertTrue(complete.await(10, TimeUnit.SECONDS)); - assertThat(response.get().getStatus(), is(200)); - assertThat(listener.getContentAsString(), is("read 13")); - } - @ParameterizedTest @ArgumentsSource(TransportProvider.class) public void test102Processing(Transport transport) throws Exception @@ -127,7 +63,7 @@ public void test102Processing(Transport transport) throws Exception scenario.start(new AbstractHandler() { @Override - public void handle(String target, Request jettyRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + public void handle(String target, Request jettyRequest, HttpServletRequest request, HttpServletResponse response) throws IOException { jettyRequest.setHandled(true); response.sendError(HttpStatus.PROCESSING_102); @@ -161,7 +97,7 @@ public Response.Listener getResponseListener() @Override public void onSuccess(Response response) { - org.eclipse.jetty.client.api.Request request = response.getRequest(); + var request = response.getRequest(); HttpConversation conversation = ((HttpRequest)request).getConversation(); // Reset the conversation listeners, since we are going to receive another response code conversation.updateResponseListeners(null); @@ -175,7 +111,7 @@ public void onSuccess(Response response) } else { - throw new IllegalStateException("should not have accepted"); + response.abort(new IllegalStateException("should not have accepted")); } } }; @@ -250,7 +186,7 @@ public Response.Listener getResponseListener() @Override public void onSuccess(Response response) { - org.eclipse.jetty.client.api.Request request = response.getRequest(); + var request = response.getRequest(); HttpConversation conversation = ((HttpRequest)request).getConversation(); // Reset the conversation listeners, since we are going to receive another response code conversation.updateResponseListeners(null); @@ -259,14 +195,13 @@ public void onSuccess(Response response) if (exchange != null && response.getStatus() == HttpStatus.EARLY_HINT_103) { // All good, continue. - System.err.println("onSuccess\n" + response.getHeaders()); hints.add(response.getHeaders().get("Hint")); exchange.resetResponse(); exchange.proceed(null); } else { - throw new IllegalStateException("should not have accepted"); + response.abort(new IllegalStateException("should not have accepted")); } } };