From 66cc353a2a537ca05830b99ba8c666af5eb64a23 Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 5 Jul 2023 15:52:06 +0200 Subject: [PATCH 01/16] Various Cleanup in ServletChannel Remove lambda for clarity --- .../jetty/ee10/servlet/ServletChannel.java | 128 +++++++++--------- 1 file changed, 66 insertions(+), 62 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index 53c0ea66d2b4..9ca10335aa06 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -20,6 +20,7 @@ import java.util.concurrent.atomic.AtomicLong; import jakarta.servlet.RequestDispatcher; +import jakarta.servlet.ServletException; import org.eclipse.jetty.ee10.servlet.ServletRequestState.Action; import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.HttpFields; @@ -476,73 +477,13 @@ public boolean handle() case DISPATCH: { - dispatch(() -> - { - try - { - _context.getServletContextHandler().requestInitialized(_servletContextRequest, _servletContextRequest.getServletApiRequest()); - - ServletHandler servletHandler = _context.getServletContextHandler().getServletHandler(); - ServletHandler.MappedServlet mappedServlet = _servletContextRequest.getMatchedResource().getResource(); - - mappedServlet.handle(servletHandler, Request.getPathInContext(_servletContextRequest), _servletContextRequest.getServletApiRequest(), _servletContextRequest.getHttpServletResponse()); - } - finally - { - _context.getServletContextHandler().requestDestroyed(_servletContextRequest, _servletContextRequest.getServletApiRequest()); - } - }); - + dispatch(this::normalDispatch); break; } case ASYNC_DISPATCH: { - dispatch(() -> - { - try - { - _context.getServletContextHandler().requestInitialized(_servletContextRequest, _servletContextRequest.getServletApiRequest()); - - HttpURI uri; - String pathInContext; - AsyncContextEvent asyncContextEvent = _state.getAsyncContextEvent(); - String dispatchString = asyncContextEvent.getDispatchPath(); - if (dispatchString != null) - { - String contextPath = _context.getContextPath(); - HttpURI.Immutable dispatchUri = HttpURI.from(dispatchString); - pathInContext = URIUtil.canonicalPath(dispatchUri.getPath()); - uri = HttpURI.build(_servletContextRequest.getHttpURI()) - .path(URIUtil.addPaths(contextPath, pathInContext)) - .query(dispatchUri.getQuery()); - } - else - { - uri = asyncContextEvent.getBaseURI(); - if (uri == null) - { - uri = _servletContextRequest.getHttpURI(); - pathInContext = Request.getPathInContext(_servletContextRequest); - } - else - { - pathInContext = uri.getCanonicalPath(); - if (_context.getContextPath().length() > 1) - pathInContext = pathInContext.substring(_context.getContextPath().length()); - } - } - // We first worked with the core pathInContext above, but now need to convert to servlet style - String decodedPathInContext = URIUtil.decodePath(pathInContext); - - Dispatcher dispatcher = new Dispatcher(getServletContextHandler(), uri, decodedPathInContext); - dispatcher.async(asyncContextEvent.getSuppliedRequest(), asyncContextEvent.getSuppliedResponse()); - } - finally - { - _context.getServletContextHandler().requestDestroyed(_servletContextRequest, _servletContextRequest.getServletApiRequest()); - } - }); + dispatch(this::asyncDispatch); break; } @@ -710,6 +651,69 @@ else if (ExceptionUtil.areNotAssociated(cause, x)) return !suspended; } + private void normalDispatch() throws ServletException, IOException + { + try + { + _context.getServletContextHandler().requestInitialized(_servletContextRequest, _servletContextRequest.getServletApiRequest()); + + ServletHandler servletHandler = _context.getServletContextHandler().getServletHandler(); + ServletHandler.MappedServlet mappedServlet = _servletContextRequest.getMatchedResource().getResource(); + + mappedServlet.handle(servletHandler, Request.getPathInContext(_servletContextRequest), _servletContextRequest.getServletApiRequest(), _servletContextRequest.getHttpServletResponse()); + } + finally + { + _context.getServletContextHandler().requestDestroyed(_servletContextRequest, _servletContextRequest.getServletApiRequest()); + } + } + + private void asyncDispatch() throws ServletException, IOException + { + try + { + _context.getServletContextHandler().requestInitialized(_servletContextRequest, _servletContextRequest.getServletApiRequest()); + + HttpURI uri; + String pathInContext; + AsyncContextEvent asyncContextEvent = _state.getAsyncContextEvent(); + String dispatchString = asyncContextEvent.getDispatchPath(); + if (dispatchString != null) + { + String contextPath = _context.getContextPath(); + HttpURI.Immutable dispatchUri = HttpURI.from(dispatchString); + pathInContext = URIUtil.canonicalPath(dispatchUri.getPath()); + uri = HttpURI.build(_servletContextRequest.getHttpURI()) + .path(URIUtil.addPaths(contextPath, pathInContext)) + .query(dispatchUri.getQuery()); + } + else + { + uri = asyncContextEvent.getBaseURI(); + if (uri == null) + { + uri = _servletContextRequest.getHttpURI(); + pathInContext = Request.getPathInContext(_servletContextRequest); + } + else + { + pathInContext = uri.getCanonicalPath(); + if (_context.getContextPath().length() > 1) + pathInContext = pathInContext.substring(_context.getContextPath().length()); + } + } + // We first worked with the core pathInContext above, but now need to convert to servlet style + String decodedPathInContext = URIUtil.decodePath(pathInContext); + + Dispatcher dispatcher = new Dispatcher(getServletContextHandler(), uri, decodedPathInContext); + dispatcher.async(asyncContextEvent.getSuppliedRequest(), asyncContextEvent.getSuppliedResponse()); + } + finally + { + _context.getServletContextHandler().requestDestroyed(_servletContextRequest, _servletContextRequest.getServletApiRequest()); + } + } + /** * @param message the error message. * @return true if we have sent an error, false if we have aborted. From adb2fb29a1c76a8c9afb45a350866f55a5d7ab86 Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 5 Jul 2023 19:10:48 +0200 Subject: [PATCH 02/16] Various Cleanup in ServletChannel removed unused variable --- .../main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java | 1 - 1 file changed, 1 deletion(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index 9ca10335aa06..20e9590af64f 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -897,7 +897,6 @@ public void onCompleted() // Callback will either be succeeded here or failed in abort(). Callback callback = _callback; - ServletContextRequest servletContextRequest = _servletContextRequest; // Must recycle before notification to allow for reuse. // Recycle always done here even if an abort is called. recycle(); From a02dd13537b6b2200ee6c2b7db51681e0100a3a2 Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 5 Jul 2023 19:15:42 +0200 Subject: [PATCH 03/16] Various Cleanup in ServletChannel Requests are always handled by ServletHandler, one way or another. --- .../org/eclipse/jetty/ee10/servlet/ServletChannel.java | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index 20e9590af64f..af43c52f2f4e 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -593,16 +593,6 @@ else if (ExceptionUtil.areNotAssociated(cause, x)) { if (!getServletContextResponse().isCommitted()) { - /* - TODO: isHandled does not exist and HttpOutput might not be explicitly closed. - if (!_request.isHandled() && !_request.getHttpOutput().isClosed()) - { - // The request was not actually handled - _response.writeError(HttpStatus.NOT_FOUND_404, _response.getCallback()); - break; - } - */ - // Indicate Connection:close if we can't consume all. if (getServletContextResponse().getStatus() >= 200) ResponseUtils.ensureConsumeAvailableOrNotPersistent(_servletContextRequest, _servletContextRequest.getServletContextResponse()); From 8faf41e41445282cc034a6f27be7b8dfb37b2b03 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Thu, 6 Jul 2023 16:37:21 +1000 Subject: [PATCH 04/16] make ServletChannel error dispatch async Signed-off-by: Lachlan Roberts --- .../jetty/ee10/servlet/ServletChannel.java | 21 +++++-------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index af43c52f2f4e..2e4aa4f7bed2 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -40,7 +40,6 @@ import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.handler.ContextHandler; import org.eclipse.jetty.server.handler.ContextRequest; -import org.eclipse.jetty.util.Blocker; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.ExceptionUtil; import org.eclipse.jetty.util.HostPort; @@ -185,11 +184,6 @@ public HttpInput getHttpInput() return _httpInput; } - public ServletContextHandler.ServletContextApi getServletContextContext() - { - return _servletContextApi; - } - public boolean isSendError() { return _state.isSendError(); @@ -525,16 +519,11 @@ public boolean handle() } else { - // TODO: do this non-blocking. - // Callback completeCallback = Callback.from(() -> _state.completed(null), _state::completed); - // _state.completing(); - try (Blocker.Callback blocker = Blocker.callback()) - { - // We do not notify ServletRequestListener on this dispatch because it might not - // be dispatched to an error page, so we delegate this responsibility to the ErrorHandler. - dispatch(() -> errorHandler.handle(_servletContextRequest, getServletContextResponse(), blocker)); - blocker.block(); - } + // We do not notify ServletRequestListener on this dispatch because it might not + // be dispatched to an error page, so we delegate this responsibility to the ErrorHandler. + // We must ignore the callback because the error servlet could go async. + dispatch(() -> + errorHandler.handle(_servletContextRequest, getServletContextResponse(), Callback.NOOP)); } } catch (Throwable x) From de21dd76d235222aedf7bf4cdd2c49b092bb69eb Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 5 Jul 2023 19:17:14 +0200 Subject: [PATCH 05/16] Various Cleanup in ServletChannel Servlet upgrade --- .../java/org/eclipse/jetty/ee10/servlet/ServletChannel.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index 2e4aa4f7bed2..74d42cce918d 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -849,6 +849,8 @@ public String toString() */ protected boolean checkAndPrepareUpgrade() { + // TODO do we need to re-implement servlet upgrade? + return false; } From 073e95a01ceead3bd6efeb46fa8e9e060ce29396 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Fri, 7 Jul 2023 10:07:56 +1000 Subject: [PATCH 06/16] scheduleDispatch after error handler callback Signed-off-by: Lachlan Roberts --- .../jetty/ee10/servlet/ServletChannel.java | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index 74d42cce918d..396e8601d392 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -17,6 +17,7 @@ import java.net.InetSocketAddress; import java.net.SocketAddress; import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; import jakarta.servlet.RequestDispatcher; @@ -519,11 +520,22 @@ public boolean handle() } else { + + AtomicInteger count = new AtomicInteger(); + Callback errorCallback = Callback.from(() -> + { + if (count.incrementAndGet() == 2) + _state.scheduleDispatch(); + }); + // We do not notify ServletRequestListener on this dispatch because it might not // be dispatched to an error page, so we delegate this responsibility to the ErrorHandler. - // We must ignore the callback because the error servlet could go async. - dispatch(() -> - errorHandler.handle(_servletContextRequest, getServletContextResponse(), Callback.NOOP)); + dispatch(() -> errorHandler.handle(_servletContextRequest, getServletContextResponse(), errorCallback)); + + // If the callback has already been completed we should continue in handle loop. + // Otherwise, the callback will schedule a dispatch to handle(). + if (count.incrementAndGet() != 2) + return false; } } catch (Throwable x) From c980ab5882d35f901a88aa33ab5cefd838cad9b5 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Mon, 17 Jul 2023 12:52:10 +1000 Subject: [PATCH 07/16] use AtomicBoolean instead of AtomicInteger for error dispatch callback Signed-off-by: Lachlan Roberts --- .../org/eclipse/jetty/ee10/servlet/ServletChannel.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index 396e8601d392..a217878c9a23 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -17,7 +17,7 @@ import java.net.InetSocketAddress; import java.net.SocketAddress; import java.util.concurrent.TimeoutException; -import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; import jakarta.servlet.RequestDispatcher; @@ -521,10 +521,10 @@ public boolean handle() else { - AtomicInteger count = new AtomicInteger(); + AtomicBoolean asyncCompletion = new AtomicBoolean(false); Callback errorCallback = Callback.from(() -> { - if (count.incrementAndGet() == 2) + if (!asyncCompletion.compareAndSet(false, true)) _state.scheduleDispatch(); }); @@ -534,7 +534,7 @@ public boolean handle() // If the callback has already been completed we should continue in handle loop. // Otherwise, the callback will schedule a dispatch to handle(). - if (count.incrementAndGet() != 2) + if (asyncCompletion.compareAndSet(false, true)) return false; } } From 0e2a0c1bee71e217ef53741edfd221201452876d Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Mon, 17 Jul 2023 17:50:40 +1000 Subject: [PATCH 08/16] remove checkAndPrepareUpgrade() from ServletChannel Signed-off-by: Lachlan Roberts --- .../jetty/ee10/servlet/ServletChannel.java | 21 ------------------- 1 file changed, 21 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index a217878c9a23..c30d2ed017a2 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -609,11 +609,6 @@ else if (ExceptionUtil.areNotAssociated(cause, x)) break; } - // If send error is called we need to break. - // TODO: is this necessary? It always returns false. - if (checkAndPrepareUpgrade()) - break; - // Set a close callback on the HttpOutput to make it an async callback getServletContextResponse().completeOutput(Callback.from(NON_BLOCKING, () -> _state.completed(null), _state::completed)); @@ -850,22 +845,6 @@ public String toString() timeStamp == 0 ? 0 : System.currentTimeMillis() - timeStamp); } - /** - *

Checks whether the processing of the request resulted in an upgrade, - * and if so performs upgrade preparation steps before the upgrade - * response is sent back to the client.

- *

This avoids a race where the server is unprepared if the client sends - * data immediately after having received the upgrade response.

- * @return true if the channel is not complete and more processing is required, - * typically because sendError has been called. - */ - protected boolean checkAndPrepareUpgrade() - { - // TODO do we need to re-implement servlet upgrade? - - return false; - } - void onTrailers(HttpFields trailers) { _servletContextRequest.setTrailers(trailers); From 68439bfda1aa41b5b573da3f345642234c04693d Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 19 Jul 2023 17:28:52 +1000 Subject: [PATCH 09/16] implement servlet upgrade for ee10 Signed-off-by: Lachlan Roberts --- .../org/eclipse/jetty/http/HttpParser.java | 4 +- .../jetty/http2/tests/HTTP2CServerTest.java | 2 +- .../jetty/server/AbstractConnector.java | 1 - .../server/ForwardedRequestCustomizer.java | 1 - .../server/{internal => }/HttpConnection.java | 21 ++-- .../jetty/server/HttpConnectionFactory.java | 1 - .../server/internal/HttpChannelState.java | 7 +- .../jetty/server/DelayedServerTest.java | 1 - .../jetty/server/ExtendedServerTest.java | 1 - .../jetty/server/HttpConnectionTest.java | 1 - .../jetty/server/HttpServerTestBase.java | 1 - .../jetty/server/ProxyProtocolTest.java | 1 - .../jetty/server/ResponseCompleteTest.java | 1 - .../org/eclipse/jetty/server/ServerTest.java | 1 - .../SlowClientWithPipelinedRequestTest.java | 1 - .../org/eclipse/jetty/server/StopTest.java | 1 - .../server/handler/DelayedHandlerTest.java | 4 +- .../ssl/SniSslConnectionFactoryTest.java | 4 +- .../server/ssl/SslConnectionFactoryTest.java | 4 +- .../jetty/ee10/servlet/ServletApiRequest.java | 99 ++++++++++++++++++- .../ee10/test/GzipWithSendErrorTest.java | 2 +- .../jetty/ee9/test/GzipWithSendErrorTest.java | 3 +- 22 files changed, 120 insertions(+), 42 deletions(-) rename jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/{internal => }/HttpConnection.java (98%) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java index 0ecfb33d8321..d2dc28b9d6a0 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java @@ -1922,8 +1922,8 @@ public void reset() public void servletUpgrade() { - setState(State.CONTENT); - _endOfContent = EndOfContent.UNKNOWN_CONTENT; + setState(State.EOF_CONTENT); + _endOfContent = EndOfContent.EOF_CONTENT; _contentLength = -1; } diff --git a/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HTTP2CServerTest.java b/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HTTP2CServerTest.java index ffbc90e291df..81d009db80cf 100644 --- a/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HTTP2CServerTest.java +++ b/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HTTP2CServerTest.java @@ -42,9 +42,9 @@ import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.EndPoint; import org.eclipse.jetty.server.Connector; +import org.eclipse.jetty.server.HttpConnection; import org.eclipse.jetty.server.HttpConnectionFactory; import org.eclipse.jetty.server.ServerConnector; -import org.eclipse.jetty.server.internal.HttpConnection; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.Utf8StringBuilder; diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java index 13bd50fec21d..2f2d880bb678 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java @@ -36,7 +36,6 @@ import org.eclipse.jetty.io.EndPoint; import org.eclipse.jetty.io.RetainableByteBuffer; import org.eclipse.jetty.io.ssl.SslConnection; -import org.eclipse.jetty.server.internal.HttpConnection; import org.eclipse.jetty.util.ProcessorUtils; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.annotation.ManagedAttribute; diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java index 9d9512ff5b8a..664b71f93c02 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java @@ -27,7 +27,6 @@ import org.eclipse.jetty.http.HttpScheme; import org.eclipse.jetty.http.HttpURI; import org.eclipse.jetty.http.QuotedCSVParser; -import org.eclipse.jetty.server.internal.HttpConnection; import org.eclipse.jetty.util.HostPort; import org.eclipse.jetty.util.Index; import org.eclipse.jetty.util.StringUtil; diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java similarity index 98% rename from jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java rename to jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java index 3034d4d1f9f9..2f04a43b24f4 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java @@ -11,7 +11,7 @@ // ======================================================================== // -package org.eclipse.jetty.server.internal; +package org.eclipse.jetty.server; import java.io.IOException; import java.net.SocketAddress; @@ -57,15 +57,7 @@ import org.eclipse.jetty.io.RuntimeIOException; import org.eclipse.jetty.io.WriteFlusher; import org.eclipse.jetty.io.ssl.SslConnection; -import org.eclipse.jetty.server.ConnectionFactory; -import org.eclipse.jetty.server.ConnectionMetaData; -import org.eclipse.jetty.server.Connector; -import org.eclipse.jetty.server.HttpChannel; -import org.eclipse.jetty.server.HttpConfiguration; -import org.eclipse.jetty.server.HttpStream; -import org.eclipse.jetty.server.Request; -import org.eclipse.jetty.server.Server; -import org.eclipse.jetty.server.TunnelSupport; +import org.eclipse.jetty.server.internal.HttpChannelState; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.HostPort; @@ -1096,18 +1088,17 @@ public void earlyEOF() HttpStreamOverHTTP1 stream = _stream.get(); if (stream != null) { - BadMessageException bad = new BadMessageException("Early EOF"); - + EofException eof = new EofException("Early EOF"); if (Content.Chunk.isFailure(stream._chunk)) - stream._chunk.getFailure().addSuppressed(bad); + stream._chunk.getFailure().addSuppressed(eof); else { if (stream._chunk != null) stream._chunk.release(); - stream._chunk = Content.Chunk.from(bad); + stream._chunk = Content.Chunk.from(eof); } - Runnable todo = _httpChannel.onFailure(bad); + Runnable todo = _httpChannel.onFailure(eof); if (todo != null) getServer().getThreadPool().execute(todo); } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java index 55167f4ae631..f7262d8d5ef7 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java @@ -18,7 +18,6 @@ import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.EndPoint; -import org.eclipse.jetty.server.internal.HttpConnection; import org.eclipse.jetty.util.annotation.Name; /** diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java index f1673add8868..e2952ca522af 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java @@ -51,6 +51,7 @@ import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.HttpChannel; import org.eclipse.jetty.server.HttpConfiguration; +import org.eclipse.jetty.server.HttpConnection; import org.eclipse.jetty.server.HttpStream; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.RequestLog; @@ -1272,7 +1273,11 @@ public void succeeded() httpChannel.lockedStreamSendCompleted(true); } if (callback != null) - httpChannel._serializedInvoker.run(callback::succeeded); + { + // We cannot use the SerializedInvoker here because this may be called from a blocking call within + // the application which is running inside the SerializedInvoker, then it is a deadlock. + callback.succeeded(); + } } /** diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/DelayedServerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/DelayedServerTest.java index 0047a14fc050..d95d9a07d9d8 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/DelayedServerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/DelayedServerTest.java @@ -19,7 +19,6 @@ import org.eclipse.jetty.http.MetaData; import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.EndPoint; -import org.eclipse.jetty.server.internal.HttpConnection; import org.eclipse.jetty.util.Callback; import org.junit.jupiter.api.BeforeEach; diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ExtendedServerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ExtendedServerTest.java index 66684f6951e8..f2c49bb24f2a 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ExtendedServerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ExtendedServerTest.java @@ -25,7 +25,6 @@ import org.eclipse.jetty.io.ManagedSelector; import org.eclipse.jetty.io.SocketChannelEndPoint; import org.eclipse.jetty.server.internal.HttpChannelState; -import org.eclipse.jetty.server.internal.HttpConnection; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.NanoTime; diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java index 5e7b46a85c87..72dc60c67737 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java @@ -44,7 +44,6 @@ import org.eclipse.jetty.io.Content; import org.eclipse.jetty.logging.StacklessLogging; import org.eclipse.jetty.server.handler.DumpHandler; -import org.eclipse.jetty.server.internal.HttpConnection; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.NanoTime; diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java index 7748d93ebed6..ae10c43335c7 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java @@ -48,7 +48,6 @@ import org.eclipse.jetty.logging.StacklessLogging; import org.eclipse.jetty.server.handler.EchoHandler; import org.eclipse.jetty.server.handler.HelloHandler; -import org.eclipse.jetty.server.internal.HttpConnection; import org.eclipse.jetty.util.Blocker; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ProxyProtocolTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ProxyProtocolTest.java index c2da721c2319..14e30fcdf3d6 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ProxyProtocolTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ProxyProtocolTest.java @@ -24,7 +24,6 @@ import java.util.Arrays; import org.eclipse.jetty.io.EndPoint; -import org.eclipse.jetty.server.internal.HttpConnection; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.TypeUtil; import org.junit.jupiter.api.AfterEach; diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseCompleteTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseCompleteTest.java index e6e1d003aa77..f79f25ae6598 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseCompleteTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseCompleteTest.java @@ -26,7 +26,6 @@ import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.EndPoint; import org.eclipse.jetty.logging.StacklessLogging; -import org.eclipse.jetty.server.internal.HttpConnection; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.component.LifeCycle; diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerTest.java index b2cf089d6d23..7f61f6491e41 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerTest.java @@ -33,7 +33,6 @@ import org.eclipse.jetty.io.QuietException; import org.eclipse.jetty.server.handler.ContextHandler; import org.eclipse.jetty.server.internal.HttpChannelState; -import org.eclipse.jetty.server.internal.HttpConnection; import org.eclipse.jetty.util.Blocker; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.component.LifeCycle; diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/SlowClientWithPipelinedRequestTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/SlowClientWithPipelinedRequestTest.java index 9defb422283a..fd0905a8b692 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/SlowClientWithPipelinedRequestTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/SlowClientWithPipelinedRequestTest.java @@ -23,7 +23,6 @@ import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.EndPoint; -import org.eclipse.jetty.server.internal.HttpConnection; import org.eclipse.jetty.util.Callback; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/StopTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/StopTest.java index 8406d6f1a72f..555206a2375d 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/StopTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/StopTest.java @@ -31,7 +31,6 @@ import org.eclipse.jetty.server.handler.ContextHandler; import org.eclipse.jetty.server.handler.ContextHandlerCollection; import org.eclipse.jetty.server.handler.StatisticsHandler; -import org.eclipse.jetty.server.internal.HttpConnection; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.FutureCallback; diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/DelayedHandlerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/DelayedHandlerTest.java index 88c213a3132d..4b0daad89863 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/DelayedHandlerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/DelayedHandlerTest.java @@ -298,7 +298,7 @@ public boolean handle(Request request, Response response, Callback callback) thr ByteArrayOutputStream out = new ByteArrayOutputStream(8192); new Throwable().printStackTrace(new PrintStream(out)); String stack = out.toString(StandardCharsets.ISO_8859_1); - assertThat(stack, containsString("org.eclipse.jetty.server.internal.HttpConnection.onFillable")); + assertThat(stack, containsString("org.eclipse.jetty.server.HttpConnection.onFillable")); assertThat(stack, containsString("org.eclipse.jetty.server.handler.DelayedHandler.handle")); // Check the content is available @@ -469,7 +469,7 @@ public boolean handle(Request request, Response response, Callback callback) thr ByteArrayOutputStream out = new ByteArrayOutputStream(8192); new Throwable().printStackTrace(new PrintStream(out)); String stack = out.toString(StandardCharsets.ISO_8859_1); - assertThat(stack, containsString("org.eclipse.jetty.server.internal.HttpConnection.onFillable")); + assertThat(stack, containsString("org.eclipse.jetty.server.HttpConnection.onFillable")); assertThat(stack, containsString("org.eclipse.jetty.server.handler.DelayedHandler.handle")); Fields fields = FormFields.from(request).get(1, TimeUnit.NANOSECONDS); diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SniSslConnectionFactoryTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SniSslConnectionFactoryTest.java index 792b2847dc7c..e817348e47a7 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SniSslConnectionFactoryTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SniSslConnectionFactoryTest.java @@ -489,8 +489,8 @@ protected void customize(Socket socket, Class connection, assertEquals("customize connector class org.eclipse.jetty.io.ssl.SslConnection,false", history.poll()); assertEquals("customize ssl class org.eclipse.jetty.io.ssl.SslConnection,false", history.poll()); - assertEquals("customize connector class org.eclipse.jetty.server.internal.HttpConnection,true", history.poll()); - assertEquals("customize http class org.eclipse.jetty.server.internal.HttpConnection,true", history.poll()); + assertEquals("customize connector class org.eclipse.jetty.server.HttpConnection,true", history.poll()); + assertEquals("customize http class org.eclipse.jetty.server.HttpConnection,true", history.poll()); assertEquals(0, history.size()); } diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SslConnectionFactoryTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SslConnectionFactoryTest.java index ebe49e17e0e1..04f4aef7305c 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SslConnectionFactoryTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SslConnectionFactoryTest.java @@ -178,8 +178,8 @@ protected void customize(Socket socket, Class connection, assertEquals("customize connector class org.eclipse.jetty.io.ssl.SslConnection,false", history.poll()); assertEquals("customize ssl class org.eclipse.jetty.io.ssl.SslConnection,false", history.poll()); - assertEquals("customize connector class org.eclipse.jetty.server.internal.HttpConnection,true", history.poll()); - assertEquals("customize http class org.eclipse.jetty.server.internal.HttpConnection,true", history.poll()); + assertEquals("customize connector class org.eclipse.jetty.server.HttpConnection,true", history.poll()); + assertEquals("customize http class org.eclipse.jetty.server.HttpConnection,true", history.poll()); assertEquals(0, history.size()); } diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java index 9dbf56b12659..a5718e7e70a6 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java @@ -44,6 +44,7 @@ import jakarta.servlet.ServletContext; import jakarta.servlet.ServletException; import jakarta.servlet.ServletInputStream; +import jakarta.servlet.ServletOutputStream; import jakarta.servlet.ServletRequest; import jakarta.servlet.ServletRequestAttributeEvent; import jakarta.servlet.ServletRequestAttributeListener; @@ -56,6 +57,7 @@ import jakarta.servlet.http.HttpUpgradeHandler; import jakarta.servlet.http.Part; import jakarta.servlet.http.PushBuilder; +import jakarta.servlet.http.WebConnection; import org.eclipse.jetty.ee10.servlet.ServletContextHandler.ServletRequestInfo; import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.CookieCache; @@ -70,12 +72,14 @@ import org.eclipse.jetty.http.HttpURI; import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http.MimeTypes; +import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.QuietException; import org.eclipse.jetty.io.RuntimeIOException; import org.eclipse.jetty.security.AuthenticationState; import org.eclipse.jetty.security.UserIdentity; import org.eclipse.jetty.server.ConnectionMetaData; import org.eclipse.jetty.server.FormFields; +import org.eclipse.jetty.server.HttpConnection; import org.eclipse.jetty.server.HttpCookieUtils; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.Response; @@ -591,8 +595,94 @@ public Part getPart(String name) throws IOException, ServletException @Override public T upgrade(Class handlerClass) throws IOException, ServletException { - // Not implemented. Throw ServletException as per spec. - throw new ServletException("Not implemented"); + Response response = _servletContextRequest.getServletContextResponse(); + if (response.getStatus() != HttpStatus.SWITCHING_PROTOCOLS_101) + throw new IllegalStateException("Response status should be 101"); + if (response.getHeaders().get("Upgrade") == null) + throw new IllegalStateException("Missing Upgrade header"); + if (!"Upgrade".equalsIgnoreCase(response.getHeaders().get("Connection"))) + throw new IllegalStateException("Invalid Connection header"); + if (response.isCommitted()) + throw new IllegalStateException("Cannot upgrade committed response"); + if (_servletChannel.getConnectionMetaData().getHttpVersion() != HttpVersion.HTTP_1_1) + throw new IllegalStateException("Only requests over HTTP/1.1 can be upgraded"); + + ServletOutputStream outputStream = _servletContextRequest.getHttpOutput(); + ServletInputStream inputStream = _servletContextRequest.getHttpInput(); + + T upgradeHandler; + try + { + upgradeHandler = handlerClass.getDeclaredConstructor().newInstance(); + } + catch (Exception e) + { + throw new ServletException("Unable to instantiate handler class", e); + } + + HttpConnection httpConnection = (HttpConnection)_servletContextRequest.getConnectionMetaData().getConnection(); + httpConnection.getParser().servletUpgrade(); + AsyncContext asyncContext = forceStartAsync(); // force the servlet in async mode + + outputStream.flush(); // commit the 101 response + httpConnection.getGenerator().servletUpgrade(); // tell the generator it can send data as-is + httpConnection.addEventListener(new Connection.Listener() + { + @Override + public void onClosed(Connection connection) + { + try + { + asyncContext.complete(); + } + catch (Exception e) + { + LOG.warn("error during upgrade AsyncContext complete", e); + } + try + { + upgradeHandler.destroy(); + } + catch (Exception e) + { + LOG.warn("error during upgrade HttpUpgradeHandler destroy", e); + } + } + + @Override + public void onOpened(Connection connection) + { + } + }); + + upgradeHandler.init(new WebConnection() + { + @Override + public void close() throws Exception + { + try + { + inputStream.close(); + } + finally + { + outputStream.close(); + } + } + + @Override + public ServletInputStream getInputStream() + { + return inputStream; + } + + @Override + public ServletOutputStream getOutputStream() + { + return outputStream; + } + }); + return upgradeHandler; } @Override @@ -1207,6 +1297,11 @@ public AsyncContext startAsync() throws IllegalStateException { if (!isAsyncSupported()) throw new IllegalStateException("Async Not Supported"); + return forceStartAsync(); + } + + private AsyncContext forceStartAsync() + { ServletRequestState state = getServletRequestInfo().getState(); if (_async == null) _async = new AsyncContextState(state); diff --git a/jetty-ee10/jetty-ee10-tests/jetty-ee10-test-integration/src/test/java/org/eclipse/jetty/ee10/test/GzipWithSendErrorTest.java b/jetty-ee10/jetty-ee10-tests/jetty-ee10-test-integration/src/test/java/org/eclipse/jetty/ee10/test/GzipWithSendErrorTest.java index 67c5bfb08e47..63d851f604d9 100644 --- a/jetty-ee10/jetty-ee10-tests/jetty-ee10-test-integration/src/test/java/org/eclipse/jetty/ee10/test/GzipWithSendErrorTest.java +++ b/jetty-ee10/jetty-ee10-tests/jetty-ee10-test-integration/src/test/java/org/eclipse/jetty/ee10/test/GzipWithSendErrorTest.java @@ -44,10 +44,10 @@ import org.eclipse.jetty.ee10.servlet.ServletContextRequest; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpMethod; +import org.eclipse.jetty.server.HttpConnection; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.server.handler.gzip.GzipHandler; -import org.eclipse.jetty.server.internal.HttpConnection; import org.eclipse.jetty.toolchain.test.MavenTestingUtils; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.IO; diff --git a/jetty-ee9/jetty-ee9-tests/jetty-ee9-test-integration/src/test/java/org/eclipse/jetty/ee9/test/GzipWithSendErrorTest.java b/jetty-ee9/jetty-ee9-tests/jetty-ee9-test-integration/src/test/java/org/eclipse/jetty/ee9/test/GzipWithSendErrorTest.java index 25542d3dcf98..46248a3a8a37 100644 --- a/jetty-ee9/jetty-ee9-tests/jetty-ee9-test-integration/src/test/java/org/eclipse/jetty/ee9/test/GzipWithSendErrorTest.java +++ b/jetty-ee9/jetty-ee9-tests/jetty-ee9-test-integration/src/test/java/org/eclipse/jetty/ee9/test/GzipWithSendErrorTest.java @@ -41,11 +41,10 @@ import org.eclipse.jetty.ee9.servlet.ServletContextHandler; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpMethod; +import org.eclipse.jetty.server.HttpConnection; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.server.handler.gzip.GzipHandler; -import org.eclipse.jetty.server.internal.HttpChannelState; -import org.eclipse.jetty.server.internal.HttpConnection; import org.eclipse.jetty.toolchain.test.MavenTestingUtils; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.IO; From 452a71bd2008846934e3558c226fa7e6776f50f7 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Thu, 20 Jul 2023 11:49:04 +1000 Subject: [PATCH 10/16] adjust implementation to fix tests Signed-off-by: Lachlan Roberts --- .../org/eclipse/jetty/server/HttpConnection.java | 2 +- .../jetty/server/internal/HttpChannelState.java | 14 ++++++++++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java index 2f04a43b24f4..1227794e1946 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java @@ -1088,7 +1088,7 @@ public void earlyEOF() HttpStreamOverHTTP1 stream = _stream.get(); if (stream != null) { - EofException eof = new EofException("Early EOF"); + BadMessageException eof = new BadMessageException("Early EOF"); if (Content.Chunk.isFailure(stream._chunk)) stream._chunk.getFailure().addSuppressed(eof); else diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java index e2952ca522af..252891855bea 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java @@ -1274,9 +1274,10 @@ public void succeeded() } if (callback != null) { - // We cannot use the SerializedInvoker here because this may be called from a blocking call within - // the application which is running inside the SerializedInvoker, then it is a deadlock. - callback.succeeded(); + // Do not use SerializedInvoker here because application code running within + // SerializedInvoker could issue blocking write() or close() that would deadlock. + // We must execute in case of recursive write within the callback. + getRequest().getContext().execute(callback::succeeded); } } @@ -1305,7 +1306,12 @@ public void failed(Throwable x) httpChannel.lockedStreamSendCompleted(false); } if (callback != null) - httpChannel._serializedInvoker.run(() -> callback.failed(x)); + { + // Do not use SerializedInvoker here because application code running within + // SerializedInvoker could issue blocking write() or close() that would deadlock. + // We must execute in case of recursive write within the callback. + getRequest().getContext().execute(() -> callback.failed(x)); + } } @Override From f222c45f8e020b33d4682c7cf127f7294c155f2f Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Fri, 18 Aug 2023 16:32:38 +1000 Subject: [PATCH 11/16] re-enable ServletUpgradeTest for ee10 Signed-off-by: Lachlan Roberts --- .../ForwardedRequestCustomizerTest.java | 1 - .../jetty/ee10/servlet/ServletChannel.java | 63 ------------------- .../ee10/servlet/ServletUpgradeTest.java | 2 - 3 files changed, 66 deletions(-) diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ForwardedRequestCustomizerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ForwardedRequestCustomizerTest.java index 7f1e65270f7e..7e1fa2dc7a7f 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ForwardedRequestCustomizerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ForwardedRequestCustomizerTest.java @@ -26,7 +26,6 @@ import org.eclipse.jetty.http.HttpTester; import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.EndPoint; -import org.eclipse.jetty.server.internal.HttpConnection; import org.eclipse.jetty.util.Callback; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index 38d78aeb3144..6984269709f2 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -647,69 +647,6 @@ private void reopen() getHttpOutput().reopen(); } - private void normalDispatch() throws ServletException, IOException - { - try - { - _context.getServletContextHandler().requestInitialized(_servletContextRequest, _servletContextRequest.getServletApiRequest()); - - ServletHandler servletHandler = _context.getServletContextHandler().getServletHandler(); - ServletHandler.MappedServlet mappedServlet = _servletContextRequest.getMatchedResource().getResource(); - - mappedServlet.handle(servletHandler, Request.getPathInContext(_servletContextRequest), _servletContextRequest.getServletApiRequest(), _servletContextRequest.getHttpServletResponse()); - } - finally - { - _context.getServletContextHandler().requestDestroyed(_servletContextRequest, _servletContextRequest.getServletApiRequest()); - } - } - - private void asyncDispatch() throws ServletException, IOException - { - try - { - _context.getServletContextHandler().requestInitialized(_servletContextRequest, _servletContextRequest.getServletApiRequest()); - - HttpURI uri; - String pathInContext; - AsyncContextEvent asyncContextEvent = _state.getAsyncContextEvent(); - String dispatchString = asyncContextEvent.getDispatchPath(); - if (dispatchString != null) - { - String contextPath = _context.getContextPath(); - HttpURI.Immutable dispatchUri = HttpURI.from(dispatchString); - pathInContext = URIUtil.canonicalPath(dispatchUri.getPath()); - uri = HttpURI.build(_servletContextRequest.getHttpURI()) - .path(URIUtil.addPaths(contextPath, pathInContext)) - .query(dispatchUri.getQuery()); - } - else - { - uri = asyncContextEvent.getBaseURI(); - if (uri == null) - { - uri = _servletContextRequest.getHttpURI(); - pathInContext = Request.getPathInContext(_servletContextRequest); - } - else - { - pathInContext = uri.getCanonicalPath(); - if (_context.getContextPath().length() > 1) - pathInContext = pathInContext.substring(_context.getContextPath().length()); - } - } - // We first worked with the core pathInContext above, but now need to convert to servlet style - String decodedPathInContext = URIUtil.decodePath(pathInContext); - - Dispatcher dispatcher = new Dispatcher(getServletContextHandler(), uri, decodedPathInContext); - dispatcher.async(asyncContextEvent.getSuppliedRequest(), asyncContextEvent.getSuppliedResponse()); - } - finally - { - _context.getServletContextHandler().requestDestroyed(_servletContextRequest, _servletContextRequest.getServletApiRequest()); - } - } - /** * @param message the error message. * @return true if we have sent an error, false if we have aborted. diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletUpgradeTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletUpgradeTest.java index 8fd6a338b95f..819c2c928d63 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletUpgradeTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletUpgradeTest.java @@ -31,7 +31,6 @@ import org.eclipse.jetty.server.ServerConnector; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -72,7 +71,6 @@ public void tearDown() throws Exception server.stop(); } - @Disabled @Test public void upgradeTest() throws Exception { From c942a7ee65656c5024906ee98e5457e20eaed1c3 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 17 Jul 2024 10:20:57 +1000 Subject: [PATCH 12/16] wip Signed-off-by: Lachlan Roberts --- .../jetty/ee10/servlet/ServletApiRequest.java | 68 +++-- .../jetty/ee10/servlet/ServletChannel.java | 8 +- .../ee10/servlet/ServletUpgradeTest.java | 236 ++++++------------ 3 files changed, 131 insertions(+), 181 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java index 30109e4a2ed2..702ba4053671 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java @@ -37,6 +37,8 @@ import java.util.concurrent.ExecutionException; import jakarta.servlet.AsyncContext; +import jakarta.servlet.AsyncEvent; +import jakarta.servlet.AsyncListener; import jakarta.servlet.DispatcherType; import jakarta.servlet.RequestDispatcher; import jakarta.servlet.ServletConnection; @@ -70,8 +72,8 @@ import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http.MimeTypes; import org.eclipse.jetty.http.SetCookieParser; -import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.http.pathmap.MatchedResource; +import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.QuietException; import org.eclipse.jetty.io.RuntimeIOException; import org.eclipse.jetty.security.AuthenticationState; @@ -769,27 +771,47 @@ public T upgrade(Class handlerClass) throws IO outputStream.flush(); // commit the 101 response httpConnection.getGenerator().servletUpgrade(); // tell the generator it can send data as-is + + _servletContextRequest.addFailureListener(t -> + { + System.err.println("failureListener: " + t); + asyncContext.complete(); + }); + + _servletContextRequest.getServletChannel().setReportRemoteErrors(true); + asyncContext.addListener(new AsyncListener() { + @Override + public void onComplete(AsyncEvent event) throws IOException + { + System.err.println("AsyncListener onComplete"); + } + + @Override + public void onTimeout(AsyncEvent event) throws IOException + { + System.err.println("AsyncListener onTimeout"); + } + + @Override + public void onError(AsyncEvent event) throws IOException + { + System.err.println("AsyncListener onError"); + } + + @Override + public void onStartAsync(AsyncEvent event) throws IOException + { + System.err.println("AsyncListener onStartAsync"); + } + }); + httpConnection.addEventListener(new Connection.Listener() { @Override public void onClosed(Connection connection) { - try - { - asyncContext.complete(); - } - catch (Exception e) - { - LOG.warn("error during upgrade AsyncContext complete", e); - } - try - { - upgradeHandler.destroy(); - } - catch (Exception e) - { - LOG.warn("error during upgrade HttpUpgradeHandler destroy", e); - } + System.err.println("Connection.Listener onClosed: " + connection); + System.err.println("asyncContext: " + asyncContext); } @Override @@ -803,14 +825,10 @@ public void onOpened(Connection connection) @Override public void close() throws Exception { - try - { - inputStream.close(); - } - finally - { - outputStream.close(); - } + IO.close(inputStream); + IO.close(outputStream); + asyncContext.complete(); + upgradeHandler.destroy(); } @Override diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index 4a6ebb620d4a..b29697a193b0 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -596,6 +596,12 @@ public void handle() LOG.debug("!handle {} {}", action, this); } + private boolean reportRemoteErrors; + public void setReportRemoteErrors(boolean reportRemoteErrors) + { + this.reportRemoteErrors = reportRemoteErrors; + } + private void reopen() { _servletContextRequest.getServletContextResponse().getHttpOutput().reopen(); @@ -661,7 +667,7 @@ else if (noStack != null) LOG.warn(request == null ? "unknown request" : request.getServletApiRequest().getRequestURI(), failure); } - if (isCommitted()) + if (isCommitted() && !reportRemoteErrors) { abort(failure); } diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletUpgradeTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletUpgradeTest.java index 819c2c928d63..a182de23d67d 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletUpgradeTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletUpgradeTest.java @@ -17,6 +17,11 @@ import java.io.InputStream; import java.io.OutputStream; import java.net.Socket; +import java.nio.charset.StandardCharsets; +import java.util.Arrays; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import jakarta.servlet.ReadListener; import jakarta.servlet.ServletException; @@ -29,6 +34,8 @@ import jakarta.servlet.http.WebConnection; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; +import org.eclipse.jetty.util.IO; +import org.eclipse.jetty.util.Utf8StringBuilder; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -37,6 +44,8 @@ import static org.eclipse.jetty.util.StringUtil.CRLF; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.endsWith; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -46,10 +55,13 @@ public class ServletUpgradeTest private Server server; private int port; + private static CountDownLatch destroyLatch; @BeforeEach public void setUp() throws Exception { + destroyLatch = new CountDownLatch(1); + server = new Server(); ServerConnector connector = new ServerConnector(server); @@ -74,126 +86,65 @@ public void tearDown() throws Exception @Test public void upgradeTest() throws Exception { - boolean passed1 = false; - boolean passed2 = false; - boolean passed3 = false; - String expectedResponse1 = "TCKHttpUpgradeHandler.init"; - String expectedResponse2 = "onDataAvailable|Hello"; - String expectedResponse3 = "onDataAvailable|World"; - - InputStream input = null; - OutputStream output = null; - Socket s = null; - - try + Socket socket = new Socket("localhost", port); + socket.setSoTimeout(0); + InputStream input = socket.getInputStream(); + OutputStream output = socket.getOutputStream(); + + String request = "POST /TestServlet HTTP/1.1" + CRLF + + "Host: localhost:" + port + CRLF + + "Accept: text/html, image/gif, image/jpeg, *; q=.2, */*; q=.2" + CRLF + + "Upgrade: YES" + CRLF + + "Connection: Upgrade" + CRLF + + "Content-type: application/x-www-form-urlencoded" + CRLF + + CRLF; + + output.write(request.getBytes()); + writeChunk(output, "Hello"); + writeChunk(output, "World"); + output.flush(); + socket.shutdownOutput(); + + CompletableFuture futureContent = new CompletableFuture<>(); + new Thread(() -> { - s = new Socket("localhost", port); - output = s.getOutputStream(); - - StringBuilder reqStr = new StringBuilder() - .append("POST /TestServlet HTTP/1.1").append(CRLF) - .append("User-Agent: Java/1.6.0_33").append(CRLF) - .append("Host: localhost:").append(port).append(CRLF) - .append("Accept: text/html, image/gif, image/jpeg, *; q=.2, */*; q=.2").append(CRLF) - .append("Upgrade: YES").append(CRLF) - .append("Connection: Upgrade").append(CRLF) - .append("Content-type: application/x-www-form-urlencoded").append(CRLF) - .append(CRLF); - - LOG.info("REQUEST=========" + reqStr.toString()); - output.write(reqStr.toString().getBytes()); - - LOG.info("Writing first chunk"); - writeChunk(output, "Hello"); - - LOG.info("Writing second chunk"); - writeChunk(output, "World"); - LOG.info("Consuming the response from the server"); - - // Consume the response from the server - input = s.getInputStream(); - int len; - byte[] b = new byte[1024]; - boolean receivedFirstMessage = false; - boolean receivedSecondMessage = false; - boolean receivedThirdMessage = false; - StringBuilder sb = new StringBuilder(); - while ((len = input.read(b)) != -1) - { - String line = new String(b, 0, len); - sb.append(line); - LOG.info("==============Read from server:" + CRLF + sb + CRLF); - if (passed1 = compareString(expectedResponse1, sb.toString())) - { - LOG.info("==============Received first expected response!" + CRLF); - receivedFirstMessage = true; - } - if (passed2 = compareString(expectedResponse2, sb.toString())) - { - LOG.info("==============Received second expected response!" + CRLF); - receivedSecondMessage = true; - } - if (passed3 = compareString(expectedResponse3, sb.toString())) - { - LOG.info("==============Received third expected response!" + CRLF); - receivedThirdMessage = true; - } - LOG.info("receivedFirstMessage : " + receivedFirstMessage); - LOG.info("receivedSecondMessage : " + receivedSecondMessage); - LOG.info("receivedThirdMessage : " + receivedThirdMessage); - if (receivedFirstMessage && receivedSecondMessage && receivedThirdMessage) - { - break; - } - } - } - finally - { + Utf8StringBuilder sb = new Utf8StringBuilder(); try { - if (input != null) + while (true) { - LOG.info("Closing input..."); - input.close(); - LOG.info("Input closed."); + int read = input.read(); + if (read == -1) + break; + sb.append((byte)read); } + futureContent.complete(sb.toCompleteString()); } - catch (Exception ex) + catch (Throwable t) { - LOG.error("Failed to close input:" + ex.getMessage(), ex); + LOG.warn("failed with content: " + sb, t); + futureContent.completeExceptionally(t); } - try - { - if (output != null) - { - LOG.info("Closing output..."); - output.close(); - LOG.info("Output closed ."); - } - } - catch (Exception ex) - { - LOG.error("Failed to close output:" + ex.getMessage(), ex); - } + }).start(); - try - { - if (s != null) - { - LOG.info("Closing socket..." + CRLF); - s.close(); - LOG.info("Socked closed."); - } - } - catch (Exception ex) - { - LOG.error("Failed to close socket:" + ex.getMessage(), ex); - } - } +// socket.close(); - assertTrue(passed1 && passed2 && passed3); + // TODO test for the 101 response. + String content = futureContent.get(500000, TimeUnit.SECONDS); + String expectedContent = """ + TCKHttpUpgradeHandler.init\r + =onDataAvailable\r + HelloWorld\r + =onAllDataRead\r + """; + assertThat(content, endsWith(expectedContent)); + + input.close(); + output.close(); + socket.close(); + assertTrue(destroyLatch.await(5, TimeUnit.SECONDS)); } private static class TestServlet extends HttpServlet @@ -225,7 +176,8 @@ public TestHttpUpgradeHandler() @Override public void destroy() { - LOG.debug("===============destroy"); + System.err.println("destroy"); + destroyLatch.countDown(); } @Override @@ -235,9 +187,9 @@ public void init(WebConnection wc) { ServletInputStream input = wc.getInputStream(); ServletOutputStream output = wc.getOutputStream(); - TestReadListener readListener = new TestReadListener("/", input, output); + TestReadListener readListener = new TestReadListener(wc, input, output); input.setReadListener(readListener); - output.println("===============TCKHttpUpgradeHandler.init"); + output.println("TCKHttpUpgradeHandler.init"); output.flush(); } catch (Exception ex) @@ -249,22 +201,25 @@ public void init(WebConnection wc) private static class TestReadListener implements ReadListener { + private final WebConnection wc; private final ServletInputStream input; private final ServletOutputStream output; - private final String delimiter; + private boolean outputOnDataAvailable = false; - TestReadListener(String del, ServletInputStream in, ServletOutputStream out) + TestReadListener(WebConnection wc, ServletInputStream in, ServletOutputStream out) { + this.wc = wc; input = in; output = out; - delimiter = del; } + @Override public void onAllDataRead() { try { - output.println("=onAllDataRead"); + System.err.println("onAllDataRead"); + output.println("\r\n=onAllDataRead"); output.close(); } catch (Exception ex) @@ -273,11 +228,17 @@ public void onAllDataRead() } } + @Override public void onDataAvailable() { try { - output.println("=onDataAvailable"); + if (!outputOnDataAvailable) + { + outputOnDataAvailable = true; + output.println("=onDataAvailable"); + } + StringBuilder sb = new StringBuilder(); int len; byte[] b = new byte[1024]; @@ -285,60 +246,25 @@ public void onDataAvailable() { String data = new String(b, 0, len); sb.append(data); + System.err.println("len: " + len); } - output.println(delimiter + sb.toString()); + output.print(sb.toString()); output.flush(); } catch (Exception ex) { + System.err.println("onDataAvailable " + ex); throw new IllegalStateException(ex); } } + @Override public void onError(final Throwable t) { LOG.error("TestReadListener error", t); } } - private static boolean compareString(String expected, String actual) - { - String[] listExpected = expected.split("[|]"); - boolean found = true; - for (int i = 0, n = listExpected.length, startIdx = 0, bodyLength = actual.length(); i < n; i++) - { - String search = listExpected[i]; - if (startIdx >= bodyLength) - { - startIdx = bodyLength; - } - - int searchIdx = actual.toLowerCase().indexOf(search.toLowerCase(), startIdx); - - LOG.debug("[ServletTestUtil] Scanning response for " + "search string: '" + search + "' starting at index " + "location: " + startIdx); - if (searchIdx < 0) - { - found = false; - String s = "[ServletTestUtil] Unable to find the following " + - "search string in the server's " + - "response: '" + search + "' at index: " + - startIdx + - "\n[ServletTestUtil] Server's response:\n" + - "-------------------------------------------\n" + - actual + - "\n-------------------------------------------\n"; - LOG.debug(s); - break; - } - - LOG.debug("[ServletTestUtil] Found search string: '" + search + "' at index '" + searchIdx + "' in the server's " + "response"); - // the new searchIdx is the old index plus the lenght of the - // search string. - startIdx = searchIdx + search.length(); - } - return found; - } - private static void writeChunk(OutputStream out, String data) throws IOException { if (data != null) From 36a85a8153ccceee1b23fdbdf1be9fb940dd2f69 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Thu, 25 Jul 2024 07:23:22 +1000 Subject: [PATCH 13/16] add listener to ServletChannel to properly implement servlet upgrade Signed-off-by: Lachlan Roberts --- .../eclipse/jetty/ee10/servlet/HttpInput.java | 5 ++ .../jetty/ee10/servlet/HttpOutput.java | 2 + .../jetty/ee10/servlet/ServletApiRequest.java | 63 +++---------------- .../jetty/ee10/servlet/ServletChannel.java | 29 +++++++++ .../ee10/servlet/ServletUpgradeTest.java | 21 ++----- 5 files changed, 50 insertions(+), 70 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java index de0f83f8a0e9..904d4164b857 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java @@ -261,6 +261,7 @@ private int read(ByteBuffer buffer, byte[] b, int off, int len) throws IOExcepti if (Content.Chunk.isFailure(chunk)) { Throwable failure = chunk.getFailure(); + _servletChannel.readComplete(); if (LOG.isDebugEnabled()) LOG.debug("read failure={} {}", failure, this); if (failure instanceof IOException) @@ -271,6 +272,7 @@ private int read(ByteBuffer buffer, byte[] b, int off, int len) throws IOExcepti // Empty and not a failure; can only be EOF as per ContentProducer.nextChunk() contract. if (LOG.isDebugEnabled()) LOG.debug("read at EOF, setting consumed EOF to true {}", this); + _servletChannel.readComplete(); _consumedEof = true; // Do we need to wake for allDataRead callback? if (onContentProducible()) @@ -355,6 +357,7 @@ public void run() if (LOG.isDebugEnabled()) LOG.debug("running failure={} {}", failure, this); _readListener.onError(failure); + _servletChannel.readComplete(); } else if (chunk.isLast() && !chunk.hasRemaining()) { @@ -363,12 +366,14 @@ else if (chunk.isLast() && !chunk.hasRemaining()) if (LOG.isDebugEnabled()) LOG.debug("running at EOF {}", this); _readListener.onAllDataRead(); + _servletChannel.readComplete(); } catch (Throwable x) { if (LOG.isDebugEnabled()) LOG.debug("running failed onAllDataRead {}", this, x); _readListener.onError(x); + _servletChannel.readComplete(); } } else diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java index 419fc0d9ebf5..b2d372183ab6 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java @@ -219,6 +219,7 @@ private void onWriteComplete(boolean last, Throwable failure) // Transition to CLOSED state if we were the last write or we have failed if (last || failure != null) { + _servletChannel.writeComplete(); _state = State.CLOSED; closedCallback = _closedCallback; _closedCallback = null; @@ -359,6 +360,7 @@ public void complete(Callback callback) if (error != null) { _servletChannel.abort(error); + _servletChannel.writeComplete(); _state = State.CLOSED; } else diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java index 702ba4053671..53ae43a24a5d 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java @@ -37,8 +37,6 @@ import java.util.concurrent.ExecutionException; import jakarta.servlet.AsyncContext; -import jakarta.servlet.AsyncEvent; -import jakarta.servlet.AsyncListener; import jakarta.servlet.DispatcherType; import jakarta.servlet.RequestDispatcher; import jakarta.servlet.ServletConnection; @@ -73,7 +71,6 @@ import org.eclipse.jetty.http.MimeTypes; import org.eclipse.jetty.http.SetCookieParser; import org.eclipse.jetty.http.pathmap.MatchedResource; -import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.QuietException; import org.eclipse.jetty.io.RuntimeIOException; import org.eclipse.jetty.security.AuthenticationState; @@ -772,63 +769,13 @@ public T upgrade(Class handlerClass) throws IO outputStream.flush(); // commit the 101 response httpConnection.getGenerator().servletUpgrade(); // tell the generator it can send data as-is - _servletContextRequest.addFailureListener(t -> - { - System.err.println("failureListener: " + t); - asyncContext.complete(); - }); - - _servletContextRequest.getServletChannel().setReportRemoteErrors(true); - asyncContext.addListener(new AsyncListener() { - @Override - public void onComplete(AsyncEvent event) throws IOException - { - System.err.println("AsyncListener onComplete"); - } - - @Override - public void onTimeout(AsyncEvent event) throws IOException - { - System.err.println("AsyncListener onTimeout"); - } - - @Override - public void onError(AsyncEvent event) throws IOException - { - System.err.println("AsyncListener onError"); - } - - @Override - public void onStartAsync(AsyncEvent event) throws IOException - { - System.err.println("AsyncListener onStartAsync"); - } - }); - - httpConnection.addEventListener(new Connection.Listener() - { - @Override - public void onClosed(Connection connection) - { - System.err.println("Connection.Listener onClosed: " + connection); - System.err.println("asyncContext: " + asyncContext); - } - - @Override - public void onOpened(Connection connection) - { - } - }); - - upgradeHandler.init(new WebConnection() + WebConnection webConnection = new WebConnection() { @Override public void close() throws Exception { IO.close(inputStream); IO.close(outputStream); - asyncContext.complete(); - upgradeHandler.destroy(); } @Override @@ -842,7 +789,15 @@ public ServletOutputStream getOutputStream() { return outputStream; } + }; + + _servletContextRequest.getServletChannel().setCompleteListener(() -> + { + asyncContext.complete(); + upgradeHandler.destroy(); }); + + upgradeHandler.init(webConnection); return upgradeHandler; } diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index b29697a193b0..c39a114037bb 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -17,6 +17,7 @@ import java.net.InetSocketAddress; import java.net.SocketAddress; import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; import jakarta.servlet.RequestDispatcher; @@ -78,6 +79,13 @@ public class ServletChannel private Request _request; private Response _response; private Callback _callback; + private CompleteListener _completeListener; + private final AtomicInteger _completeCount = new AtomicInteger(2); + + public interface CompleteListener + { + void complete(); + } public ServletChannel(ServletContextHandler servletContextHandler, Request request) { @@ -883,4 +891,25 @@ public void dispatchAsync() throws Exception servletContextHandler.requestDestroyed(servletContextRequest, servletApiRequest); } } + + public void readComplete() + { + if (_completeCount.decrementAndGet() == 0 && _completeListener != null) + { + _completeListener.complete(); + } + } + + public void writeComplete() + { + if (_completeCount.decrementAndGet() == 0 && _completeListener != null) + { + _completeListener.complete(); + } + } + + public void setCompleteListener(CompleteListener listener) + { + _completeListener = listener; + } } diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletUpgradeTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletUpgradeTest.java index a182de23d67d..943f97e459bc 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletUpgradeTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletUpgradeTest.java @@ -17,8 +17,6 @@ import java.io.InputStream; import java.io.OutputStream; import java.net.Socket; -import java.nio.charset.StandardCharsets; -import java.util.Arrays; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; @@ -34,7 +32,6 @@ import jakarta.servlet.http.WebConnection; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; -import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.Utf8StringBuilder; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; @@ -45,8 +42,8 @@ import static org.eclipse.jetty.util.StringUtil.CRLF; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.endsWith; -import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.startsWith; import static org.junit.jupiter.api.Assertions.assertTrue; public class ServletUpgradeTest @@ -129,16 +126,14 @@ public void upgradeTest() throws Exception }).start(); -// socket.close(); - - // TODO test for the 101 response. - String content = futureContent.get(500000, TimeUnit.SECONDS); + String content = futureContent.get(5, TimeUnit.SECONDS); String expectedContent = """ TCKHttpUpgradeHandler.init\r =onDataAvailable\r HelloWorld\r =onAllDataRead\r """; + assertThat(content, startsWith("HTTP/1.1 101 Switching Protocols")); assertThat(content, endsWith(expectedContent)); input.close(); @@ -176,7 +171,6 @@ public TestHttpUpgradeHandler() @Override public void destroy() { - System.err.println("destroy"); destroyLatch.countDown(); } @@ -187,7 +181,7 @@ public void init(WebConnection wc) { ServletInputStream input = wc.getInputStream(); ServletOutputStream output = wc.getOutputStream(); - TestReadListener readListener = new TestReadListener(wc, input, output); + TestReadListener readListener = new TestReadListener(input, output); input.setReadListener(readListener); output.println("TCKHttpUpgradeHandler.init"); output.flush(); @@ -201,14 +195,12 @@ public void init(WebConnection wc) private static class TestReadListener implements ReadListener { - private final WebConnection wc; private final ServletInputStream input; private final ServletOutputStream output; private boolean outputOnDataAvailable = false; - TestReadListener(WebConnection wc, ServletInputStream in, ServletOutputStream out) + TestReadListener(ServletInputStream in, ServletOutputStream out) { - this.wc = wc; input = in; output = out; } @@ -218,7 +210,6 @@ public void onAllDataRead() { try { - System.err.println("onAllDataRead"); output.println("\r\n=onAllDataRead"); output.close(); } @@ -246,14 +237,12 @@ public void onDataAvailable() { String data = new String(b, 0, len); sb.append(data); - System.err.println("len: " + len); } output.print(sb.toString()); output.flush(); } catch (Exception ex) { - System.err.println("onDataAvailable " + ex); throw new IllegalStateException(ex); } } From ef6021d72dad2f9610db7cbe8f6b089a280f9d1b Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Thu, 25 Jul 2024 07:41:11 +1000 Subject: [PATCH 14/16] fix checkstyle issues Signed-off-by: Lachlan Roberts --- .../java/org/eclipse/jetty/ee10/servlet/ServletChannel.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index c39a114037bb..73136542ba0f 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -81,6 +81,7 @@ public class ServletChannel private Callback _callback; private CompleteListener _completeListener; private final AtomicInteger _completeCount = new AtomicInteger(2); + private boolean _reportRemoteErrors; public interface CompleteListener { @@ -604,10 +605,9 @@ public void handle() LOG.debug("!handle {} {}", action, this); } - private boolean reportRemoteErrors; public void setReportRemoteErrors(boolean reportRemoteErrors) { - this.reportRemoteErrors = reportRemoteErrors; + this._reportRemoteErrors = reportRemoteErrors; } private void reopen() @@ -675,7 +675,7 @@ else if (noStack != null) LOG.warn(request == null ? "unknown request" : request.getServletApiRequest().getRequestURI(), failure); } - if (isCommitted() && !reportRemoteErrors) + if (isCommitted() && !_reportRemoteErrors) { abort(failure); } From af300f0fc278e09dccd28c4b49cd96436a60503e Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 6 Aug 2024 15:42:55 +1000 Subject: [PATCH 15/16] PR #10128 - change ordering in the ServletChannel complete listener Signed-off-by: Lachlan Roberts --- .../java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java index 564ca7ca4032..28e1aed5fd8a 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java @@ -793,8 +793,8 @@ public ServletOutputStream getOutputStream() _servletContextRequest.getServletChannel().setCompleteListener(() -> { - asyncContext.complete(); upgradeHandler.destroy(); + asyncContext.complete(); }); upgradeHandler.init(webConnection); From aa8eeaa05aba5ba213b80cc090b986b2b40bd02c Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 20 Aug 2024 09:58:58 +1000 Subject: [PATCH 16/16] changes to ServletUpgradeTest Signed-off-by: Lachlan Roberts --- .../java/org/eclipse/jetty/ee10/servlet/ServletUpgradeTest.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletUpgradeTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletUpgradeTest.java index 943f97e459bc..49b7076e36a8 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletUpgradeTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletUpgradeTest.java @@ -90,10 +90,8 @@ public void upgradeTest() throws Exception String request = "POST /TestServlet HTTP/1.1" + CRLF + "Host: localhost:" + port + CRLF + - "Accept: text/html, image/gif, image/jpeg, *; q=.2, */*; q=.2" + CRLF + "Upgrade: YES" + CRLF + "Connection: Upgrade" + CRLF + - "Content-type: application/x-www-form-urlencoded" + CRLF + CRLF; output.write(request.getBytes());