From 944b9e8943c896fc8bed1def1dd844bfa64034c0 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Tue, 30 Jul 2024 19:05:52 +0200 Subject: [PATCH 1/2] Improvements to HttpSender. * Changed ContentSender demand from iterate()+IDLE to succeeded()+SCHEDULED. This ensures that there is no re-iteration in case a 100 Continue response arrives. This, in turn, avoids that the demand is performed multiple times, causing ISE to be thrown. * Changed the 100 Continue action of the proxy Servlet/Handler, that provides the request content, to be executed by the HttpSender, rather than by the HttpReceiver. Signed-off-by: Simone Bordet --- .../jetty/client/ContinueProtocolHandler.java | 11 ++- .../jetty/client/transport/HttpChannel.java | 4 +- .../jetty/client/transport/HttpExchange.java | 4 +- .../jetty/client/transport/HttpSender.java | 93 ++++++++++++------- .../org/eclipse/jetty/proxy/ProxyHandler.java | 10 +- .../ee10/proxy/AbstractProxyServlet.java | 9 +- .../ee10/proxy/AsyncMiddleManServlet.java | 8 +- .../jetty/ee10/proxy/ProxyServlet.java | 10 +- .../jetty/ee9/proxy/AbstractProxyServlet.java | 9 +- .../ee9/proxy/AsyncMiddleManServlet.java | 8 +- .../eclipse/jetty/ee9/proxy/ProxyServlet.java | 10 +- 11 files changed, 98 insertions(+), 78 deletions(-) diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/ContinueProtocolHandler.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/ContinueProtocolHandler.java index 146b241e6440..213e0ef2586d 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/ContinueProtocolHandler.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/ContinueProtocolHandler.java @@ -52,8 +52,9 @@ public Response.Listener getResponseListener() return new ContinueListener(); } - protected void onContinue(Request request) + protected Runnable onContinue(Request request) { + return null; } protected class ContinueListener extends BufferingResponseListener @@ -79,8 +80,10 @@ public void onSuccess(Response response) { // All good, continue. exchange.resetResponse(); - exchange.proceed(null); - onContinue(request); + Runnable proceedAction = onContinue(request); + // Pass the proceed action to be executed + // by the sender, not here by the receiver. + exchange.proceed(proceedAction, null); } else { @@ -90,7 +93,7 @@ public void onSuccess(Response response) ResponseListeners listeners = exchange.getResponseListeners(); HttpContentResponse contentResponse = new HttpContentResponse(response, getContent(), getMediaType(), getEncoding()); listeners.emitSuccess(contentResponse); - exchange.proceed(new HttpRequestException("Expectation failed", request)); + exchange.proceed(null, new HttpRequestException("Expectation failed", request)); } } diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpChannel.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpChannel.java index b524ede09f9f..6545d31d07ff 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpChannel.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpChannel.java @@ -146,9 +146,9 @@ public void send() public abstract void release(); - public void proceed(HttpExchange exchange, Throwable failure) + public void proceed(HttpExchange exchange, Runnable proceedAction, Throwable failure) { - getHttpSender().proceed(exchange, failure); + getHttpSender().proceed(exchange, proceedAction, failure); } public void abort(HttpExchange exchange, Throwable requestFailure, Throwable responseFailure, Promise promise) diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpExchange.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpExchange.java index b68815bea285..0028e6c3a4bd 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpExchange.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpExchange.java @@ -317,11 +317,11 @@ public void resetResponse() } } - public void proceed(Throwable failure) + public void proceed(Runnable proceedAction, Throwable failure) { HttpChannel channel = getHttpChannel(); if (channel != null) - channel.proceed(this, failure); + channel.proceed(this, proceedAction, failure); } @Override diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpSender.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpSender.java index 551a210ae0e7..43903c1437a7 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpSender.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpSender.java @@ -317,12 +317,15 @@ protected void dispose() { } - public void proceed(HttpExchange exchange, Throwable failure) + public void proceed(HttpExchange exchange, Runnable proceedAction, Throwable failure) { - // Received a 100 Continue, although Expect header was not sent. + // Received a 100 Continue, although the Expect header was not sent. if (!contentSender.expect100) return; + // Write the fields in this order, since the reader of + // these fields will read them in the opposite order. + contentSender.proceedAction = proceedAction; contentSender.expect100 = false; if (failure == null) { @@ -462,32 +465,39 @@ private enum RequestState private class ContentSender extends IteratingCallback { - private HttpExchange exchange; + // Fields that are set externally. + private volatile HttpExchange exchange; + private volatile Runnable proceedAction; + private volatile boolean expect100; + // Fields only used internally. private Content.Chunk chunk; private ByteBuffer contentBuffer; - private boolean expect100; private boolean committed; private boolean success; private boolean complete; private Promise abort; + private boolean demanded; @Override public boolean reset() { exchange = null; + proceedAction = null; + expect100 = false; chunk = null; contentBuffer = null; - expect100 = false; committed = false; success = false; complete = false; abort = null; + demanded = false; return super.reset(); } @Override protected Action process() throws Throwable { + HttpExchange exchange = this.exchange; if (complete) { if (success) @@ -498,15 +508,26 @@ protected Action process() throws Throwable HttpRequest request = exchange.getRequest(); Content.Source content = request.getBody(); + boolean expect100 = this.expect100; if (expect100) { + // If the request was sent already, wait for + // the 100 response before sending the content. if (committed) return Action.IDLE; - else - chunk = null; + // Do not send any content yet. + chunk = null; } else { + // Run the proceed action first, which likely will provide + // content after having received the 100 Continue response. + Runnable action = proceedAction; + proceedAction = null; + if (action != null) + action.run(); + + // Read the request content. chunk = content.read(); } if (LOG.isDebugEnabled()) @@ -516,11 +537,14 @@ protected Action process() throws Throwable { if (committed) { - content.demand(this::iterate); - return Action.IDLE; + // No content after the headers, demand. + demanded = true; + content.demand(this::succeeded); + return Action.SCHEDULED; } else { + // Normalize to avoid null checks. chunk = Content.Chunk.EMPTY; } } @@ -545,49 +569,50 @@ protected Action process() throws Throwable @Override protected void onSuccess() { - boolean proceed = true; - if (committed) + if (demanded) { - if (contentBuffer.hasRemaining()) - proceed = someToContent(exchange, contentBuffer); + // Content is now available, reset + // the demand and iterate again. + demanded = false; } else { - committed = true; - if (headersToCommit(exchange)) + boolean proceed = true; + if (committed) { - // Was any content sent while committing? if (contentBuffer.hasRemaining()) proceed = someToContent(exchange, contentBuffer); } else { - proceed = false; + committed = true; + proceed = headersToCommit(exchange); + if (proceed) + { + // Was any content sent while committing? + if (contentBuffer.hasRemaining()) + proceed = someToContent(exchange, contentBuffer); + } } - } - boolean last = chunk.isLast(); - chunk.release(); - chunk = null; + boolean last = chunk.isLast(); + chunk.release(); + chunk = null; - if (proceed) - { - if (last) + if (proceed) { - success = true; - complete = true; + if (last) + { + success = true; + complete = true; + } } - else if (expect100) + else { - if (LOG.isDebugEnabled()) - LOG.debug("Expecting 100 Continue for {}", exchange.getRequest()); + // There was some concurrent error, terminate. + complete = true; } } - else - { - // There was some concurrent error, terminate. - complete = true; - } } @Override diff --git a/jetty-core/jetty-proxy/src/main/java/org/eclipse/jetty/proxy/ProxyHandler.java b/jetty-core/jetty-proxy/src/main/java/org/eclipse/jetty/proxy/ProxyHandler.java index 99baae9f5910..f72236241881 100644 --- a/jetty-core/jetty-proxy/src/main/java/org/eclipse/jetty/proxy/ProxyHandler.java +++ b/jetty-core/jetty-proxy/src/main/java/org/eclipse/jetty/proxy/ProxyHandler.java @@ -444,12 +444,11 @@ protected void onServerToProxyResponseFailure(Request clientToProxyRequest, org. Response.writeError(clientToProxyRequest, proxyToClientResponse, callback, status); } - protected void onServerToProxyResponse100Continue(Request clientToProxyRequest, org.eclipse.jetty.client.Request proxyToServerRequest) + protected Runnable onServerToProxyResponse100Continue(Request clientToProxyRequest, org.eclipse.jetty.client.Request proxyToServerRequest) { if (LOG.isDebugEnabled()) LOG.debug("{} P2C 100 continue response", requestId(clientToProxyRequest)); - Runnable action = (Runnable)proxyToServerRequest.getAttributes().get(PROXY_TO_SERVER_CONTINUE_ATTRIBUTE); - action.run(); + return (Runnable)proxyToServerRequest.getAttributes().get(PROXY_TO_SERVER_CONTINUE_ATTRIBUTE); } protected void onServerToProxyResponse102Processing(Request clientToProxyRequest, org.eclipse.jetty.client.Request proxyToServerRequest, HttpFields serverToProxyResponseHeaders, Response proxyToClientResponse) @@ -776,13 +775,12 @@ public InvocationType getInvocationType() private class ProxyContinueProtocolHandler extends ContinueProtocolHandler { @Override - protected void onContinue(org.eclipse.jetty.client.Request proxyToServerRequest) + protected Runnable onContinue(org.eclipse.jetty.client.Request proxyToServerRequest) { - super.onContinue(proxyToServerRequest); var clientToProxyRequest = (Request)proxyToServerRequest.getAttributes().get(CLIENT_TO_PROXY_REQUEST_ATTRIBUTE); if (LOG.isDebugEnabled()) LOG.debug("{} S2P received 100 Continue", requestId(clientToProxyRequest)); - onServerToProxyResponse100Continue(clientToProxyRequest, proxyToServerRequest); + return onServerToProxyResponse100Continue(clientToProxyRequest, proxyToServerRequest); } } diff --git a/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AbstractProxyServlet.java b/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AbstractProxyServlet.java index 8cad022eec05..dd7db3f33a05 100644 --- a/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AbstractProxyServlet.java +++ b/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AbstractProxyServlet.java @@ -763,10 +763,9 @@ protected void sendProxyResponseError(HttpServletRequest clientRequest, HttpServ } } - protected void onContinue(HttpServletRequest clientRequest, Request proxyRequest) + protected Runnable onContinue(HttpServletRequest clientRequest, Request proxyRequest) { - if (_log.isDebugEnabled()) - _log.debug("{} handling 100 Continue", getRequestId(clientRequest)); + return null; } /** @@ -851,10 +850,10 @@ protected String rewriteTarget(HttpServletRequest request) class ProxyContinueProtocolHandler extends ContinueProtocolHandler { @Override - protected void onContinue(Request request) + protected Runnable onContinue(Request request) { HttpServletRequest clientRequest = (HttpServletRequest)request.getAttributes().get(CLIENT_REQUEST_ATTRIBUTE); - AbstractProxyServlet.this.onContinue(clientRequest, request); + return AbstractProxyServlet.this.onContinue(clientRequest, request); } } } diff --git a/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AsyncMiddleManServlet.java b/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AsyncMiddleManServlet.java index 765495e64add..a03be8bdb542 100644 --- a/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AsyncMiddleManServlet.java +++ b/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AsyncMiddleManServlet.java @@ -171,11 +171,11 @@ protected ContentTransformer newServerResponseContentTransformer(HttpServletRequ } @Override - protected void onContinue(HttpServletRequest clientRequest, Request proxyRequest) + protected Runnable onContinue(HttpServletRequest clientRequest, Request proxyRequest) { - super.onContinue(clientRequest, proxyRequest); - Runnable action = (Runnable)proxyRequest.getAttributes().get(CONTINUE_ACTION_ATTRIBUTE); - action.run(); + if (_log.isDebugEnabled()) + _log.debug("{} handling 100 Continue", getRequestId(clientRequest)); + return (Runnable)proxyRequest.getAttributes().get(CONTINUE_ACTION_ATTRIBUTE); } private void transform(ContentTransformer transformer, ByteBuffer input, boolean finished, List output) throws IOException diff --git a/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/ProxyServlet.java b/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/ProxyServlet.java index c8e0f532b97f..bfb8c26c783e 100644 --- a/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/ProxyServlet.java +++ b/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/ProxyServlet.java @@ -16,7 +16,6 @@ import java.io.IOException; import java.io.InputStream; import java.nio.ByteBuffer; -import java.util.concurrent.Executor; import java.util.concurrent.TimeUnit; import jakarta.servlet.AsyncContext; @@ -144,12 +143,11 @@ protected void onResponseContent(HttpServletRequest request, HttpServletResponse } @Override - protected void onContinue(HttpServletRequest clientRequest, Request proxyRequest) + protected Runnable onContinue(HttpServletRequest clientRequest, Request proxyRequest) { - super.onContinue(clientRequest, proxyRequest); - Runnable action = (Runnable)proxyRequest.getAttributes().get(CONTINUE_ACTION_ATTRIBUTE); - Executor executor = getHttpClient().getExecutor(); - executor.execute(action); + if (_log.isDebugEnabled()) + _log.debug("{} handling 100 Continue", getRequestId(clientRequest)); + return (Runnable)proxyRequest.getAttributes().get(CONTINUE_ACTION_ATTRIBUTE); } /** diff --git a/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/AbstractProxyServlet.java b/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/AbstractProxyServlet.java index 0b3acdacd530..f021c65f5b8e 100644 --- a/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/AbstractProxyServlet.java +++ b/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/AbstractProxyServlet.java @@ -768,10 +768,9 @@ protected void sendProxyResponseError(HttpServletRequest clientRequest, HttpServ } } - protected void onContinue(HttpServletRequest clientRequest, Request proxyRequest) + protected Runnable onContinue(HttpServletRequest clientRequest, Request proxyRequest) { - if (_log.isDebugEnabled()) - _log.debug("{} handling 100 Continue", getRequestId(clientRequest)); + return null; } /** @@ -856,10 +855,10 @@ protected String rewriteTarget(HttpServletRequest request) class ProxyContinueProtocolHandler extends ContinueProtocolHandler { @Override - protected void onContinue(Request request) + protected Runnable onContinue(Request request) { HttpServletRequest clientRequest = (HttpServletRequest)request.getAttributes().get(CLIENT_REQUEST_ATTRIBUTE); - AbstractProxyServlet.this.onContinue(clientRequest, request); + return AbstractProxyServlet.this.onContinue(clientRequest, request); } } } diff --git a/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/AsyncMiddleManServlet.java b/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/AsyncMiddleManServlet.java index b70bfab77475..e929a724a96f 100644 --- a/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/AsyncMiddleManServlet.java +++ b/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/AsyncMiddleManServlet.java @@ -171,11 +171,11 @@ protected ContentTransformer newServerResponseContentTransformer(HttpServletRequ } @Override - protected void onContinue(HttpServletRequest clientRequest, Request proxyRequest) + protected Runnable onContinue(HttpServletRequest clientRequest, Request proxyRequest) { - super.onContinue(clientRequest, proxyRequest); - Runnable action = (Runnable)proxyRequest.getAttributes().get(CONTINUE_ACTION_ATTRIBUTE); - action.run(); + if (_log.isDebugEnabled()) + _log.debug("{} handling 100 Continue", getRequestId(clientRequest)); + return (Runnable)proxyRequest.getAttributes().get(CONTINUE_ACTION_ATTRIBUTE); } private void transform(ContentTransformer transformer, ByteBuffer input, boolean finished, List output) throws IOException diff --git a/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/ProxyServlet.java b/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/ProxyServlet.java index f9520096bcf5..9691bcb96282 100644 --- a/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/ProxyServlet.java +++ b/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/ProxyServlet.java @@ -16,7 +16,6 @@ import java.io.IOException; import java.io.InputStream; import java.nio.ByteBuffer; -import java.util.concurrent.Executor; import java.util.concurrent.TimeUnit; import jakarta.servlet.AsyncContext; @@ -144,12 +143,11 @@ protected void onResponseContent(HttpServletRequest request, HttpServletResponse } @Override - protected void onContinue(HttpServletRequest clientRequest, Request proxyRequest) + protected Runnable onContinue(HttpServletRequest clientRequest, Request proxyRequest) { - super.onContinue(clientRequest, proxyRequest); - Runnable action = (Runnable)proxyRequest.getAttributes().get(CONTINUE_ACTION_ATTRIBUTE); - Executor executor = getHttpClient().getExecutor(); - executor.execute(action); + if (_log.isDebugEnabled()) + _log.debug("{} handling 100 Continue", getRequestId(clientRequest)); + return (Runnable)proxyRequest.getAttributes().get(CONTINUE_ACTION_ATTRIBUTE); } /** From 9110e89bf6bffa3735ff03136cde7b56eca9c9e4 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Fri, 2 Aug 2024 11:27:39 +0200 Subject: [PATCH 2/2] Updates after review. Signed-off-by: Simone Bordet --- .../org/eclipse/jetty/ee9/proxy/AbstractProxyServlet.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/AbstractProxyServlet.java b/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/AbstractProxyServlet.java index f021c65f5b8e..c8a6cd738b99 100644 --- a/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/AbstractProxyServlet.java +++ b/jetty-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/AbstractProxyServlet.java @@ -768,6 +768,14 @@ protected void sendProxyResponseError(HttpServletRequest clientRequest, HttpServ } } + /** + *

Returns the action to perform when the proxy receives + * a 100 Continue response from the server.

+ * + * @param clientRequest the client request + * @param proxyRequest the request being proxied + * @return the 100 Continue action to run + */ protected Runnable onContinue(HttpServletRequest clientRequest, Request proxyRequest) { return null;