From dae910a43dfcb092c821145d711a129efce24750 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Sun, 3 Dec 2023 18:46:58 +0100 Subject: [PATCH 1/7] Fixes #11016 - Jetty 12 IllegalStateException when stopping Server with pending requests * Made ServletChannel error handling more robust. A failure in error handling is now remembered so that the Handler callback can be failed later. * Avoid failing the Handler callback from ServletChannel.abort(), as it is too early: should be failed when processing the TERMINATED state, similarly to when it is succeeded. * Removed dead code from HttpConnection.SendCallback.reset(), since response is always non-null. Signed-off-by: Simone Bordet --- .../server/internal/HttpChannelState.java | 9 ++- .../jetty/server/internal/HttpConnection.java | 18 ++--- .../jetty/ee10/servlet/ErrorHandler.java | 2 +- .../jetty/ee10/servlet/ServletChannel.java | 40 +++++------- .../ee10/servlet/ServletChannelState.java | 31 +++++---- .../servlet/AsyncServletLongPollTest.java | 63 +++++++++++++++++- jetty-ee9/jetty-ee9-servlet/pom.xml | 5 ++ .../ee9/servlet/AsyncServletLongPollTest.java | 65 +++++++++++++++++-- 8 files changed, 174 insertions(+), 59 deletions(-) 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 811f4237d488..90077292ef07 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 @@ -1457,10 +1457,15 @@ public void failed(Throwable failure) Throwable unconsumed = stream.consumeAvailable(); ExceptionUtil.addSuppressedIfNotAssociated(failure, unconsumed); + ChannelResponse response = httpChannelState._response; if (LOG.isDebugEnabled()) - LOG.debug("failed stream.isCommitted={}, response.isCommitted={} {}", httpChannelState._stream.isCommitted(), httpChannelState._response.isCommitted(), this); + { + LOG.debug("failed stream.isCommitted={}, response.isCommitted={} {}", httpChannelState._stream.isCommitted(), response.isCommitted(), this); + } - if (!stream.isCommitted()) + // There may have been an attempt to write an error response that failed. + // Do not try to write again an error response if already committed. + if (!response.isCommitted()) errorResponse = new ErrorResponse(request); } 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/internal/HttpConnection.java index e61ed5a06154..70c31744b263 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/internal/HttpConnection.java @@ -740,26 +740,18 @@ private boolean reset(MetaData.Request request, MetaData.Response response, Byte _lastContent = last; _callback = callback; _header = null; - if (getConnector().isShutdown()) _generator.setPersistent(false); - return true; } - - if (isClosed() && response == null && last && content == null) + else { - callback.succeeded(); + if (isClosed()) + callback.failed(new EofException()); + else + callback.failed(new WritePendingException()); return false; } - - LOG.warn("reset failed {}", this); - - if (isClosed()) - callback.failed(new EofException()); - else - callback.failed(new WritePendingException()); - return false; } @Override diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ErrorHandler.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ErrorHandler.java index 4e05050e400e..521b6afa651b 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ErrorHandler.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ErrorHandler.java @@ -350,7 +350,7 @@ protected void generateAcceptableResponse(ServletContextRequest baseRequest, Htt } // Do an asynchronous completion. - baseRequest.getServletChannel().sendResponseAndComplete(); + baseRequest.getServletChannel().sendErrorResponseAndComplete(); } protected void handleErrorPage(HttpServletRequest request, Writer writer, int code, String message) throws IOException 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 3cbe97490fb4..be09a570b877 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 @@ -505,7 +505,7 @@ public void handle() // If we can't have a body or have no ErrorHandler, then create a minimal error response. if (HttpStatus.hasNoBody(getServletContextResponse().getStatus()) || errorHandler == null) { - sendResponseAndComplete(); + sendErrorResponseAndComplete(); } else { @@ -513,7 +513,7 @@ public void handle() // be dispatched to an error page, so we delegate this responsibility to the ErrorHandler. reopen(); _state.errorHandling(); - errorHandler.handle(getServletContextRequest(), getServletContextResponse(), Callback.from(_state::errorHandlingComplete)); + errorHandler.handle(getServletContextRequest(), getServletContextResponse(), Callback.from(() -> _state.errorHandlingComplete(null), _state::errorHandlingComplete)); } } catch (Throwable x) @@ -523,23 +523,16 @@ public void handle() else ExceptionUtil.addSuppressedIfNotAssociated(cause, x); if (LOG.isDebugEnabled()) - LOG.debug("Could not perform ERROR dispatch, aborting", cause); + LOG.debug("Could not perform error handling, aborting", cause); if (_state.isResponseCommitted()) { - abort(cause); + // Perform the same behavior as when the callback is failed. + _state.errorHandlingComplete(cause); } else { - try - { - getServletContextResponse().resetContent(); - sendResponseAndComplete(); - } - catch (Throwable t) - { - ExceptionUtil.addSuppressedIfNotAssociated(cause, t); - abort(cause); - } + getServletContextResponse().resetContent(); + sendErrorResponseAndComplete(); } } finally @@ -712,7 +705,7 @@ protected Throwable unwrap(Throwable failure, Class... targets) return null; } - public void sendResponseAndComplete() + public void sendErrorResponseAndComplete() { try { @@ -722,6 +715,7 @@ public void sendResponseAndComplete() catch (Throwable x) { abort(x); + _state.completed(x); } } @@ -777,8 +771,11 @@ public void onCompleted() // Callback will either be succeeded here or failed in abort(). Callback callback = _callback; - if (_state.completeResponse()) + Throwable failure = _state.completeResponse(); + if (failure == null) callback.succeeded(); + else + callback.failed(failure); } public boolean isCommitted() @@ -816,13 +813,10 @@ protected void execute(Runnable task) */ public void abort(Throwable failure) { - // Callback will either be failed here or succeeded in onCompleted(). - if (_state.abortResponse()) - { - if (LOG.isDebugEnabled()) - LOG.debug("abort {}", this, failure); - _callback.failed(failure); - } + // Callback will failed in onCompleted(). + boolean aborted = _state.abortResponse(failure); + if (LOG.isDebugEnabled()) + LOG.debug("abort={} {}", aborted, this, failure); } private void dispatch() throws Exception diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannelState.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannelState.java index b0e6996a2550..c545b1b2b9de 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannelState.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannelState.java @@ -151,6 +151,7 @@ public enum Action private long _timeoutMs = DEFAULT_TIMEOUT; private AsyncContextEvent _event; private Thread _onTimeoutThread; + private Throwable _failure; protected ServletChannelState(ServletChannel servletChannel) { @@ -292,19 +293,13 @@ public String getStatusString() } } - public boolean completeResponse() + public Throwable completeResponse() { try (AutoLock ignored = lock()) { - switch (_outputState) - { - case OPEN: - _outputState = OutputState.COMPLETED; - return true; - - default: - return false; - } + if (_outputState == OutputState.OPEN) + _outputState = OutputState.COMPLETED; + return _failure; } } @@ -321,7 +316,7 @@ public boolean isResponseCompleted() } } - public boolean abortResponse() + public boolean abortResponse(Throwable failure) { try (AutoLock ignored = lock()) { @@ -334,10 +329,12 @@ public boolean abortResponse() case OPEN: _servletChannel.getServletContextResponse().setStatus(HttpStatus.INTERNAL_SERVER_ERROR_500); _outputState = OutputState.ABORTED; + _failure = failure; return true; default: _outputState = OutputState.ABORTED; + _failure = failure; return true; } } @@ -559,19 +556,25 @@ public void errorHandling() } } - public void errorHandlingComplete() + public void errorHandlingComplete(Throwable failure) { boolean handle; try (AutoLock ignored = lock()) { if (LOG.isDebugEnabled()) - LOG.debug("errorHandlingComplete {}", toStringLocked()); + LOG.debug("errorHandlingComplete {}", toStringLocked(), failure); handle = _state == State.WAITING; if (handle) _state = State.WOKEN; + + // If there is a failure while trying to + // handle a previous failure, just bail out. + if (failure != null) + abortResponse(failure); + if (_requestState == RequestState.ERRORING) - _requestState = RequestState.COMPLETE; + _requestState = failure == null ? RequestState.COMPLETE : RequestState.COMPLETED; } if (handle) scheduleDispatch(); diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/AsyncServletLongPollTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/AsyncServletLongPollTest.java index 2bedf4c2312a..4129ee58bf05 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/AsyncServletLongPollTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/AsyncServletLongPollTest.java @@ -15,23 +15,30 @@ import java.io.IOException; import java.io.OutputStream; +import java.net.InetSocketAddress; import java.net.Socket; +import java.nio.channels.SocketChannel; import java.nio.charset.StandardCharsets; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; import jakarta.servlet.AsyncContext; import jakarta.servlet.ServletException; import jakarta.servlet.http.HttpServlet; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; +import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpTester; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; +import org.hamcrest.Matchers; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; +import static org.awaitility.Awaitility.await; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; public class AsyncServletLongPollTest @@ -65,7 +72,7 @@ public void destroy() throws Exception @Test public void testSuspendedRequestCompletedByAnotherRequest() throws Exception { - final CountDownLatch asyncLatch = new CountDownLatch(1); + CountDownLatch asyncLatch = new CountDownLatch(1); prepare(new HttpServlet() { private volatile AsyncContext asyncContext; @@ -93,7 +100,7 @@ protected void doDelete(HttpServletRequest request, HttpServletResponse response if (param != null) error = Integer.parseInt(param); - final AsyncContext asyncContext = this.asyncContext; + AsyncContext asyncContext = this.asyncContext; if (asyncContext != null) { HttpServletResponse asyncResponse = (HttpServletResponse)asyncContext.getResponse(); @@ -152,4 +159,56 @@ protected void doDelete(HttpServletRequest request, HttpServletResponse response assertEquals(200, response3.getStatus()); } } + + @Test + public void testSuspendedRequestThenServerStop() throws Exception + { + AtomicReference asyncContextRef = new AtomicReference<>(); + prepare(new HttpServlet() + { + @Override + protected void service(HttpServletRequest request, HttpServletResponse response) + { + // Suspend the request. + AsyncContext asyncContext = request.startAsync(); + asyncContextRef.set(asyncContext); + } + + @Override + public void destroy() + { + // Try to write an error response when shutting down. + AsyncContext asyncContext = asyncContextRef.get(); + try + { + HttpServletResponse response = (HttpServletResponse)asyncContext.getResponse(); + response.sendError(HttpStatus.INTERNAL_SERVER_ERROR_500); + } + catch (IOException x) + { + throw new RuntimeException(x); + } + finally + { + asyncContext.complete(); + } + } + }); + + try (SocketChannel client = SocketChannel.open(new InetSocketAddress("localhost", connector.getLocalPort()))) + { + HttpTester.Request request = HttpTester.newRequest(); + request.setURI(uri); + client.write(request.generate()); + + await().atMost(5, TimeUnit.SECONDS).until(asyncContextRef::get, Matchers.notNullValue()); + + server.stop(); + + client.socket().setSoTimeout(1000); + // The connection has been closed, no response. + HttpTester.Response response = HttpTester.parseResponse(client); + assertNull(response); + } + } } diff --git a/jetty-ee9/jetty-ee9-servlet/pom.xml b/jetty-ee9/jetty-ee9-servlet/pom.xml index 4189797230dc..2df2c781f6a3 100644 --- a/jetty-ee9/jetty-ee9-servlet/pom.xml +++ b/jetty-ee9/jetty-ee9-servlet/pom.xml @@ -39,6 +39,11 @@ org.slf4j slf4j-api + + org.awaitility + awaitility + test + org.eclipse.jetty jetty-client diff --git a/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/AsyncServletLongPollTest.java b/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/AsyncServletLongPollTest.java index 4ffc95034f39..9a5db257a2b6 100644 --- a/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/AsyncServletLongPollTest.java +++ b/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/AsyncServletLongPollTest.java @@ -15,27 +15,32 @@ import java.io.IOException; import java.io.OutputStream; +import java.net.InetSocketAddress; import java.net.Socket; +import java.nio.channels.SocketChannel; import java.nio.charset.StandardCharsets; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; import jakarta.servlet.AsyncContext; import jakarta.servlet.ServletException; import jakarta.servlet.http.HttpServlet; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; +import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpTester; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; +import org.hamcrest.Matchers; import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; +import static org.awaitility.Awaitility.await; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; -@Disabled // TODO public class AsyncServletLongPollTest { private Server server; @@ -66,7 +71,7 @@ public void destroy() throws Exception @Test public void testSuspendedRequestCompletedByAnotherRequest() throws Exception { - final CountDownLatch asyncLatch = new CountDownLatch(1); + CountDownLatch asyncLatch = new CountDownLatch(1); prepare(new HttpServlet() { private volatile AsyncContext asyncContext; @@ -94,7 +99,7 @@ protected void doDelete(HttpServletRequest request, HttpServletResponse response if (param != null) error = Integer.parseInt(param); - final AsyncContext asyncContext = this.asyncContext; + AsyncContext asyncContext = this.asyncContext; if (asyncContext != null) { HttpServletResponse asyncResponse = (HttpServletResponse)asyncContext.getResponse(); @@ -153,4 +158,56 @@ protected void doDelete(HttpServletRequest request, HttpServletResponse response assertEquals(200, response3.getStatus()); } } + + @Test + public void testSuspendedRequestThenServerStop() throws Exception + { + AtomicReference asyncContextRef = new AtomicReference<>(); + prepare(new HttpServlet() + { + @Override + protected void service(HttpServletRequest request, HttpServletResponse response) + { + // Suspend the request. + AsyncContext asyncContext = request.startAsync(); + asyncContextRef.set(asyncContext); + } + + @Override + public void destroy() + { + // Try to write an error response when shutting down. + AsyncContext asyncContext = asyncContextRef.get(); + try + { + HttpServletResponse response = (HttpServletResponse)asyncContext.getResponse(); + response.sendError(HttpStatus.INTERNAL_SERVER_ERROR_500); + } + catch (IOException x) + { + throw new RuntimeException(x); + } + finally + { + asyncContext.complete(); + } + } + }); + + try (SocketChannel client = SocketChannel.open(new InetSocketAddress("localhost", connector.getLocalPort()))) + { + HttpTester.Request request = HttpTester.newRequest(); + request.setURI(uri); + client.write(request.generate()); + + await().atMost(5, TimeUnit.SECONDS).until(asyncContextRef::get, Matchers.notNullValue()); + + server.stop(); + + client.socket().setSoTimeout(1000); + // The connection has been closed, no response. + HttpTester.Response response = HttpTester.parseResponse(client); + assertNull(response); + } + } } From 6c839f688f01699a1b2c1b2ba4ca5df92acd5d3b Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Sun, 3 Dec 2023 19:16:04 +0100 Subject: [PATCH 2/7] Restored check of stream.isCommitted() rather than response.isCommitted(). This may run the ErrorHandler twice when using ee10. Signed-off-by: Simone Bordet --- .../org/eclipse/jetty/server/internal/HttpChannelState.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) 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 90077292ef07..24bd38818c17 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 @@ -1459,13 +1459,11 @@ public void failed(Throwable failure) ChannelResponse response = httpChannelState._response; if (LOG.isDebugEnabled()) - { - LOG.debug("failed stream.isCommitted={}, response.isCommitted={} {}", httpChannelState._stream.isCommitted(), response.isCommitted(), this); - } + LOG.debug("failed stream.isCommitted={}, response.isCommitted={} {}", stream.isCommitted(), response.isCommitted(), this); // There may have been an attempt to write an error response that failed. // Do not try to write again an error response if already committed. - if (!response.isCommitted()) + if (!stream.isCommitted()) errorResponse = new ErrorResponse(request); } From efee6f0ffeba6e4b7d615e9d37a6be6fcd9541ed Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Sun, 3 Dec 2023 19:36:53 +0100 Subject: [PATCH 3/7] Fixed dependency to awaitility in jetty-ee8-servlet. Signed-off-by: Simone Bordet --- jetty-ee8/jetty-ee8-servlet/pom.xml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/jetty-ee8/jetty-ee8-servlet/pom.xml b/jetty-ee8/jetty-ee8-servlet/pom.xml index 499fa2451dde..7b9c5234323a 100644 --- a/jetty-ee8/jetty-ee8-servlet/pom.xml +++ b/jetty-ee8/jetty-ee8-servlet/pom.xml @@ -40,6 +40,11 @@ org.slf4j slf4j-api + + org.awaitility + awaitility + test + org.eclipse.jetty jetty-client From d93cba7d4d2023af3af77c17743a8e370c1ebe3e Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Sun, 3 Dec 2023 23:23:56 +0100 Subject: [PATCH 4/7] Improved handling of abort(). Signed-off-by: Simone Bordet --- .../jetty/ee10/servlet/ServletChannel.java | 6 ++--- .../ee10/servlet/ServletChannelState.java | 22 ++++++++++++++++++- 2 files changed, 23 insertions(+), 5 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 be09a570b877..49ca0e3bcba3 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 @@ -813,10 +813,8 @@ protected void execute(Runnable task) */ public void abort(Throwable failure) { - // Callback will failed in onCompleted(). - boolean aborted = _state.abortResponse(failure); - if (LOG.isDebugEnabled()) - LOG.debug("abort={} {}", aborted, this, failure); + // Callback will be failed in onCompleted(). + _state.abort(failure); } private void dispatch() throws Exception diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannelState.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannelState.java index c545b1b2b9de..807fc6ba11b6 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannelState.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannelState.java @@ -316,7 +316,7 @@ public boolean isResponseCompleted() } } - public boolean abortResponse(Throwable failure) + private boolean abortResponse(Throwable failure) { try (AutoLock ignored = lock()) { @@ -340,6 +340,26 @@ public boolean abortResponse(Throwable failure) } } + public void abort(Throwable failure) + { + boolean handle = false; + try (AutoLock ignored = lock()) + { + boolean aborted = abortResponse(failure); + if (LOG.isDebugEnabled()) + LOG.debug("abort={} {}", aborted, this, failure); + if (aborted) + { + handle = _state == State.WAITING; + if (handle) + _state = State.WOKEN; + _requestState = RequestState.COMPLETED; + } + } + if (handle) + scheduleDispatch(); + } + /** * @return Next handling of the request should proceed */ From 586d57a21358c7030cb324caf1977cd0d4ad5730 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Thu, 14 Dec 2023 10:13:06 +0100 Subject: [PATCH 5/7] Updates from review. Signed-off-by: Simone Bordet --- .../jetty/ee10/servlet/ServletChannel.java | 2 +- .../ee10/servlet/ServletChannelState.java | 23 +++++++++++-------- 2 files changed, 15 insertions(+), 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 7925e8327d33..b5185f4396b7 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 @@ -729,7 +729,7 @@ public void onCompleted() _servletContextRequest.setAttribute(CustomRequestLog.LOG_DETAIL, logDetail); } - // Callback will either be succeeded here or failed in abort(). + // Callback is completed here. Callback callback = _callback; Throwable failure = _state.completeResponse(); if (failure == null) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannelState.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannelState.java index 807fc6ba11b6..a1e1f6e81557 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannelState.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannelState.java @@ -326,12 +326,6 @@ private boolean abortResponse(Throwable failure) case ABORTED: return false; - case OPEN: - _servletChannel.getServletContextResponse().setStatus(HttpStatus.INTERNAL_SERVER_ERROR_500); - _outputState = OutputState.ABORTED; - _failure = failure; - return true; - default: _outputState = OutputState.ABORTED; _failure = failure; @@ -566,7 +560,12 @@ public String toString() } } - public void errorHandling() + /** + * Called when an asynchronous call to {@code ErrorHandler.handle()} is about to happen. + * + * @see #errorHandlingComplete(Throwable) + */ + void errorHandling() { try (AutoLock ignored = lock()) { @@ -576,7 +575,13 @@ public void errorHandling() } } - public void errorHandlingComplete(Throwable failure) + /** + * Called when the {@code Callback} passed to {@code ErrorHandler.handle()} is completed. + * + * @param failure the failure reported by the error handling, + * or {@code null} if there was no failure + */ + void errorHandlingComplete(Throwable failure) { boolean handle; try (AutoLock ignored = lock()) @@ -594,7 +599,7 @@ public void errorHandlingComplete(Throwable failure) abortResponse(failure); if (_requestState == RequestState.ERRORING) - _requestState = failure == null ? RequestState.COMPLETE : RequestState.COMPLETED; + _requestState = RequestState.COMPLETE; } if (handle) scheduleDispatch(); From ad46276ae262d496d20a258bea8e6022ffe9a3a8 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Fri, 15 Dec 2023 09:19:18 +0100 Subject: [PATCH 6/7] Updates from review. Signed-off-by: Simone Bordet --- .../java/org/eclipse/jetty/ee10/servlet/ServletChannel.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/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index 3316cf635cbc..a68f2ccce814 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 @@ -736,7 +736,7 @@ public void onCompleted() _servletContextRequest.setAttribute(CustomRequestLog.LOG_DETAIL, logDetail); } - // Callback is completed here. + // Callback is completed only here. Callback callback = _callback; Throwable failure = _state.completeResponse(); if (failure == null) From 80c183c6f6d10f8a3ac98bc7ed5b1899f920e3a7 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Fri, 15 Dec 2023 15:07:08 +0100 Subject: [PATCH 7/7] Updates from review. Signed-off-by: Simone Bordet --- .../org/eclipse/jetty/ee10/servlet/ServletChannelState.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannelState.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannelState.java index 4c99a234da2c..8deffb1280f3 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannelState.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannelState.java @@ -298,8 +298,14 @@ public Throwable completeResponse() { try (AutoLock ignored = lock()) { + // This method is called when the state machine + // is about to terminate the processing, just + // before completing the Handler's callback. + assert _outputState == OutputState.OPEN || _failure != null; + if (_outputState == OutputState.OPEN) _outputState = OutputState.COMPLETED; + return _failure; } }