From eb1e9eb8c444ea01fff3a2f62169134718a4f94d Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Fri, 15 Dec 2023 15:09:20 +0100 Subject: [PATCH] =?UTF-8?q?Fixes=20#11016=20-=20Jetty=2012=20IllegalStateE?= =?UTF-8?q?xception=20when=20stopping=20Server=20wi=E2=80=A6=20(#11017)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 | 5 +- .../jetty/server/internal/HttpConnection.java | 18 ++--- .../jetty/ee10/servlet/ErrorHandler.java | 2 +- .../jetty/ee10/servlet/ServletChannel.java | 40 +++++------ .../ee10/servlet/ServletChannelState.java | 70 ++++++++++++++----- .../servlet/AsyncServletLongPollTest.java | 63 ++++++++++++++++- jetty-ee8/jetty-ee8-servlet/pom.xml | 5 ++ jetty-ee9/jetty-ee9-servlet/pom.xml | 5 ++ .../ee9/servlet/AsyncServletLongPollTest.java | 65 +++++++++++++++-- 9 files changed, 210 insertions(+), 63 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 c1e2031e77c..b75da0e92a6 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 @@ -1501,9 +1501,12 @@ 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={} {}", 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 (!stream.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 2b8f41e8bf1..7035d901d2e 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 @@ -741,26 +741,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 4e05050e400..521b6afa651 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 cc75fea6ac6..a68f2ccce81 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 @@ -470,7 +470,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 { @@ -485,7 +485,7 @@ public void handle() // that ignores existing failures. However, the error handler needs to be able to call servlet pages, // so it will need to do a new call to associate(req,res,callback) or similar, to make the servlet request and // response wrap the error request and response. Have to think about what callback is passed. - errorHandler.handle(getServletContextRequest(), getServletContextResponse(), Callback.from(_state::errorHandlingComplete)); + errorHandler.handle(getServletContextRequest(), getServletContextResponse(), Callback.from(() -> _state.errorHandlingComplete(null), _state::errorHandlingComplete)); } } catch (Throwable x) @@ -495,23 +495,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 @@ -684,7 +677,7 @@ protected Throwable unwrap(Throwable failure, Class... targets) return null; } - public void sendResponseAndComplete() + public void sendErrorResponseAndComplete() { try { @@ -694,6 +687,7 @@ public void sendResponseAndComplete() catch (Throwable x) { abort(x); + _state.completed(x); } } @@ -742,10 +736,13 @@ public void onCompleted() _servletContextRequest.setAttribute(CustomRequestLog.LOG_DETAIL, logDetail); } - // Callback will either be succeeded here or failed in abort(). + // Callback is completed only here. Callback callback = _callback; - if (_state.completeResponse()) + Throwable failure = _state.completeResponse(); + if (failure == null) callback.succeeded(); + else + callback.failed(failure); } public boolean isCommitted() @@ -783,13 +780,8 @@ 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 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 2b46cf99576..8deffb1280f 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; private boolean _failureListener; protected ServletChannelState(ServletChannel servletChannel) @@ -293,19 +294,19 @@ public String getStatusString() } } - public boolean completeResponse() + public Throwable completeResponse() { try (AutoLock ignored = lock()) { - switch (_outputState) - { - case OPEN: - _outputState = OutputState.COMPLETED; - return true; + // 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; - default: - return false; - } + if (_outputState == OutputState.OPEN) + _outputState = OutputState.COMPLETED; + + return _failure; } } @@ -322,7 +323,7 @@ public boolean isResponseCompleted() } } - public boolean abortResponse() + private boolean abortResponse(Throwable failure) { try (AutoLock ignored = lock()) { @@ -332,18 +333,34 @@ public boolean abortResponse() case ABORTED: return false; - case OPEN: - _servletChannel.getServletContextResponse().setStatus(HttpStatus.INTERNAL_SERVER_ERROR_500); - _outputState = OutputState.ABORTED; - return true; - default: _outputState = OutputState.ABORTED; + _failure = failure; return true; } } } + 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 */ @@ -555,7 +572,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()) { @@ -565,17 +587,29 @@ public void errorHandling() } } - public void errorHandlingComplete() + /** + * 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()) { 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; } 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 2bedf4c2312..4129ee58bf0 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-ee8/jetty-ee8-servlet/pom.xml b/jetty-ee8/jetty-ee8-servlet/pom.xml index 5af3c2fdea8..349d71d6baf 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 diff --git a/jetty-ee9/jetty-ee9-servlet/pom.xml b/jetty-ee9/jetty-ee9-servlet/pom.xml index bb7a5f736a2..1bc62b75745 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 4ffc95034f3..9a5db257a2b 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); + } + } }