From bde86467f4e5df595773ab11ed5e80c615b741f3 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 26 Aug 2019 17:55:58 +1000 Subject: [PATCH] Issue #3806 - Make Async sendError fully Async (#3912) * Issue #3806 async sendError Avoid using isHandled as a test withing sendError as this can be called asynchronously and is in a race with the normal dispatch of the request, which could also be setting handled status. The ErrorHandler was dispatching directly to a context from within sendError. This meant that an async thread can call sendError and be dispatched to within the servlet container at the same time that the original thread was still dispatched to the container. This commit fixes that problem by using an async dispatch for error pages within the ErrorHandler. However, this introduces a new problem that a well behaved async app will call complete after calling sendError. Thus we have ignore complete ISEs for the remainder of the current async cycle. Fixed the closing of the output after calling sendError. Do not close if the request was async (and thus might be dispatched to an async error) or if it is now async because the error page itself is async. * updates from review * better tests * revert ignore complete * added some TODOs * more TODOs * fixed rename * cleanup ISE and more TODOs * refactored to call sendError for uncaught exceptions rather than onError * more of the refactor * extra tests for sendError from completing state Reworked HttpChannelState and sendError so that sendError is now just a change of state. All the work is done in the ErrorDispatch action, including calling the ErrorHandler. Async not yet working. Additional tests Converted ERRORED state to a separate boolean so it can be used for both Sync and Async dispatches. Removed ASYNC_IO state as it was just the same as DISPATCHED The async onError listener handling is now most likely broken. WIP making sendError simpler and more tests pass WIP handling async and thrown exceptions WIP passing tests Improved thread handling removed bad test Implemented error dispatch on complete properly more fixed tests sendError state looks committed - Added resetContent method to leave more non-content headers during sendError - Fixed security tests - simplified the non dispatch error page writing. Moved towards being able to write async * fixed gzipHandlerTest * Updated handling of timeout errors. According to servlet spec, exceptions thrown from onTimeout should not be passed to onError, but just logged and ignored: If an exception is thrown while invoking methods in an AsyncListener, it is logged and will not affect the invocation of any other AsyncListeners. * This changes several tests. * Dispatcher/ContextHandler changes for new ERROR dispatch handling. Feels a bit fragile! * Fixed tests in jetty-servlets * Fixed tests in jetty-proxy * more test fixes * Fixed head handling reverted unnecessary changes Improved reason handling WIP on fully async error handling. Simplified HttpChannelState state machines to allow for async actions during completing more WIP on fully async error handling. sendError and completion are not both non-blocking, without using a startAsync operation. However we are lacking unit tests that actually exercise those code paths. * Simplified name of states Added test for async completion * Cleanups and javadoc * Cleanups and javadoc * remove snake case * feedback from review * Write error page into fixed pooled buffer Use the response to get/release a pooled buffer into which the error page can be written. Make it a fixed sized buffer and if it overflows then no error page is generated (first overflow turns off showstacks to save space). The ErrorHandler badly needs to be refactored, but we cannot change API in jetty-9 * More test fixes for different error page format * minor cleanups * Cleanup from Review * Fixed javadoc * cleanups and simplifications * Cleanup from Review * renaming and some TODOs * Cleanup from Review * Checkstyle fixes * Cleanup from Review * Code cleanups and simplifications * fixed debug * Cleanup from Review * Ensure response sent before server shutdown * removed unnecessary optimisation * fixed duplicate from merge * Updates from review Signed-off-by: Greg Wilkins --- .../org/eclipse/jetty/http/HttpStatus.java | 14 + .../jetty/io/ByteBufferOutputStream.java | 62 + .../jetty/proxy/AbstractProxyServlet.java | 8 + .../rewrite/handler/ResponsePatternRule.java | 15 +- .../jetty/rewrite/handler/ValidUrlRule.java | 10 +- .../handler/ResponsePatternRuleTest.java | 8 +- .../security/SpecExampleConstraintTest.java | 4 +- .../SpnegoAuthenticatorTest.java | 44 +- .../jetty/server/AsyncContextEvent.java | 2 +- .../org/eclipse/jetty/server/Dispatcher.java | 12 +- .../org/eclipse/jetty/server/HttpChannel.java | 308 ++-- .../jetty/server/HttpChannelState.java | 1350 +++++++++-------- .../eclipse/jetty/server/HttpConnection.java | 14 +- .../org/eclipse/jetty/server/HttpInput.java | 3 +- .../org/eclipse/jetty/server/HttpOutput.java | 265 ++-- .../org/eclipse/jetty/server/Request.java | 10 +- .../org/eclipse/jetty/server/Response.java | 189 +-- .../java/org/eclipse/jetty/server/Server.java | 12 +- .../jetty/server/handler/ContextHandler.java | 31 +- .../jetty/server/handler/ErrorHandler.java | 316 ++-- .../jetty/server/handler/ShutdownHandler.java | 3 +- .../jetty/server/AbstractHttpTest.java | 12 +- .../jetty/server/AsyncCompletionTest.java | 221 +++ .../jetty/server/ErrorHandlerTest.java | 38 - .../jetty/server/HttpInputAsyncStateTest.java | 4 + ...ManyWaysToAsyncCommitBadBehaviourTest.java | 133 -- .../server/HttpManyWaysToAsyncCommitTest.java | 776 ++++++---- .../jetty/server/HttpServerTestBase.java | 106 +- .../jetty/server/HttpServerTestFixture.java | 28 +- .../jetty/server/LocalAsyncContextTest.java | 5 + .../eclipse/jetty/server/ResponseTest.java | 114 +- .../server/handler/NcsaRequestLogTest.java | 25 +- .../handler/SecuredRedirectHandlerTest.java | 1 + .../ssl/SniSslConnectionFactoryTest.java | 69 +- .../jetty/servlet/AsyncContextTest.java | 5 +- .../jetty/servlet/AsyncListenerTest.java | 17 +- .../jetty/servlet/AsyncServletIOTest.java | 8 +- .../jetty/servlet/AsyncServletTest.java | 43 +- .../jetty/servlet/CustomRequestLogTest.java | 3 +- .../eclipse/jetty/servlet/ErrorPageTest.java | 443 +++++- .../jetty/servlet/GzipHandlerTest.java | 5 +- .../org/eclipse/jetty/servlets/DoSFilter.java | 11 +- .../org/eclipse/jetty/util/BufferUtil.java | 13 +- .../jetty/util/thread/QueuedThreadPool.java | 3 +- .../tests/WebSocketConnectionStatsTest.java | 2 +- .../tests/WebSocketNegotiationTest.java | 2 +- .../server/WebSocketInvalidVersionTest.java | 2 +- .../jetty/tests/distribution/BadAppTests.java | 8 +- 48 files changed, 2893 insertions(+), 1884 deletions(-) create mode 100644 jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferOutputStream.java create mode 100644 jetty-server/src/test/java/org/eclipse/jetty/server/AsyncCompletionTest.java delete mode 100644 jetty-server/src/test/java/org/eclipse/jetty/server/HttpManyWaysToAsyncCommitBadBehaviourTest.java diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpStatus.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpStatus.java index 887bb636259..20e6f8cf431 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpStatus.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpStatus.java @@ -320,6 +320,20 @@ public class HttpStatus } } + public static boolean hasNoBody(int status) + { + switch (status) + { + case NO_CONTENT_204: + case NOT_MODIFIED_304: + case PARTIAL_CONTENT_206: + return true; + + default: + return status < OK_200; + } + } + /** * Simple test against an code to determine if it falls into the * Informational message category as defined in the = 400) { - response.sendError(code, _reason); + if (!StringUtil.isBlank(_reason)) + { + // use both setStatusWithReason (to set the reason) and sendError to set the message + Request.getBaseRequest(request).getResponse().setStatusWithReason(code, _reason); + response.sendError(code, _reason); + } + else + { + response.sendError(code); + } } else { - response.setStatus(code); + response.setStatus(code, _reason); } return target; } diff --git a/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/ValidUrlRule.java b/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/ValidUrlRule.java index 2ac00d233d2..1e3207bb7dc 100644 --- a/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/ValidUrlRule.java +++ b/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/ValidUrlRule.java @@ -22,6 +22,8 @@ import java.io.IOException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import org.eclipse.jetty.server.Request; +import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.URIUtil; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; @@ -88,7 +90,13 @@ public class ValidUrlRule extends Rule // status code 400 and up are error codes so include a reason if (code >= 400) { - response.sendError(code, _reason); + if (StringUtil.isBlank(_reason)) + response.sendError(code); + else + { + Request.getBaseRequest(request).getResponse().setStatusWithReason(code, _reason); + response.sendError(code, _reason); + } } else { diff --git a/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ResponsePatternRuleTest.java b/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ResponsePatternRuleTest.java index aeca52412ed..fd6c1290b9b 100644 --- a/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ResponsePatternRuleTest.java +++ b/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ResponsePatternRuleTest.java @@ -20,6 +20,8 @@ package org.eclipse.jetty.rewrite.handler; import java.io.IOException; +import org.eclipse.jetty.http.HttpStatus; +import org.eclipse.jetty.server.Dispatcher; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -59,7 +61,7 @@ public class ResponsePatternRuleTest extends AbstractRuleTestCase _rule.apply(null, _request, _response); assertEquals(i, _response.getStatus()); - assertEquals(null, _response.getReason()); + assertEquals("reason" + i, _response.getReason()); } } @@ -72,7 +74,7 @@ public class ResponsePatternRuleTest extends AbstractRuleTestCase _rule.apply(null, _request, _response); assertEquals(i, _response.getStatus()); - assertEquals("", _response.getReason()); + assertEquals(HttpStatus.getMessage(i), _request.getAttribute(Dispatcher.ERROR_MESSAGE)); super.reset(); } } @@ -87,7 +89,7 @@ public class ResponsePatternRuleTest extends AbstractRuleTestCase _rule.apply(null, _request, _response); assertEquals(i, _response.getStatus()); - assertEquals("reason-" + i, _response.getReason()); + assertEquals("reason-" + i, _request.getAttribute(Dispatcher.ERROR_MESSAGE)); super.reset(); } } diff --git a/jetty-security/src/test/java/org/eclipse/jetty/security/SpecExampleConstraintTest.java b/jetty-security/src/test/java/org/eclipse/jetty/security/SpecExampleConstraintTest.java index 2f8d243071c..cd0d46a7a98 100644 --- a/jetty-security/src/test/java/org/eclipse/jetty/security/SpecExampleConstraintTest.java +++ b/jetty-security/src/test/java/org/eclipse/jetty/security/SpecExampleConstraintTest.java @@ -44,6 +44,7 @@ import org.junit.jupiter.api.Test; import static java.nio.charset.StandardCharsets.ISO_8859_1; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.startsWith; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -321,7 +322,8 @@ public class SpecExampleConstraintTest response = _connector.getResponse("POST /ctx/acme/wholesale/index.html HTTP/1.0\r\n" + "Authorization: Basic " + encodedChris + "\r\n" + "\r\n"); - assertThat(response, startsWith("HTTP/1.1 403 !")); + assertThat(response, startsWith("HTTP/1.1 403 Forbidden")); + assertThat(response, containsString("!Secure")); //a user in role HOMEOWNER can do a GET response = _connector.getResponse("GET /ctx/acme/retail/index.html HTTP/1.0\r\n" + diff --git a/jetty-security/src/test/java/org/eclipse/jetty/security/authentication/SpnegoAuthenticatorTest.java b/jetty-security/src/test/java/org/eclipse/jetty/security/authentication/SpnegoAuthenticatorTest.java index 3e771934fa9..00af87e7ecc 100644 --- a/jetty-security/src/test/java/org/eclipse/jetty/security/authentication/SpnegoAuthenticatorTest.java +++ b/jetty-security/src/test/java/org/eclipse/jetty/security/authentication/SpnegoAuthenticatorTest.java @@ -18,6 +18,7 @@ package org.eclipse.jetty.security.authentication; +import java.io.IOException; import javax.servlet.http.HttpServletResponse; import org.eclipse.jetty.http.HttpFields; @@ -26,6 +27,7 @@ import org.eclipse.jetty.http.HttpURI; import org.eclipse.jetty.http.MetaData; import org.eclipse.jetty.server.Authentication; import org.eclipse.jetty.server.HttpChannel; +import org.eclipse.jetty.server.HttpChannelState; import org.eclipse.jetty.server.HttpConfiguration; import org.eclipse.jetty.server.HttpOutput; import org.eclipse.jetty.server.Request; @@ -34,6 +36,8 @@ import org.eclipse.jetty.server.Server; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -61,21 +65,27 @@ public class SpnegoAuthenticatorTest { return null; } - }; - Request req = new Request(channel, null); - HttpOutput out = new HttpOutput(channel) - { + @Override - public void close() + protected HttpOutput newHttpOutput() { - return; + return new HttpOutput(this) + { + @Override + public void close() {} + + @Override + public void flush() throws IOException {} + }; } }; - Response res = new Response(channel, out); + Request req = channel.getRequest(); + Response res = channel.getResponse(); MetaData.Request metadata = new MetaData.Request(new HttpFields()); metadata.setURI(new HttpURI("http://localhost")); req.setMetaData(metadata); + assertThat(channel.getState().handling(), is(HttpChannelState.Action.DISPATCH)); assertEquals(Authentication.SEND_CONTINUE, _authenticator.validateRequest(req, res, true)); assertEquals(HttpHeader.NEGOTIATE.asString(), res.getHeader(HttpHeader.WWW_AUTHENTICATE.asString())); assertEquals(HttpServletResponse.SC_UNAUTHORIZED, res.getStatus()); @@ -91,17 +101,22 @@ public class SpnegoAuthenticatorTest { return null; } - }; - Request req = new Request(channel, null); - HttpOutput out = new HttpOutput(channel) - { + @Override - public void close() + protected HttpOutput newHttpOutput() { - return; + return new HttpOutput(this) + { + @Override + public void close() {} + + @Override + public void flush() throws IOException {} + }; } }; - Response res = new Response(channel, out); + Request req = channel.getRequest(); + Response res = channel.getResponse(); HttpFields http_fields = new HttpFields(); // Create a bogus Authorization header. We don't care about the actual credentials. http_fields.add(HttpHeader.AUTHORIZATION, "Basic asdf"); @@ -109,6 +124,7 @@ public class SpnegoAuthenticatorTest metadata.setURI(new HttpURI("http://localhost")); req.setMetaData(metadata); + assertThat(channel.getState().handling(), is(HttpChannelState.Action.DISPATCH)); assertEquals(Authentication.SEND_CONTINUE, _authenticator.validateRequest(req, res, true)); assertEquals(HttpHeader.NEGOTIATE.asString(), res.getHeader(HttpHeader.WWW_AUTHENTICATE.asString())); assertEquals(HttpServletResponse.SC_UNAUTHORIZED, res.getStatus()); diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContextEvent.java b/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContextEvent.java index 0cd93ff67be..52520ffd71f 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContextEvent.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContextEvent.java @@ -160,7 +160,7 @@ public class AsyncContextEvent extends AsyncEvent implements Runnable Scheduler.Task task = _timeoutTask; _timeoutTask = null; if (task != null) - _state.getHttpChannel().execute(() -> _state.onTimeout()); + _state.timeout(); } public void addThrowable(Throwable e) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Dispatcher.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Dispatcher.java index 840335af2af..09249f955ab 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Dispatcher.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Dispatcher.java @@ -42,8 +42,6 @@ public class Dispatcher implements RequestDispatcher { private static final Logger LOG = Log.getLogger(Dispatcher.class); - public static final String __ERROR_DISPATCH = "org.eclipse.jetty.server.Dispatcher.ERROR"; - /** * Dispatch include attribute names */ @@ -83,15 +81,7 @@ public class Dispatcher implements RequestDispatcher public void error(ServletRequest request, ServletResponse response) throws ServletException, IOException { - try - { - request.setAttribute(__ERROR_DISPATCH, Boolean.TRUE); - forward(request, response, DispatcherType.ERROR); - } - finally - { - request.setAttribute(__ERROR_DISPATCH, null); - } + forward(request, response, DispatcherType.ERROR); } @Override diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java index bd2ad60a63b..31714973255 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java @@ -25,7 +25,6 @@ import java.util.ArrayList; import java.util.List; import java.util.concurrent.Executor; import java.util.concurrent.TimeoutException; -import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; import java.util.function.BiConsumer; import java.util.function.Consumer; @@ -33,12 +32,12 @@ import java.util.function.Function; import java.util.function.Supplier; import javax.servlet.DispatcherType; import javax.servlet.RequestDispatcher; +import javax.servlet.ServletException; import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.HttpFields; import org.eclipse.jetty.http.HttpGenerator; import org.eclipse.jetty.http.HttpHeader; -import org.eclipse.jetty.http.HttpMethod; import org.eclipse.jetty.http.HttpScheme; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpVersion; @@ -51,6 +50,7 @@ import org.eclipse.jetty.io.QuietException; import org.eclipse.jetty.server.HttpChannelState.Action; import org.eclipse.jetty.server.handler.ContextHandler; import org.eclipse.jetty.server.handler.ErrorHandler; +import org.eclipse.jetty.server.handler.ErrorHandler.ErrorPageMapper; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.SharedBlockingCallback.Blocker; @@ -71,8 +71,6 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor { private static final Logger LOG = Log.getLogger(HttpChannel.class); - private final AtomicBoolean _committed = new AtomicBoolean(); - private final AtomicBoolean _responseCompleted = new AtomicBoolean(); private final AtomicLong _requests = new AtomicLong(); private final Connector _connector; private final Executor _executor; @@ -121,6 +119,11 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor _state); } + public boolean isSendError() + { + return _state.isSendError(); + } + protected HttpInput newHttpInput(HttpChannelState state) { return new HttpInput(state); @@ -284,8 +287,6 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor public void recycle() { - _committed.set(false); - _responseCompleted.set(false); _request.recycle(); _response.recycle(); _committedMetaData = null; @@ -320,7 +321,7 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor public boolean handle() { if (LOG.isDebugEnabled()) - LOG.debug("{} handle {} ", this, _request.getHttpURI()); + LOG.debug("handle {} {} ", _request.getHttpURI(), this); HttpChannelState.Action action = _state.handling(); @@ -334,19 +335,18 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor try { if (LOG.isDebugEnabled()) - LOG.debug("{} action {}", this, action); + LOG.debug("action {} {}", action, this); switch (action) { case TERMINATED: + onCompleted(); + break loop; + case WAIT: // break loop without calling unhandle break loop; - case NOOP: - // do nothing other than call unhandle - break; - case DISPATCH: { if (!_request.hasMetaData()) @@ -354,35 +354,17 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor _request.setHandled(false); _response.getHttpOutput().reopen(); - try + dispatch(DispatcherType.REQUEST, () -> { - _request.setDispatcherType(DispatcherType.REQUEST); - notifyBeforeDispatch(_request); - - List customizers = _configuration.getCustomizers(); - if (!customizers.isEmpty()) + for (HttpConfiguration.Customizer customizer : _configuration.getCustomizers()) { - for (HttpConfiguration.Customizer customizer : customizers) - { - customizer.customize(getConnector(), _configuration, _request); - if (_request.isHandled()) - break; - } + customizer.customize(getConnector(), _configuration, _request); + if (_request.isHandled()) + return; } + getServer().handle(HttpChannel.this); + }); - if (!_request.isHandled()) - getServer().handle(this); - } - catch (Throwable x) - { - notifyDispatchFailure(_request, x); - throw x; - } - finally - { - notifyAfterDispatch(_request); - _request.setDispatcherType(null); - } break; } @@ -391,70 +373,70 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor _request.setHandled(false); _response.getHttpOutput().reopen(); - try - { - _request.setDispatcherType(DispatcherType.ASYNC); - notifyBeforeDispatch(_request); - getServer().handleAsync(this); - } - catch (Throwable x) - { - notifyDispatchFailure(_request, x); - throw x; - } - finally - { - notifyAfterDispatch(_request); - _request.setDispatcherType(null); - } + dispatch(DispatcherType.ASYNC,() -> getServer().handleAsync(this)); break; } - case ERROR_DISPATCH: + case ASYNC_TIMEOUT: + _state.onTimeout(); + break; + + case SEND_ERROR: { try { - _response.reset(true); - Integer icode = (Integer)_request.getAttribute(RequestDispatcher.ERROR_STATUS_CODE); - int code = icode != null ? icode : HttpStatus.INTERNAL_SERVER_ERROR_500; - _response.setStatus(code); - _request.setAttribute(RequestDispatcher.ERROR_STATUS_CODE, code); + // Get ready to send an error response _request.setHandled(false); + _response.resetContent(); _response.getHttpOutput().reopen(); - try + // the following is needed as you cannot trust the response code and reason + // as those could have been modified after calling sendError + Integer code = (Integer)_request.getAttribute(RequestDispatcher.ERROR_STATUS_CODE); + _response.setStatus(code != null ? code : HttpStatus.INTERNAL_SERVER_ERROR_500); + + ContextHandler.Context context = (ContextHandler.Context)_request.getAttribute(ErrorHandler.ERROR_CONTEXT); + ErrorHandler errorHandler = ErrorHandler.getErrorHandler(getServer(), context == null ? null : context.getContextHandler()); + + // If we can't have a body, then create a minimal error response. + if (HttpStatus.hasNoBody(_response.getStatus()) || errorHandler == null || !errorHandler.errorPageForMethod(_request.getMethod())) { - _request.setDispatcherType(DispatcherType.ERROR); - notifyBeforeDispatch(_request); - getServer().handle(this); + sendResponseAndComplete(); + break; } - catch (Throwable x) + + // Look for an error page dispatcher + String errorPage = (errorHandler instanceof ErrorPageMapper) ? ((ErrorPageMapper)errorHandler).getErrorPage(_request) : null; + Dispatcher errorDispatcher = errorPage != null ? (Dispatcher)context.getRequestDispatcher(errorPage) : null; + if (errorDispatcher == null) { - notifyDispatchFailure(_request, x); - throw x; + // Allow ErrorHandler to generate response + errorHandler.handle(null, _request, _request, _response); + _request.setHandled(true); } - finally + else { - notifyAfterDispatch(_request); - _request.setDispatcherType(null); + // Do the error page dispatch + dispatch(DispatcherType.ERROR,() -> errorDispatcher.error(_request, _response)); } } catch (Throwable x) { if (LOG.isDebugEnabled()) LOG.debug("Could not perform ERROR dispatch, aborting", x); - Throwable failure = (Throwable)_request.getAttribute(RequestDispatcher.ERROR_EXCEPTION); - if (failure == null) - { - minimalErrorResponse(x); - } + if (_state.isResponseCommitted()) + abort(x); else { - if (x != failure) - failure.addSuppressed(x); - minimalErrorResponse(failure); + _response.resetContent(); + sendResponseAndComplete(); } } + finally + { + // clean up the context that was set in Response.sendError + _request.removeAttribute(ErrorHandler.ERROR_CONTEXT); + } break; } @@ -463,6 +445,12 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor throw _state.getAsyncContextEvent().getThrowable(); } + case READ_REGISTER: + { + onAsyncWaitForContent(); + break; + } + case READ_PRODUCE: { _request.getHttpInput().asyncReadProduce(); @@ -491,45 +479,37 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor case COMPLETE: { - try + if (!_response.isCommitted() && !_request.isHandled() && !_response.getHttpOutput().isClosed()) { - if (!_response.isCommitted() && !_request.isHandled()) - { - _response.sendError(HttpStatus.NOT_FOUND_404); - } - else - { - // RFC 7230, section 3.3. - int status = _response.getStatus(); - boolean hasContent = !(_request.isHead() || - HttpMethod.CONNECT.is(_request.getMethod()) && status == HttpStatus.OK_200 || - HttpStatus.isInformational(status) || - status == HttpStatus.NO_CONTENT_204 || - status == HttpStatus.NOT_MODIFIED_304); - if (hasContent && !_response.isContentComplete(_response.getHttpOutput().getWritten())) - { - if (isCommitted()) - abort(new IOException("insufficient content written")); - else - _response.sendError(HttpStatus.INTERNAL_SERVER_ERROR_500, "insufficient content written"); - } - } - _response.closeOutput(); - } - finally - { - _request.setHandled(true); - _state.onComplete(); - onCompleted(); + _response.sendError(HttpStatus.NOT_FOUND_404); + break; } - break loop; + // RFC 7230, section 3.3. + if (!_request.isHead() && !_response.isContentComplete(_response.getHttpOutput().getWritten())) + { + if (isCommitted()) + abort(new IOException("insufficient content written")); + else + { + _response.sendError(HttpStatus.INTERNAL_SERVER_ERROR_500, "insufficient content written"); + break; + } + } + + // TODO Currently a blocking/aborting consumeAll is done in the handling of the TERMINATED + // TODO Action triggered by the completed callback below. It would be possible to modify the + // TODO callback to do a non-blocking consumeAll at this point and only call completed when + // TODO that is done. + + // Set a close callback on the HttpOutput to make it an async callback + _response.closeOutput(Callback.from(_state::completed)); + + break; } default: - { - throw new IllegalStateException("state=" + _state); - } + throw new IllegalStateException(this.toString()); } } catch (Throwable failure) @@ -544,26 +524,29 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor } if (LOG.isDebugEnabled()) - LOG.debug("{} handle exit, result {}", this, action); + LOG.debug("!handle {} {}", action, this); boolean suspended = action == Action.WAIT; return !suspended; } - protected void sendError(int code, String reason) + private void dispatch(DispatcherType type, Dispatchable dispatchable) throws IOException, ServletException { try { - _response.sendError(code, reason); + _request.setDispatcherType(type); + notifyBeforeDispatch(_request); + dispatchable.dispatch(); } catch (Throwable x) { - if (LOG.isDebugEnabled()) - LOG.debug("Could not send error " + code + " " + reason, x); + notifyDispatchFailure(_request, x); + throw x; } finally { - _state.errorComplete(); + notifyAfterDispatch(_request); + _request.setDispatcherType(null); } } @@ -591,27 +574,19 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor { // No stack trace unless there is debug turned on if (LOG.isDebugEnabled()) - LOG.debug(_request.getRequestURI(), failure); + LOG.warn("handleException " + _request.getRequestURI(), failure); else - LOG.warn("{} {}", _request.getRequestURI(), noStack.toString()); + LOG.warn("handleException {} {}", _request.getRequestURI(), noStack.toString()); } else { LOG.warn(_request.getRequestURI(), failure); } - try - { + if (isCommitted()) + abort(failure); + else _state.onError(failure); - } - catch (Throwable e) - { - if (e != failure) - failure.addSuppressed(e); - LOG.warn("ERROR dispatch failed", failure); - // Try to send a minimal response. - minimalErrorResponse(failure); - } } /** @@ -635,30 +610,17 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor return null; } - private void minimalErrorResponse(Throwable failure) + public void sendResponseAndComplete() { try { - int code = 500; - Integer status = (Integer)_request.getAttribute(RequestDispatcher.ERROR_STATUS_CODE); - if (status != null) - code = status.intValue(); - else - { - Throwable cause = unwrap(failure, BadMessageException.class); - if (cause instanceof BadMessageException) - code = ((BadMessageException)cause).getCode(); - } - - _response.reset(true); - _response.setStatus(code); - _response.flushBuffer(); + _request.setHandled(true); + _state.completing(); + sendResponse(null, _response.getHttpOutput().getBuffer(), true, Callback.from(_state::completed)); } catch (Throwable x) { - if (x != failure) - failure.addSuppressed(x); - abort(failure); + abort(x); } } @@ -676,11 +638,11 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor public String toString() { long timeStamp = _request.getTimeStamp(); - return String.format("%s@%x{r=%s,c=%b,c=%b/%b,a=%s,uri=%s,age=%d}", + return String.format("%s@%x{s=%s,r=%s,c=%b/%b,a=%s,uri=%s,age=%d}", getClass().getSimpleName(), hashCode(), + _state, _requests, - _committed.get(), isRequestCompleted(), isResponseCompleted(), _state.getState(), @@ -717,7 +679,7 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor public boolean onContent(HttpInput.Content content) { if (LOG.isDebugEnabled()) - LOG.debug("{} onContent {}", this, content); + LOG.debug("onContent {} {}", this, content); notifyRequestContent(_request, content.getByteBuffer()); return _request.getHttpInput().addContent(content); } @@ -725,7 +687,7 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor public boolean onContentComplete() { if (LOG.isDebugEnabled()) - LOG.debug("{} onContentComplete", this); + LOG.debug("onContentComplete {}", this); notifyRequestContentEnd(_request); return false; } @@ -733,7 +695,7 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor public void onTrailers(HttpFields trailers) { if (LOG.isDebugEnabled()) - LOG.debug("{} onTrailers {}", this, trailers); + LOG.debug("onTrailers {} {}", this, trailers); _trailers = trailers; notifyRequestTrailers(_request); } @@ -741,7 +703,7 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor public boolean onRequestComplete() { if (LOG.isDebugEnabled()) - LOG.debug("{} onRequestComplete", this); + LOG.debug("onRequestComplete {}", this); boolean result = _request.getHttpInput().eof(); notifyRequestEnd(_request); return result; @@ -750,7 +712,7 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor public void onCompleted() { if (LOG.isDebugEnabled()) - LOG.debug("COMPLETE for {} written={}", getRequest().getRequestURI(), getBytesWritten()); + LOG.debug("onCompleted for {} written={}", getRequest().getRequestURI(), getBytesWritten()); if (_requestLog != null) _requestLog.log(_request, _response); @@ -773,7 +735,7 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor { int status = failure.getCode(); String reason = failure.getReason(); - if (status < 400 || status > 599) + if (status < HttpStatus.BAD_REQUEST_400 || status > 599) failure = new BadMessageException(HttpStatus.BAD_REQUEST_400, reason, failure); notifyRequestFailure(_request, failure); @@ -823,9 +785,9 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor } } - protected boolean sendResponse(MetaData.Response info, ByteBuffer content, boolean complete, final Callback callback) + public boolean sendResponse(MetaData.Response info, ByteBuffer content, boolean complete, final Callback callback) { - boolean committing = _committed.compareAndSet(false, true); + boolean committing = _state.commitResponse(); if (LOG.isDebugEnabled()) LOG.debug("sendResponse info={} content={} complete={} committing={} callback={}", @@ -844,7 +806,9 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor // wrap callback to process 100 responses final int status = info.getStatus(); - final Callback committed = (status < 200 && status >= 100) ? new Send100Callback(callback) : new SendCallback(callback, content, true, complete); + final Callback committed = (status < HttpStatus.OK_200 && status >= HttpStatus.CONTINUE_100) + ? new Send100Callback(callback) + : new SendCallback(callback, content, true, complete); notifyResponseBegin(_request); @@ -891,7 +855,7 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor public boolean isCommitted() { - return _committed.get(); + return _state.isResponseCommitted(); } /** @@ -907,7 +871,7 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor */ public boolean isResponseCompleted() { - return _responseCompleted.get(); + return _state.isResponseCompleted(); } public boolean isPersistent() @@ -970,8 +934,11 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor */ public void abort(Throwable failure) { - notifyResponseFailure(_request, failure); - _transport.abort(failure); + if (_state.abortResponse()) + { + notifyResponseFailure(_request, failure); + _transport.abort(failure); + } } private void notifyRequestBegin(Request request) @@ -1095,6 +1062,11 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor } } + interface Dispatchable + { + void dispatch() throws IOException, ServletException; + } + /** *

Listener for {@link HttpChannel} events.

*

HttpChannel will emit events for the various phases it goes through while @@ -1280,16 +1252,15 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor public void succeeded() { _written += _length; + if (_complete) + _response.getHttpOutput().closed(); super.succeeded(); if (_commit) notifyResponseCommit(_request); if (_length > 0) notifyResponseContent(_request, _content); - if (_complete) - { - _responseCompleted.set(true); + if (_complete && _state.completeResponse()) notifyResponseEnd(_request); - } } @Override @@ -1305,13 +1276,14 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor @Override public void succeeded() { - super.failed(x); _response.getHttpOutput().closed(); + super.failed(x); } @Override public void failed(Throwable th) { + _response.getHttpOutput().closed(); abort(x); super.failed(x); } @@ -1335,7 +1307,7 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor @Override public void succeeded() { - if (_committed.compareAndSet(true, false)) + if (_state.partialResponse()) super.succeeded(); else super.failed(new IllegalStateException()); diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelState.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelState.java index 0b8029ab10f..d75eb7a7e5b 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelState.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelState.java @@ -21,9 +21,7 @@ package org.eclipse.jetty.server; import java.util.ArrayList; import java.util.List; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicReference; import javax.servlet.AsyncListener; -import javax.servlet.RequestDispatcher; import javax.servlet.ServletContext; import javax.servlet.ServletResponse; import javax.servlet.UnavailableException; @@ -32,14 +30,16 @@ import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.server.handler.ContextHandler; import org.eclipse.jetty.server.handler.ContextHandler.Context; +import org.eclipse.jetty.server.handler.ErrorHandler; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; -import org.eclipse.jetty.util.thread.Locker; import org.eclipse.jetty.util.thread.Scheduler; import static javax.servlet.RequestDispatcher.ERROR_EXCEPTION; import static javax.servlet.RequestDispatcher.ERROR_EXCEPTION_TYPE; import static javax.servlet.RequestDispatcher.ERROR_MESSAGE; +import static javax.servlet.RequestDispatcher.ERROR_REQUEST_URI; +import static javax.servlet.RequestDispatcher.ERROR_SERVLET_NAME; import static javax.servlet.RequestDispatcher.ERROR_STATUS_CODE; /** @@ -51,21 +51,77 @@ public class HttpChannelState private static final long DEFAULT_TIMEOUT = Long.getLong("org.eclipse.jetty.server.HttpChannelState.DEFAULT_TIMEOUT", 30000L); - /** + /* * The state of the HttpChannel,used to control the overall lifecycle. + * 

+     *     IDLE <-----> HANDLING ----> WAITING
+     *       |                 ^       /
+     *       |                  \     /
+     *       v                   \   v
+     *    UPGRADED               WOKEN
+     *
*/ public enum State { - IDLE, // Idle request - DISPATCHED, // Request dispatched to filter/servlet - THROWN, // Exception thrown while DISPATCHED - ASYNC_WAIT, // Suspended and waiting - ASYNC_WOKEN, // Dispatch to handle from ASYNC_WAIT - ASYNC_IO, // Dispatched for async IO - ASYNC_ERROR, // Async error from ASYNC_WAIT - COMPLETING, // Response is completable - COMPLETED, // Response is completed - UPGRADED // Request upgraded the connection + IDLE, // Idle request + HANDLING, // Request dispatched to filter/servlet or Async IO callback + WAITING, // Suspended and waiting + WOKEN, // Dispatch to handle from ASYNC_WAIT + UPGRADED // Request upgraded the connection + } + + /* + * The state of the request processing lifecycle. + * 
+     *       BLOCKING <----> COMPLETING ---> COMPLETED
+     *       ^  |  ^            ^
+     *      /   |   \           |
+     *     |    |    DISPATCH   |
+     *     |    |    ^  ^       |
+     *     |    v   /   |       |
+     *     |  ASYNC -------> COMPLETE
+     *     |    |       |       ^
+     *     |    v       |       |
+     *     |  EXPIRE    |       |
+     *      \   |      /        |
+     *       \  v     /         |
+     *       EXPIRING ----------+
+     *
+ */ + private enum RequestState + { + BLOCKING, // Blocking request dispatched + ASYNC, // AsyncContext.startAsync() has been called + DISPATCH, // AsyncContext.dispatch() has been called + EXPIRE, // AsyncContext timeout has happened + EXPIRING, // AsyncListeners are being called + COMPLETE, // AsyncContext.complete() has been called + COMPLETING, // Request is being closed (maybe asynchronously) + COMPLETED // Response is completed + } + + /* + * The input readiness state, which works together with {@link HttpInput.State} + */ + private enum InputState + { + IDLE, // No isReady; No data + REGISTER, // isReady()==false handling; No data + REGISTERED, // isReady()==false !handling; No data + POSSIBLE, // isReady()==false async read callback called (http/1 only) + PRODUCING, // isReady()==false READ_PRODUCE action is being handled (http/1 only) + READY // isReady() was false, onContentAdded has been called + } + + /* + * The output committed state, which works together with {@link HttpOutput.State} + */ + private enum OutputState + { + OPEN, + COMMITTED, + COMPLETED, + ABORTED, } /** @@ -73,51 +129,28 @@ public class HttpChannelState */ public enum Action { - NOOP, // No action DISPATCH, // handle a normal request dispatch ASYNC_DISPATCH, // handle an async request dispatch - ERROR_DISPATCH, // handle a normal error + SEND_ERROR, // Generate an error page or error dispatch ASYNC_ERROR, // handle an async error + ASYNC_TIMEOUT, // call asyncContext onTimeout WRITE_CALLBACK, // handle an IO write callback + READ_REGISTER, // Register for fill interest READ_PRODUCE, // Check is a read is possible by parsing/filling READ_CALLBACK, // handle an IO read callback - COMPLETE, // Complete the response + COMPLETE, // Complete the response by closing output TERMINATED, // No further actions WAIT, // Wait for further events } - /** - * The state of the servlet async API. - */ - private enum Async - { - NOT_ASYNC, - STARTED, // AsyncContext.startAsync() has been called - DISPATCH, // AsyncContext.dispatch() has been called - COMPLETE, // AsyncContext.complete() has been called - EXPIRING, // AsyncContext timeout just happened - EXPIRED, // AsyncContext timeout has been processed - ERRORING, // An error just happened - ERRORED // The error has been processed - } - - private enum AsyncRead - { - IDLE, // No isReady; No data - REGISTER, // isReady()==false handling; No data - REGISTERED, // isReady()==false !handling; No data - POSSIBLE, // isReady()==false async read callback called (http/1 only) - PRODUCING, // isReady()==false READ_PRODUCE action is being handled (http/1 only) - READY // isReady() was false, onContentAdded has been called - } - - private final Locker _locker = new Locker(); private final HttpChannel _channel; private List _asyncListeners; - private State _state; - private Async _async; - private boolean _initial; - private AsyncRead _asyncRead = AsyncRead.IDLE; + private State _state = State.IDLE; + private RequestState _requestState = RequestState.BLOCKING; + private OutputState _outputState = OutputState.OPEN; + private InputState _inputState = InputState.IDLE; + private boolean _initial = true; + private boolean _sendError; private boolean _asyncWritePossible; private long _timeoutMs = DEFAULT_TIMEOUT; private AsyncContextEvent _event; @@ -125,14 +158,11 @@ public class HttpChannelState protected HttpChannelState(HttpChannel channel) { _channel = channel; - _state = State.IDLE; - _async = Async.NOT_ASYNC; - _initial = true; } public State getState() { - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { return _state; } @@ -140,7 +170,7 @@ public class HttpChannelState public void addListener(AsyncListener listener) { - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { if (_asyncListeners == null) _asyncListeners = new ArrayList<>(); @@ -150,7 +180,7 @@ public class HttpChannelState public boolean hasListener(AsyncListener listener) { - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { if (_asyncListeners == null) return false; @@ -167,9 +197,17 @@ public class HttpChannelState } } + public boolean isSendError() + { + synchronized (this) + { + return _sendError; + } + } + public void setTimeout(long ms) { - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { _timeoutMs = ms; } @@ -177,7 +215,7 @@ public class HttpChannelState public long getTimeout() { - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { return _timeoutMs; } @@ -185,7 +223,7 @@ public class HttpChannelState public AsyncContextEvent getAsyncContextEvent() { - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { return _event; } @@ -194,43 +232,139 @@ public class HttpChannelState @Override public String toString() { - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { return toStringLocked(); } } - public String toStringLocked() + private String toStringLocked() { - return String.format("%s@%x{s=%s a=%s i=%b r=%s w=%b}", + return String.format("%s@%x{%s}", getClass().getSimpleName(), hashCode(), - _state, - _async, - _initial, - _asyncRead, - _asyncWritePossible); + getStatusStringLocked()); } private String getStatusStringLocked() { - return String.format("s=%s i=%b a=%s", _state, _initial, _async); + return String.format("s=%s rs=%s os=%s is=%s awp=%b se=%b i=%b al=%d", + _state, + _requestState, + _outputState, + _inputState, + _asyncWritePossible, + _sendError, + _initial, + _asyncListeners == null ? 0 : _asyncListeners.size()); } public String getStatusString() { - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { return getStatusStringLocked(); } } + public boolean commitResponse() + { + synchronized (this) + { + switch (_outputState) + { + case OPEN: + _outputState = OutputState.COMMITTED; + return true; + + default: + return false; + } + } + } + + public boolean partialResponse() + { + synchronized (this) + { + switch (_outputState) + { + case COMMITTED: + _outputState = OutputState.OPEN; + return true; + + default: + return false; + } + } + } + + public boolean completeResponse() + { + synchronized (this) + { + switch (_outputState) + { + case OPEN: + case COMMITTED: + _outputState = OutputState.COMPLETED; + return true; + + default: + return false; + } + } + } + + public boolean isResponseCommitted() + { + synchronized (this) + { + switch (_outputState) + { + case OPEN: + return false; + default: + return true; + } + } + } + + public boolean isResponseCompleted() + { + synchronized (this) + { + return _outputState == OutputState.COMPLETED; + } + } + + public boolean abortResponse() + { + synchronized (this) + { + switch (_outputState) + { + case ABORTED: + return false; + + case OPEN: + _channel.getResponse().setStatus(500); + _outputState = OutputState.ABORTED; + return true; + + default: + _outputState = OutputState.ABORTED; + return true; + } + } + } + /** * @return Next handling of the request should proceed */ - protected Action handling() + public Action handling() { - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { if (LOG.isDebugEnabled()) LOG.debug("handling {}", toStringLocked()); @@ -238,90 +372,168 @@ public class HttpChannelState switch (_state) { case IDLE: + if (_requestState != RequestState.BLOCKING) + throw new IllegalStateException(getStatusStringLocked()); _initial = true; - _state = State.DISPATCHED; + _state = State.HANDLING; return Action.DISPATCH; - case COMPLETING: - case COMPLETED: - return Action.TERMINATED; - - case ASYNC_WOKEN: - switch (_asyncRead) + case WOKEN: + if (_event != null && _event.getThrowable() != null && !_sendError) { - case POSSIBLE: - _state = State.ASYNC_IO; - _asyncRead = AsyncRead.PRODUCING; - return Action.READ_PRODUCE; - case READY: - _state = State.ASYNC_IO; - _asyncRead = AsyncRead.IDLE; - return Action.READ_CALLBACK; - case REGISTER: - case PRODUCING: - case IDLE: - case REGISTERED: - break; - default: - throw new IllegalStateException(getStatusStringLocked()); + _state = State.HANDLING; + return Action.ASYNC_ERROR; } - if (_asyncWritePossible) - { - _state = State.ASYNC_IO; - _asyncWritePossible = false; - return Action.WRITE_CALLBACK; - } + Action action = nextAction(true); + if (LOG.isDebugEnabled()) + LOG.debug("nextAction(true) {} {}", action, toStringLocked()); + return action; - switch (_async) - { - case COMPLETE: - _state = State.COMPLETING; - return Action.COMPLETE; - case DISPATCH: - _state = State.DISPATCHED; - _async = Async.NOT_ASYNC; - return Action.ASYNC_DISPATCH; - case EXPIRED: - case ERRORED: - _state = State.DISPATCHED; - _async = Async.NOT_ASYNC; - return Action.ERROR_DISPATCH; - case STARTED: - case EXPIRING: - case ERRORING: - _state = State.ASYNC_WAIT; - return Action.NOOP; - case NOT_ASYNC: - default: - throw new IllegalStateException(getStatusStringLocked()); - } - - case ASYNC_ERROR: - return Action.ASYNC_ERROR; - - case ASYNC_IO: - case ASYNC_WAIT: - case DISPATCHED: - case UPGRADED: default: throw new IllegalStateException(getStatusStringLocked()); } } } + /** + * Signal that the HttpConnection has finished handling the request. + * For blocking connectors, this call may block if the request has + * been suspended (startAsync called). + * + * @return next actions + * be handled again (eg because of a resume that happened before unhandle was called) + */ + protected Action unhandle() + { + boolean readInterested = false; + + synchronized (this) + { + if (LOG.isDebugEnabled()) + LOG.debug("unhandle {}", toStringLocked()); + + if (_state != State.HANDLING) + throw new IllegalStateException(this.getStatusStringLocked()); + + _initial = false; + + Action action = nextAction(false); + if (LOG.isDebugEnabled()) + LOG.debug("nextAction(false) {} {}", action, toStringLocked()); + return action; + } + } + + private Action nextAction(boolean handling) + { + // Assume we can keep going, but exceptions are below + _state = State.HANDLING; + + if (_sendError) + { + switch (_requestState) + { + case BLOCKING: + case ASYNC: + case COMPLETE: + case DISPATCH: + case COMPLETING: + _requestState = RequestState.BLOCKING; + _sendError = false; + return Action.SEND_ERROR; + + default: + break; + } + } + + switch (_requestState) + { + case BLOCKING: + if (handling) + throw new IllegalStateException(getStatusStringLocked()); + _requestState = RequestState.COMPLETING; + return Action.COMPLETE; + + case ASYNC: + switch (_inputState) + { + case POSSIBLE: + _inputState = InputState.PRODUCING; + return Action.READ_PRODUCE; + case READY: + _inputState = InputState.IDLE; + return Action.READ_CALLBACK; + case REGISTER: + case PRODUCING: + _inputState = InputState.REGISTERED; + return Action.READ_REGISTER; + case IDLE: + case REGISTERED: + break; + default: + throw new IllegalStateException(getStatusStringLocked()); + } + + if (_asyncWritePossible) + { + _asyncWritePossible = false; + return Action.WRITE_CALLBACK; + } + + Scheduler scheduler = _channel.getScheduler(); + if (scheduler != null && _timeoutMs > 0 && !_event.hasTimeoutTask()) + _event.setTimeoutTask(scheduler.schedule(_event, _timeoutMs, TimeUnit.MILLISECONDS)); + _state = State.WAITING; + return Action.WAIT; + + case DISPATCH: + _requestState = RequestState.BLOCKING; + return Action.ASYNC_DISPATCH; + + case EXPIRE: + _requestState = RequestState.EXPIRING; + return Action.ASYNC_TIMEOUT; + + case EXPIRING: + if (handling) + throw new IllegalStateException(getStatusStringLocked()); + sendError(HttpStatus.INTERNAL_SERVER_ERROR_500, "AsyncContext timeout"); + // handle sendError immediately + _requestState = RequestState.BLOCKING; + _sendError = false; + return Action.SEND_ERROR; + + case COMPLETE: + _requestState = RequestState.COMPLETING; + return Action.COMPLETE; + + case COMPLETING: + _state = State.WAITING; + return Action.WAIT; + + case COMPLETED: + _state = State.IDLE; + return Action.TERMINATED; + + default: + throw new IllegalStateException(getStatusStringLocked()); + } + } + public void startAsync(AsyncContextEvent event) { final List lastAsyncListeners; - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { if (LOG.isDebugEnabled()) LOG.debug("startAsync {}", toStringLocked()); - if (_state != State.DISPATCHED || _async != Async.NOT_ASYNC) + if (_state != State.HANDLING || _requestState != RequestState.BLOCKING) throw new IllegalStateException(this.getStatusStringLocked()); - _async = Async.STARTED; + _requestState = RequestState.ASYNC; _event = event; lastAsyncListeners = _asyncListeners; _asyncListeners = null; @@ -359,216 +571,36 @@ public class HttpChannelState } } - public void asyncError(Throwable failure) - { - AsyncContextEvent event = null; - try (Locker.Lock lock = _locker.lock()) - { - switch (_state) - { - case IDLE: - case DISPATCHED: - case COMPLETING: - case COMPLETED: - case UPGRADED: - case ASYNC_IO: - case ASYNC_WOKEN: - case ASYNC_ERROR: - { - break; - } - case ASYNC_WAIT: - { - _event.addThrowable(failure); - _state = State.ASYNC_ERROR; - event = _event; - break; - } - default: - { - throw new IllegalStateException(getStatusStringLocked()); - } - } - } - - if (event != null) - { - cancelTimeout(event); - runInContext(event, _channel); - } - } - - /** - * Signal that the HttpConnection has finished handling the request. - * For blocking connectors, this call may block if the request has - * been suspended (startAsync called). - * - * @return next actions - * be handled again (eg because of a resume that happened before unhandle was called) - */ - protected Action unhandle() - { - boolean readInterested = false; - - try (Locker.Lock lock = _locker.lock()) - { - if (LOG.isDebugEnabled()) - LOG.debug("unhandle {}", toStringLocked()); - - switch (_state) - { - case COMPLETING: - case COMPLETED: - return Action.TERMINATED; - - case THROWN: - _state = State.DISPATCHED; - return Action.ERROR_DISPATCH; - - case DISPATCHED: - case ASYNC_IO: - case ASYNC_ERROR: - case ASYNC_WAIT: - break; - - default: - throw new IllegalStateException(this.getStatusStringLocked()); - } - - _initial = false; - switch (_async) - { - case COMPLETE: - _state = State.COMPLETING; - _async = Async.NOT_ASYNC; - return Action.COMPLETE; - - case DISPATCH: - _state = State.DISPATCHED; - _async = Async.NOT_ASYNC; - return Action.ASYNC_DISPATCH; - - case STARTED: - switch (_asyncRead) - { - case READY: - _state = State.ASYNC_IO; - _asyncRead = AsyncRead.IDLE; - return Action.READ_CALLBACK; - - case POSSIBLE: - _state = State.ASYNC_IO; - _asyncRead = AsyncRead.PRODUCING; - return Action.READ_PRODUCE; - - case REGISTER: - case PRODUCING: - _asyncRead = AsyncRead.REGISTERED; - readInterested = true; - break; - - case IDLE: - case REGISTERED: - break; - } - - if (_asyncWritePossible) - { - _state = State.ASYNC_IO; - _asyncWritePossible = false; - return Action.WRITE_CALLBACK; - } - else - { - _state = State.ASYNC_WAIT; - - Scheduler scheduler = _channel.getScheduler(); - if (scheduler != null && _timeoutMs > 0 && !_event.hasTimeoutTask()) - _event.setTimeoutTask(scheduler.schedule(_event, _timeoutMs, TimeUnit.MILLISECONDS)); - - return Action.WAIT; - } - - case EXPIRING: - // onTimeout callbacks still being called, so just WAIT - _state = State.ASYNC_WAIT; - return Action.WAIT; - - case EXPIRED: - // onTimeout handling is complete, but did not dispatch as - // we were handling. So do the error dispatch here - _state = State.DISPATCHED; - _async = Async.NOT_ASYNC; - return Action.ERROR_DISPATCH; - - case ERRORED: - _state = State.DISPATCHED; - _async = Async.NOT_ASYNC; - return Action.ERROR_DISPATCH; - - case NOT_ASYNC: - _state = State.COMPLETING; - return Action.COMPLETE; - - default: - _state = State.COMPLETING; - return Action.COMPLETE; - } - } - finally - { - if (readInterested) - _channel.onAsyncWaitForContent(); - } - } - public void dispatch(ServletContext context, String path) { boolean dispatch = false; AsyncContextEvent event; - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { if (LOG.isDebugEnabled()) LOG.debug("dispatch {} -> {}", toStringLocked(), path); - boolean started = false; - event = _event; - switch (_async) + switch (_requestState) { - case STARTED: - started = true; - break; + case ASYNC: case EXPIRING: - case ERRORING: - case ERRORED: break; default: throw new IllegalStateException(this.getStatusStringLocked()); } - _async = Async.DISPATCH; if (context != null) _event.setDispatchContext(context); if (path != null) _event.setDispatchPath(path); - if (started) + if (_requestState == RequestState.ASYNC && _state == State.WAITING) { - switch (_state) - { - case DISPATCHED: - case ASYNC_IO: - case ASYNC_WOKEN: - break; - case ASYNC_WAIT: - _state = State.ASYNC_WOKEN; - dispatch = true; - break; - default: - LOG.warn("async dispatched when complete {}", this); - break; - } + _state = State.WOKEN; + dispatch = true; } + _requestState = RequestState.DISPATCH; + event = _event; } cancelTimeout(event); @@ -576,23 +608,47 @@ public class HttpChannelState scheduleDispatch(); } + protected void timeout() + { + boolean dispatch = false; + synchronized (this) + { + if (LOG.isDebugEnabled()) + LOG.debug("Timeout {}", toStringLocked()); + + if (_requestState != RequestState.ASYNC) + return; + _requestState = RequestState.EXPIRE; + + if (_state == State.WAITING) + { + _state = State.WOKEN; + dispatch = true; + } + } + + if (dispatch) + { + if (LOG.isDebugEnabled()) + LOG.debug("Dispatch after async timeout {}", this); + scheduleDispatch(); + } + } + protected void onTimeout() { final List listeners; AsyncContextEvent event; - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { if (LOG.isDebugEnabled()) LOG.debug("onTimeout {}", toStringLocked()); - - if (_async != Async.STARTED) - return; - _async = Async.EXPIRING; + if (_requestState != RequestState.EXPIRING || _state != State.HANDLING) + throw new IllegalStateException(toStringLocked()); event = _event; listeners = _asyncListeners; } - final AtomicReference error = new AtomicReference<>(); if (listeners != null) { Runnable task = new Runnable() @@ -610,11 +666,6 @@ public class HttpChannelState { LOG.warn(x + " while invoking onTimeout listener " + listener); LOG.debug(x); - Throwable failure = error.get(); - if (failure == null) - error.set(x); - else if (x != failure) - failure.addSuppressed(x); } } } @@ -628,86 +679,34 @@ public class HttpChannelState runInContext(event, task); } - - Throwable th = error.get(); - boolean dispatch = false; - try (Locker.Lock lock = _locker.lock()) - { - switch (_async) - { - case EXPIRING: - _async = th == null ? Async.EXPIRED : Async.ERRORING; - break; - - case COMPLETE: - case DISPATCH: - if (th != null) - { - LOG.ignore(th); - th = null; - } - break; - - default: - throw new IllegalStateException(); - } - - if (_state == State.ASYNC_WAIT) - { - _state = State.ASYNC_WOKEN; - dispatch = true; - } - } - - if (th != null) - { - if (LOG.isDebugEnabled()) - LOG.debug("Error after async timeout {}", this, th); - onError(th); - } - - if (dispatch) - { - if (LOG.isDebugEnabled()) - LOG.debug("Dispatch after async timeout {}", this); - scheduleDispatch(); - } } public void complete() { - - // just like resume, except don't set _dispatched=true; boolean handle = false; AsyncContextEvent event; - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { if (LOG.isDebugEnabled()) LOG.debug("complete {}", toStringLocked()); - boolean started = false; event = _event; - - switch (_async) + switch (_requestState) { - case STARTED: - started = true; - break; case EXPIRING: - case ERRORING: - case ERRORED: + case ASYNC: + _requestState = _sendError ? RequestState.BLOCKING : RequestState.COMPLETE; break; + case COMPLETE: return; default: throw new IllegalStateException(this.getStatusStringLocked()); } - _async = Async.COMPLETE; - - if (started && _state == State.ASYNC_WAIT) + if (_state == State.WAITING) { handle = true; - _state = State.ASYNC_WOKEN; + _state = State.WOKEN; } } @@ -716,228 +715,308 @@ public class HttpChannelState runInContext(event, _channel); } - public void errorComplete() + public void asyncError(Throwable failure) { - try (Locker.Lock lock = _locker.lock()) + // This method is called when an failure occurs asynchronously to + // normal handling. If the request is async, we arrange for the + // exception to be thrown from the normal handling loop and then + // actually handled by #thrownException + + AsyncContextEvent event = null; + synchronized (this) { if (LOG.isDebugEnabled()) - LOG.debug("error complete {}", toStringLocked()); + LOG.debug("asyncError " + toStringLocked(), failure); - _async = Async.COMPLETE; - _event.setDispatchContext(null); - _event.setDispatchPath(null); - } - - cancelTimeout(); - } - - protected void onError(Throwable th) - { - final List listeners; - final AsyncContextEvent event; - final Request baseRequest = _channel.getRequest(); - - int code = HttpStatus.INTERNAL_SERVER_ERROR_500; - String reason = null; - Throwable cause = _channel.unwrap(th, BadMessageException.class, UnavailableException.class); - if (cause instanceof BadMessageException) - { - BadMessageException bme = (BadMessageException)cause; - code = bme.getCode(); - reason = bme.getReason(); - } - else if (cause instanceof UnavailableException) - { - if (((UnavailableException)cause).isPermanent()) - code = HttpStatus.NOT_FOUND_404; - else - code = HttpStatus.SERVICE_UNAVAILABLE_503; - } - - try (Locker.Lock lock = _locker.lock()) - { - if (LOG.isDebugEnabled()) - LOG.debug("onError {} {}", toStringLocked(), th); - - // Set error on request. - if (_event != null) + if (_state == State.WAITING && _requestState == RequestState.ASYNC) { - _event.addThrowable(th); - _event.getSuppliedRequest().setAttribute(ERROR_STATUS_CODE, code); - _event.getSuppliedRequest().setAttribute(ERROR_EXCEPTION, th); - _event.getSuppliedRequest().setAttribute(ERROR_EXCEPTION_TYPE, th == null ? null : th.getClass()); - _event.getSuppliedRequest().setAttribute(ERROR_MESSAGE, reason); + _state = State.WOKEN; + _event.addThrowable(failure); + event = _event; } else { - Throwable error = (Throwable)baseRequest.getAttribute(ERROR_EXCEPTION); - if (error != null) - throw new IllegalStateException("Error already set", error); - baseRequest.setAttribute(ERROR_STATUS_CODE, code); - baseRequest.setAttribute(ERROR_EXCEPTION, th); - baseRequest.setAttribute(RequestDispatcher.ERROR_EXCEPTION_TYPE, th == null ? null : th.getClass()); - baseRequest.setAttribute(ERROR_MESSAGE, reason); - } - - // Are we blocking? - if (_async == Async.NOT_ASYNC) - { - // Only called from within HttpChannel Handling, so much be dispatched, let's stay dispatched! - if (_state == State.DISPATCHED) - { - _state = State.THROWN; - return; - } - throw new IllegalStateException(this.getStatusStringLocked()); - } - - // We are Async - _async = Async.ERRORING; - listeners = _asyncListeners; - event = _event; - } - - if (listeners != null) - { - Runnable task = new Runnable() - { - @Override - public void run() - { - for (AsyncListener listener : listeners) - { - try - { - listener.onError(event); - } - catch (Throwable x) - { - LOG.warn(x + " while invoking onError listener " + listener); - LOG.debug(x); - } - } - } - - @Override - public String toString() - { - return "onError"; - } - }; - runInContext(event, task); - } - - boolean dispatch = false; - try (Locker.Lock lock = _locker.lock()) - { - switch (_async) - { - case ERRORING: - { - // Still in this state ? The listeners did not invoke API methods - // and the container must provide a default error dispatch. - _async = Async.ERRORED; - break; - } - case DISPATCH: - case COMPLETE: - { - // The listeners called dispatch() or complete(). - break; - } - default: - { - throw new IllegalStateException(toString()); - } - } - - if (_state == State.ASYNC_WAIT) - { - _state = State.ASYNC_WOKEN; - dispatch = true; - } - } - - if (dispatch) - { - if (LOG.isDebugEnabled()) - LOG.debug("Dispatch after error {}", this); - scheduleDispatch(); - } - } - - protected void onComplete() - { - final List aListeners; - final AsyncContextEvent event; - - try (Locker.Lock lock = _locker.lock()) - { - if (LOG.isDebugEnabled()) - LOG.debug("onComplete {}", toStringLocked()); - - switch (_state) - { - case COMPLETING: - aListeners = _asyncListeners; - event = _event; - _state = State.COMPLETED; - _async = Async.NOT_ASYNC; - break; - - default: - throw new IllegalStateException(this.getStatusStringLocked()); + LOG.warn(failure.toString()); + LOG.debug(failure); } } if (event != null) { + cancelTimeout(event); + runInContext(event, _channel); + } + } + + protected void onError(Throwable th) + { + final AsyncContextEvent asyncEvent; + final List asyncListeners; + synchronized (this) + { + if (LOG.isDebugEnabled()) + LOG.debug("thrownException " + getStatusStringLocked(), th); + + // This can only be called from within the handle loop + if (_state != State.HANDLING) + throw new IllegalStateException(getStatusStringLocked()); + + // If sendError has already been called, we can only handle one failure at a time! + if (_sendError) + { + LOG.warn("unhandled due to prior sendError", th); + return; + } + + // Check async state to determine type of handling + switch (_requestState) + { + case BLOCKING: + // handle the exception with a sendError + sendError(th); + return; + + case DISPATCH: // Dispatch has already been called but we ignore and handle exception below + case COMPLETE: // Complete has already been called but we ignore and handle exception below + case ASYNC: + if (_asyncListeners == null || _asyncListeners.isEmpty()) + { + sendError(th); + return; + } + asyncEvent = _event; + asyncEvent.addThrowable(th); + asyncListeners = _asyncListeners; + break; + + default: + LOG.warn("unhandled in state " + _requestState, new IllegalStateException(th)); + return; + } + } + + // If we are async and have async listeners + // call onError + runInContext(asyncEvent, () -> + { + for (AsyncListener listener : asyncListeners) + { + try + { + listener.onError(asyncEvent); + } + catch (Throwable x) + { + LOG.warn(x + " while invoking onError listener " + listener); + LOG.debug(x); + } + } + }); + + // check the actions of the listeners + synchronized (this) + { + // If we are still async and nobody has called sendError + if (_requestState == RequestState.ASYNC && !_sendError) + // Then the listeners did not invoke API methods + // and the container must provide a default error dispatch. + sendError(th); + else + LOG.warn("unhandled in state " + _requestState, new IllegalStateException(th)); + } + } + + private void sendError(Throwable th) + { + // No sync as this is always called with lock held + + // Determine the actual details of the exception + final Request request = _channel.getRequest(); + final int code; + final String message; + Throwable cause = _channel.unwrap(th, BadMessageException.class, UnavailableException.class); + if (cause == null) + { + code = HttpStatus.INTERNAL_SERVER_ERROR_500; + message = th.toString(); + } + else if (cause instanceof BadMessageException) + { + BadMessageException bme = (BadMessageException)cause; + code = bme.getCode(); + message = bme.getReason(); + } + else if (cause instanceof UnavailableException) + { + message = cause.toString(); + if (((UnavailableException)cause).isPermanent()) + code = HttpStatus.NOT_FOUND_404; + else + code = HttpStatus.SERVICE_UNAVAILABLE_503; + } + else + { + code = HttpStatus.INTERNAL_SERVER_ERROR_500; + message = null; + } + + sendError(code, message); + + // No ISE, so good to modify request/state + request.setAttribute(ERROR_EXCEPTION, th); + request.setAttribute(ERROR_EXCEPTION_TYPE, th.getClass()); + // Ensure any async lifecycle is ended! + _requestState = RequestState.BLOCKING; + } + + public void sendError(int code, String message) + { + // This method is called by Response.sendError to organise for an error page to be generated when it is possible: + // + The response is reset and temporarily closed. + // + The details of the error are saved as request attributes + // + The _sendError boolean is set to true so that an ERROR_DISPATCH action will be generated: + // - after unhandle for sync + // - after both unhandle and complete for async + + final Request request = _channel.getRequest(); + final Response response = _channel.getResponse(); + if (message == null) + message = HttpStatus.getMessage(code); + + synchronized (this) + { + if (LOG.isDebugEnabled()) + LOG.debug("sendError {}", toStringLocked()); + + switch (_state) + { + case HANDLING: + case WOKEN: + case WAITING: + break; + default: + throw new IllegalStateException(getStatusStringLocked()); + } + if (_outputState != OutputState.OPEN) + throw new IllegalStateException("Response is " + _outputState); + + response.getHttpOutput().closedBySendError(); + response.setStatus(code); + + request.setAttribute(ErrorHandler.ERROR_CONTEXT, request.getErrorContext()); + request.setAttribute(ERROR_REQUEST_URI, request.getRequestURI()); + request.setAttribute(ERROR_SERVLET_NAME, request.getServletName()); + request.setAttribute(ERROR_STATUS_CODE, code); + request.setAttribute(ERROR_MESSAGE, message); + + _sendError = true; + if (_event != null) + { + Throwable cause = (Throwable)request.getAttribute(ERROR_EXCEPTION); + if (cause != null) + _event.addThrowable(cause); + } + } + } + + protected void completing() + { + synchronized (this) + { + if (LOG.isDebugEnabled()) + LOG.debug("completing {}", toStringLocked()); + + switch (_requestState) + { + case COMPLETED: + throw new IllegalStateException(getStatusStringLocked()); + default: + _requestState = RequestState.COMPLETING; + } + } + } + + protected void completed() + { + final List aListeners; + final AsyncContextEvent event; + boolean handle = false; + + synchronized (this) + { + if (LOG.isDebugEnabled()) + LOG.debug("completed {}", toStringLocked()); + + if (_requestState != RequestState.COMPLETING) + throw new IllegalStateException(this.getStatusStringLocked()); + + if (_event == null) + { + _requestState = RequestState.COMPLETED; + aListeners = null; + event = null; + if (_state == State.WAITING) + { + _state = State.WOKEN; + handle = true; + } + } + else + { + aListeners = _asyncListeners; + event = _event; + } + } + + if (event != null) + { + cancelTimeout(event); if (aListeners != null) { - Runnable callback = new Runnable() + runInContext(event, () -> { - @Override - public void run() + for (AsyncListener listener : aListeners) { - for (AsyncListener listener : aListeners) + try { - try - { - listener.onComplete(event); - } - catch (Throwable e) - { - LOG.warn(e + " while invoking onComplete listener " + listener); - LOG.debug(e); - } + listener.onComplete(event); + } + catch (Throwable e) + { + LOG.warn(e + " while invoking onComplete listener " + listener); + LOG.debug(e); } } - - @Override - public String toString() - { - return "onComplete"; - } - }; - - runInContext(event, callback); + }); } event.completed(); + + synchronized (this) + { + _requestState = RequestState.COMPLETED; + if (_state == State.WAITING) + { + _state = State.WOKEN; + handle = true; + } + } } + + if (handle) + _channel.handle(); } protected void recycle() { cancelTimeout(); - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { if (LOG.isDebugEnabled()) LOG.debug("recycle {}", toStringLocked()); switch (_state) { - case DISPATCHED: - case ASYNC_IO: + case HANDLING: throw new IllegalStateException(getStatusStringLocked()); case UPGRADED: return; @@ -946,9 +1025,10 @@ public class HttpChannelState } _asyncListeners = null; _state = State.IDLE; - _async = Async.NOT_ASYNC; + _requestState = RequestState.BLOCKING; + _outputState = OutputState.OPEN; _initial = true; - _asyncRead = AsyncRead.IDLE; + _inputState = InputState.IDLE; _asyncWritePossible = false; _timeoutMs = DEFAULT_TIMEOUT; _event = null; @@ -958,7 +1038,7 @@ public class HttpChannelState public void upgrade() { cancelTimeout(); - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { if (LOG.isDebugEnabled()) LOG.debug("upgrade {}", toStringLocked()); @@ -966,16 +1046,15 @@ public class HttpChannelState switch (_state) { case IDLE: - case COMPLETED: break; default: throw new IllegalStateException(getStatusStringLocked()); } _asyncListeners = null; _state = State.UPGRADED; - _async = Async.NOT_ASYNC; + _requestState = RequestState.BLOCKING; _initial = true; - _asyncRead = AsyncRead.IDLE; + _inputState = InputState.IDLE; _asyncWritePossible = false; _timeoutMs = DEFAULT_TIMEOUT; _event = null; @@ -990,7 +1069,7 @@ public class HttpChannelState protected void cancelTimeout() { final AsyncContextEvent event; - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { event = _event; } @@ -1005,7 +1084,7 @@ public class HttpChannelState public boolean isIdle() { - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { return _state == State.IDLE; } @@ -1013,15 +1092,16 @@ public class HttpChannelState public boolean isExpired() { - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { - return _async == Async.EXPIRED; + // TODO review + return _requestState == RequestState.EXPIRE || _requestState == RequestState.EXPIRING; } } public boolean isInitial() { - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { return _initial; } @@ -1029,51 +1109,35 @@ public class HttpChannelState public boolean isSuspended() { - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { - return _state == State.ASYNC_WAIT || _state == State.DISPATCHED && _async == Async.STARTED; - } - } - - boolean isCompleting() - { - try (Locker.Lock lock = _locker.lock()) - { - return _state == State.COMPLETING; + return _state == State.WAITING || _state == State.HANDLING && _requestState == RequestState.ASYNC; } } boolean isCompleted() { - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { - return _state == State.COMPLETED; + return _requestState == RequestState.COMPLETED; } } public boolean isAsyncStarted() { - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { - if (_state == State.DISPATCHED) - return _async != Async.NOT_ASYNC; - return _async == Async.STARTED || _async == Async.EXPIRING; - } - } - - public boolean isAsyncComplete() - { - try (Locker.Lock lock = _locker.lock()) - { - return _async == Async.COMPLETE; + if (_state == State.HANDLING) + return _requestState != RequestState.BLOCKING; + return _requestState == RequestState.ASYNC || _requestState == RequestState.EXPIRING; } } public boolean isAsync() { - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { - return !_initial || _async != Async.NOT_ASYNC; + return !_initial || _requestState != RequestState.BLOCKING; } } @@ -1090,7 +1154,7 @@ public class HttpChannelState public ContextHandler getContextHandler() { final AsyncContextEvent event; - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { event = _event; } @@ -1111,7 +1175,7 @@ public class HttpChannelState public ServletResponse getServletResponse() { final AsyncContextEvent event; - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { event = _event; } @@ -1159,23 +1223,23 @@ public class HttpChannelState public void onReadUnready() { boolean interested = false; - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { if (LOG.isDebugEnabled()) LOG.debug("onReadUnready {}", toStringLocked()); - switch (_asyncRead) + switch (_inputState) { case IDLE: case READY: - if (_state == State.ASYNC_WAIT) + if (_state == State.WAITING) { interested = true; - _asyncRead = AsyncRead.REGISTERED; + _inputState = InputState.REGISTERED; } else { - _asyncRead = AsyncRead.REGISTER; + _inputState = InputState.REGISTER; } break; @@ -1202,28 +1266,28 @@ public class HttpChannelState public boolean onContentAdded() { boolean woken = false; - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { if (LOG.isDebugEnabled()) LOG.debug("onContentAdded {}", toStringLocked()); - switch (_asyncRead) + switch (_inputState) { case IDLE: case READY: break; case PRODUCING: - _asyncRead = AsyncRead.READY; + _inputState = InputState.READY; break; case REGISTER: case REGISTERED: - _asyncRead = AsyncRead.READY; - if (_state == State.ASYNC_WAIT) + _inputState = InputState.READY; + if (_state == State.WAITING) { woken = true; - _state = State.ASYNC_WOKEN; + _state = State.WOKEN; } break; @@ -1245,19 +1309,19 @@ public class HttpChannelState public boolean onReadReady() { boolean woken = false; - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { if (LOG.isDebugEnabled()) LOG.debug("onReadReady {}", toStringLocked()); - switch (_asyncRead) + switch (_inputState) { case IDLE: - _asyncRead = AsyncRead.READY; - if (_state == State.ASYNC_WAIT) + _inputState = InputState.READY; + if (_state == State.WAITING) { woken = true; - _state = State.ASYNC_WOKEN; + _state = State.WOKEN; } break; @@ -1278,19 +1342,19 @@ public class HttpChannelState public boolean onReadPossible() { boolean woken = false; - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { if (LOG.isDebugEnabled()) LOG.debug("onReadPossible {}", toStringLocked()); - switch (_asyncRead) + switch (_inputState) { case REGISTERED: - _asyncRead = AsyncRead.POSSIBLE; - if (_state == State.ASYNC_WAIT) + _inputState = InputState.POSSIBLE; + if (_state == State.WAITING) { woken = true; - _state = State.ASYNC_WOKEN; + _state = State.WOKEN; } break; @@ -1310,17 +1374,17 @@ public class HttpChannelState public boolean onReadEof() { boolean woken = false; - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { if (LOG.isDebugEnabled()) LOG.debug("onEof {}", toStringLocked()); // Force read ready so onAllDataRead can be called - _asyncRead = AsyncRead.READY; - if (_state == State.ASYNC_WAIT) + _inputState = InputState.READY; + if (_state == State.WAITING) { woken = true; - _state = State.ASYNC_WOKEN; + _state = State.WOKEN; } } return woken; @@ -1330,15 +1394,15 @@ public class HttpChannelState { boolean wake = false; - try (Locker.Lock lock = _locker.lock()) + synchronized (this) { if (LOG.isDebugEnabled()) LOG.debug("onWritePossible {}", toStringLocked()); _asyncWritePossible = true; - if (_state == State.ASYNC_WAIT) + if (_state == State.WAITING) { - _state = State.ASYNC_WOKEN; + _state = State.WOKEN; wake = true; } } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java index d81963c4058..80c31516e00 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java @@ -278,18 +278,8 @@ public class HttpConnection extends AbstractConnection implements Runnable, Http } else if (filled < 0) { - switch (_channel.getState().getState()) - { - case COMPLETING: - case COMPLETED: - case IDLE: - case THROWN: - case ASYNC_ERROR: - getEndPoint().shutdownOutput(); - break; - default: - break; - } + if (_channel.getState().isIdle()) + getEndPoint().shutdownOutput(); break; } } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java index bb5b1d58833..15f04a2c864 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java @@ -291,7 +291,8 @@ public class HttpInput extends ServletInputStream implements Runnable { BadMessageException bad = new BadMessageException(HttpStatus.REQUEST_TIMEOUT_408, String.format("Request content data rate < %d B/s", minRequestDataRate)); - _channelState.getHttpChannel().abort(bad); + if (_channelState.isResponseCommitted()) + _channelState.getHttpChannel().abort(bad); throw bad; } } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java index e7dea9cba67..655e8cd5dbd 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java @@ -18,6 +18,7 @@ package org.eclipse.jetty.server; +import java.io.Closeable; import java.io.IOException; import java.io.InputStream; import java.nio.ByteBuffer; @@ -62,8 +63,31 @@ import org.eclipse.jetty.util.log.Logger; public class HttpOutput extends ServletOutputStream implements Runnable { private static final String LSTRING_FILE = "javax.servlet.LocalStrings"; + private static final Callback BLOCKING_CLOSE_CALLBACK = new Callback() {}; private static ResourceBundle lStrings = ResourceBundle.getBundle(LSTRING_FILE); + /* + ACTION OPEN ASYNC READY PENDING UNREADY CLOSING CLOSED + -------------------------------------------------------------------------------------------------- + setWriteListener() READY->owp ise ise ise ise ise ise + write() OPEN ise PENDING wpe wpe eof eof + flush() OPEN ise PENDING wpe wpe eof eof + close() CLOSING CLOSING CLOSING CLOSED CLOSED CLOSING CLOSED + isReady() OPEN:true READY:true READY:true UNREADY:false UNREADY:false CLOSED:true CLOSED:true + write completed - - - ASYNC READY->owp CLOSED - + */ + enum State + { + OPEN, // Open in blocking mode + ASYNC, // Open in async mode + READY, // isReady() has returned true + PENDING, // write operating in progress + UNREADY, // write operating in progress, isReady has returned false + ERROR, // An error has occured + CLOSING, // Asynchronous close in progress + CLOSED // Closed + } + /** * The HttpOutput.Interceptor is a single intercept point for all * output written to the HttpOutput: via writer; via output stream; @@ -129,6 +153,7 @@ public class HttpOutput extends ServletOutputStream implements Runnable private static Logger LOG = Log.getLogger(HttpOutput.class); private static final ThreadLocal _encoder = new ThreadLocal<>(); + private final AtomicReference _state = new AtomicReference<>(State.OPEN); private final HttpChannel _channel; private final SharedBlockingCallback _writeBlocker; private Interceptor _interceptor; @@ -140,23 +165,7 @@ public class HttpOutput extends ServletOutputStream implements Runnable private int _commitSize; private WriteListener _writeListener; private volatile Throwable _onError; - - /* - ACTION OPEN ASYNC READY PENDING UNREADY CLOSED - ------------------------------------------------------------------------------------------- - setWriteListener() READY->owp ise ise ise ise ise - write() OPEN ise PENDING wpe wpe eof - flush() OPEN ise PENDING wpe wpe eof - close() CLOSED CLOSED CLOSED CLOSED CLOSED CLOSED - isReady() OPEN:true READY:true READY:true UNREADY:false UNREADY:false CLOSED:true - write completed - - - ASYNC READY->owp - - */ - private enum OutputState - { - OPEN, ASYNC, READY, PENDING, UNREADY, ERROR, CLOSED - } - - private final AtomicReference _state = new AtomicReference<>(OutputState.OPEN); + private Callback _closeCallback; public HttpOutput(HttpChannel channel) { @@ -200,7 +209,7 @@ public class HttpOutput extends ServletOutputStream implements Runnable public void reopen() { - _state.set(OutputState.OPEN); + _state.set(State.OPEN); } private boolean isLastContentToWrite(int len) @@ -256,28 +265,78 @@ public class HttpOutput extends ServletOutputStream implements Runnable _channel.abort(failure); } - @Override - public void close() + public void closedBySendError() { while (true) { - OutputState state = _state.get(); + State state = _state.get(); switch (state) { + case OPEN: + case READY: + case ASYNC: + if (!_state.compareAndSet(state, State.CLOSED)) + continue; + return; + + default: + throw new IllegalStateException(state.toString()); + } + } + } + + public void close(Closeable wrapper, Callback callback) + { + _closeCallback = callback; + try + { + if (wrapper != null) + wrapper.close(); + if (!isClosed()) + close(); + } + catch (Throwable th) + { + closed(); + if (_closeCallback == null) + LOG.ignore(th); + else + callback.failed(th); + } + finally + { + if (_closeCallback != null) + callback.succeeded(); + _closeCallback = null; + } + } + + @Override + public void close() + { + Callback closeCallback = _closeCallback == null ? BLOCKING_CLOSE_CALLBACK : _closeCallback; + + while (true) + { + State state = _state.get(); + switch (state) + { + case CLOSING: case CLOSED: { + _closeCallback = null; + closeCallback.succeeded(); return; } case ASYNC: { // A close call implies a write operation, thus in asynchronous mode // a call to isReady() that returned true should have been made. - // However it is desirable to allow a close at any time, specially if - // complete is called. Thus we simulate a call to isReady here, assuming - // that we can transition to READY. - if (!_state.compareAndSet(state, OutputState.READY)) - continue; - break; + // However it is desirable to allow a close at any time, specially if + // complete is called. Thus we simulate a call to isReady here, by + // trying to move to READY state. Either way we continue. + _state.compareAndSet(state, State.READY); + continue; } case UNREADY: case PENDING: @@ -288,34 +347,45 @@ public class HttpOutput extends ServletOutputStream implements Runnable // complete is called. Because the prior write has not yet completed // and/or isReady has not been called, this close is allowed, but will // abort the response. - if (!_state.compareAndSet(state, OutputState.CLOSED)) + if (!_state.compareAndSet(state, State.CLOSED)) continue; IOException ex = new IOException("Closed while Pending/Unready"); LOG.warn(ex.toString()); LOG.debug(ex); abort(ex); + _closeCallback = null; + closeCallback.failed(ex); return; } default: { - if (!_state.compareAndSet(state, OutputState.CLOSED)) + if (!_state.compareAndSet(state, State.CLOSING)) continue; // Do a normal close by writing the aggregate buffer or an empty buffer. If we are // not including, then indicate this is the last write. try { - write(BufferUtil.hasContent(_aggregate) ? _aggregate : BufferUtil.EMPTY_BUFFER, !_channel.getResponse().isIncluding()); + ByteBuffer content = BufferUtil.hasContent(_aggregate) ? _aggregate : BufferUtil.EMPTY_BUFFER; + if (closeCallback == BLOCKING_CLOSE_CALLBACK) + { + // Do a blocking close + write(content, !_channel.getResponse().isIncluding()); + _closeCallback = null; + closeCallback.succeeded(); + } + else + { + _closeCallback = null; + write(content, !_channel.getResponse().isIncluding(), closeCallback); + } } catch (IOException x) { LOG.ignore(x); // Ignore it, it's been already logged in write(). + _closeCallback = null; + closeCallback.failed(x); } - finally - { - releaseBuffer(); - } - // Return even if an exception is thrown by write(). return; } } @@ -326,11 +396,11 @@ public class HttpOutput extends ServletOutputStream implements Runnable * Called to indicate that the last write has been performed. * It updates the state and performs cleanup operations. */ - void closed() + public void closed() { while (true) { - OutputState state = _state.get(); + State state = _state.get(); switch (state) { case CLOSED: @@ -339,15 +409,16 @@ public class HttpOutput extends ServletOutputStream implements Runnable } case UNREADY: { - if (_state.compareAndSet(state, OutputState.ERROR)) + if (_state.compareAndSet(state, State.ERROR)) _writeListener.onError(_onError == null ? new EofException("Async closed") : _onError); break; } default: { - if (!_state.compareAndSet(state, OutputState.CLOSED)) + if (!_state.compareAndSet(state, State.CLOSED)) break; + // Just make sure write and output stream really are closed try { _channel.getResponse().closeOutput(); @@ -369,6 +440,18 @@ public class HttpOutput extends ServletOutputStream implements Runnable } } + public ByteBuffer getBuffer() + { + return _aggregate; + } + + public ByteBuffer acquireBuffer() + { + if (_aggregate == null) + _aggregate = _channel.getByteBufferPool().acquire(getBufferSize(), _interceptor.isOptimizedForDirectBuffers()); + return _aggregate; + } + private void releaseBuffer() { if (_aggregate != null) @@ -380,7 +463,14 @@ public class HttpOutput extends ServletOutputStream implements Runnable public boolean isClosed() { - return _state.get() == OutputState.CLOSED; + switch (_state.get()) + { + case CLOSING: + case CLOSED: + return true; + default: + return false; + } } public boolean isAsync() @@ -402,7 +492,8 @@ public class HttpOutput extends ServletOutputStream implements Runnable { while (true) { - switch (_state.get()) + State state = _state.get(); + switch (state) { case OPEN: write(BufferUtil.hasContent(_aggregate) ? _aggregate : BufferUtil.EMPTY_BUFFER, false); @@ -412,25 +503,24 @@ public class HttpOutput extends ServletOutputStream implements Runnable throw new IllegalStateException("isReady() not called"); case READY: - if (!_state.compareAndSet(OutputState.READY, OutputState.PENDING)) + if (!_state.compareAndSet(state, State.PENDING)) continue; new AsyncFlush().iterate(); return; - case PENDING: - return; - case UNREADY: throw new WritePendingException(); case ERROR: throw new EofException(_onError); + case PENDING: + case CLOSING: case CLOSED: return; default: - throw new IllegalStateException(); + throw new IllegalStateException(state.toString()); } } } @@ -441,7 +531,8 @@ public class HttpOutput extends ServletOutputStream implements Runnable // Async or Blocking ? while (true) { - switch (_state.get()) + State state = _state.get(); + switch (state) { case OPEN: // process blocking below @@ -451,15 +542,14 @@ public class HttpOutput extends ServletOutputStream implements Runnable throw new IllegalStateException("isReady() not called"); case READY: - if (!_state.compareAndSet(OutputState.READY, OutputState.PENDING)) + if (!_state.compareAndSet(state, State.PENDING)) continue; // Should we aggregate? boolean last = isLastContentToWrite(len); if (!last && len <= _commitSize) { - if (_aggregate == null) - _aggregate = _channel.getByteBufferPool().acquire(getBufferSize(), _interceptor.isOptimizedForDirectBuffers()); + acquireBuffer(); // YES - fill the aggregate with content from the buffer int filled = BufferUtil.fill(_aggregate, b, off, len); @@ -467,8 +557,8 @@ public class HttpOutput extends ServletOutputStream implements Runnable // return if we are not complete, not full and filled all the content if (filled == len && !BufferUtil.isFull(_aggregate)) { - if (!_state.compareAndSet(OutputState.PENDING, OutputState.ASYNC)) - throw new IllegalStateException(); + if (!_state.compareAndSet(State.PENDING, State.ASYNC)) + throw new IllegalStateException(_state.get().toString()); return; } @@ -488,11 +578,12 @@ public class HttpOutput extends ServletOutputStream implements Runnable case ERROR: throw new EofException(_onError); + case CLOSING: case CLOSED: throw new EofException("Closed"); default: - throw new IllegalStateException(); + throw new IllegalStateException(state.toString()); } break; } @@ -504,8 +595,7 @@ public class HttpOutput extends ServletOutputStream implements Runnable boolean last = isLastContentToWrite(len); if (!last && len <= _commitSize) { - if (_aggregate == null) - _aggregate = _channel.getByteBufferPool().acquire(capacity, _interceptor.isOptimizedForDirectBuffers()); + acquireBuffer(); // YES - fill the aggregate with content from the buffer int filled = BufferUtil.fill(_aggregate, b, off, len); @@ -554,9 +644,6 @@ public class HttpOutput extends ServletOutputStream implements Runnable { write(BufferUtil.EMPTY_BUFFER, true); } - - if (last) - closed(); } public void write(ByteBuffer buffer) throws IOException @@ -566,7 +653,8 @@ public class HttpOutput extends ServletOutputStream implements Runnable // Async or Blocking ? while (true) { - switch (_state.get()) + State state = _state.get(); + switch (state) { case OPEN: // process blocking below @@ -576,7 +664,7 @@ public class HttpOutput extends ServletOutputStream implements Runnable throw new IllegalStateException("isReady() not called"); case READY: - if (!_state.compareAndSet(OutputState.READY, OutputState.PENDING)) + if (!_state.compareAndSet(state, State.PENDING)) continue; // Do the asynchronous writing from the callback @@ -591,11 +679,12 @@ public class HttpOutput extends ServletOutputStream implements Runnable case ERROR: throw new EofException(_onError); + case CLOSING: case CLOSED: throw new EofException("Closed"); default: - throw new IllegalStateException(); + throw new IllegalStateException(state.toString()); } break; } @@ -613,9 +702,6 @@ public class HttpOutput extends ServletOutputStream implements Runnable write(buffer, last); else if (last) write(BufferUtil.EMPTY_BUFFER, true); - - if (last) - closed(); } @Override @@ -630,34 +716,28 @@ public class HttpOutput extends ServletOutputStream implements Runnable switch (_state.get()) { case OPEN: - if (_aggregate == null) - _aggregate = _channel.getByteBufferPool().acquire(getBufferSize(), _interceptor.isOptimizedForDirectBuffers()); + acquireBuffer(); BufferUtil.append(_aggregate, (byte)b); // Check if all written or full if (complete || BufferUtil.isFull(_aggregate)) - { write(_aggregate, complete); - if (complete) - closed(); - } break; case ASYNC: throw new IllegalStateException("isReady() not called"); case READY: - if (!_state.compareAndSet(OutputState.READY, OutputState.PENDING)) + if (!_state.compareAndSet(State.READY, State.PENDING)) continue; - if (_aggregate == null) - _aggregate = _channel.getByteBufferPool().acquire(getBufferSize(), _interceptor.isOptimizedForDirectBuffers()); + acquireBuffer(); BufferUtil.append(_aggregate, (byte)b); // Check if all written or full if (!complete && !BufferUtil.isFull(_aggregate)) { - if (!_state.compareAndSet(OutputState.PENDING, OutputState.ASYNC)) + if (!_state.compareAndSet(State.PENDING, State.ASYNC)) throw new IllegalStateException(); return; } @@ -673,6 +753,7 @@ public class HttpOutput extends ServletOutputStream implements Runnable case ERROR: throw new EofException(_onError); + case CLOSING: case CLOSED: throw new EofException("Closed"); @@ -810,7 +891,6 @@ public class HttpOutput extends ServletOutputStream implements Runnable _written += content.remaining(); write(content, true); - closed(); } /** @@ -966,7 +1046,7 @@ public class HttpOutput extends ServletOutputStream implements Runnable switch (_state.get()) { case OPEN: - if (!_state.compareAndSet(OutputState.OPEN, OutputState.PENDING)) + if (!_state.compareAndSet(State.OPEN, State.PENDING)) continue; break; @@ -974,6 +1054,7 @@ public class HttpOutput extends ServletOutputStream implements Runnable callback.failed(new EofException(_onError)); return; + case CLOSING: case CLOSED: callback.failed(new EofException("Closed")); return; @@ -1073,6 +1154,7 @@ public class HttpOutput extends ServletOutputStream implements Runnable _onError = null; _firstByteTimeStamp = -1; _flushed = 0; + _closeCallback = null; reopen(); } @@ -1082,7 +1164,6 @@ public class HttpOutput extends ServletOutputStream implements Runnable if (BufferUtil.hasContent(_aggregate)) BufferUtil.clear(_aggregate); _written = 0; - reopen(); } @Override @@ -1091,7 +1172,7 @@ public class HttpOutput extends ServletOutputStream implements Runnable if (!_channel.getState().isAsync()) throw new IllegalStateException("!ASYNC"); - if (_state.compareAndSet(OutputState.OPEN, OutputState.READY)) + if (_state.compareAndSet(State.OPEN, State.READY)) { _writeListener = writeListener; if (_channel.getState().onWritePossible()) @@ -1109,30 +1190,25 @@ public class HttpOutput extends ServletOutputStream implements Runnable switch (_state.get()) { case OPEN: + case READY: + case ERROR: + case CLOSING: + case CLOSED: return true; case ASYNC: - if (!_state.compareAndSet(OutputState.ASYNC, OutputState.READY)) + if (!_state.compareAndSet(State.ASYNC, State.READY)) continue; return true; - case READY: - return true; - case PENDING: - if (!_state.compareAndSet(OutputState.PENDING, OutputState.UNREADY)) + if (!_state.compareAndSet(State.PENDING, State.UNREADY)) continue; return false; case UNREADY: return false; - case ERROR: - return true; - - case CLOSED: - return true; - default: throw new IllegalStateException(); } @@ -1144,12 +1220,13 @@ public class HttpOutput extends ServletOutputStream implements Runnable { while (true) { - OutputState state = _state.get(); + State state = _state.get(); if (_onError != null) { switch (state) { + case CLOSING: case CLOSED: case ERROR: { @@ -1158,7 +1235,7 @@ public class HttpOutput extends ServletOutputStream implements Runnable } default: { - if (_state.compareAndSet(state, OutputState.ERROR)) + if (_state.compareAndSet(state, State.ERROR)) { Throwable th = _onError; _onError = null; @@ -1234,16 +1311,16 @@ public class HttpOutput extends ServletOutputStream implements Runnable { while (true) { - OutputState last = _state.get(); + State last = _state.get(); switch (last) { case PENDING: - if (!_state.compareAndSet(OutputState.PENDING, OutputState.ASYNC)) + if (!_state.compareAndSet(State.PENDING, State.ASYNC)) continue; break; case UNREADY: - if (!_state.compareAndSet(OutputState.UNREADY, OutputState.READY)) + if (!_state.compareAndSet(State.UNREADY, State.READY)) continue; if (_last) closed(); diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index 76388346ccb..a57a550d80b 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -759,10 +759,18 @@ public class Request implements HttpServletRequest } /** - * @return The current {@link Context context} used for this request, or null if {@link #setContext} has not yet been called. + * @return The current {@link Context context} used for this error handling for this request. If the request is asynchronous, + * then it is the context that called async. Otherwise it is the last non-null context passed to #setContext */ public Context getErrorContext() { + if (isAsyncStarted()) + { + ContextHandler handler = _channel.getState().getContextHandler(); + if (handler != null) + return handler.getServletContext(); + } + return _errorContext; } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java index 10fc3f33d25..04870c50949 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java @@ -18,18 +18,18 @@ package org.eclipse.jetty.server; +import java.io.Closeable; import java.io.IOException; import java.io.PrintWriter; import java.nio.channels.IllegalSelectorException; import java.util.Collection; import java.util.Collections; import java.util.EnumSet; -import java.util.List; +import java.util.Iterator; import java.util.ListIterator; import java.util.Locale; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Supplier; -import javax.servlet.RequestDispatcher; import javax.servlet.ServletOutputStream; import javax.servlet.ServletResponse; import javax.servlet.ServletResponseWrapper; @@ -57,9 +57,8 @@ import org.eclipse.jetty.http.MetaData; import org.eclipse.jetty.http.MimeTypes; import org.eclipse.jetty.http.PreEncodedHttpField; import org.eclipse.jetty.io.RuntimeIOException; -import org.eclipse.jetty.server.handler.ContextHandler; -import org.eclipse.jetty.server.handler.ErrorHandler; import org.eclipse.jetty.server.session.SessionHandler; +import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.URIUtil; import org.eclipse.jetty.util.log.Log; @@ -378,70 +377,35 @@ public class Response implements HttpServletResponse sendError(sc, null); } + /** + * Send an error response. + *

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

+ *
+ *
102
Send a partial PROCESSING response and allow additional responses
+ *
-1
Abort the HttpChannel and close the connection/stream
+ *
+ * @param code The error code + * @param message The message + * @throws IOException If an IO problem occurred sending the error response. + */ @Override public void sendError(int code, String message) throws IOException { if (isIncluding()) return; - if (isCommitted()) - { - if (LOG.isDebugEnabled()) - LOG.debug("Aborting on sendError on committed response {} {}", code, message); - code = -1; - } - else - resetBuffer(); - switch (code) { case -1: - _channel.abort(new IOException()); - return; - case 102: + _channel.abort(new IOException(message)); + break; + case HttpStatus.PROCESSING_102: sendProcessing(); - return; + break; default: + _channel.getState().sendError(code, message); break; } - - _outputType = OutputType.NONE; - setContentType(null); - setCharacterEncoding(null); - setHeader(HttpHeader.EXPIRES, null); - setHeader(HttpHeader.LAST_MODIFIED, null); - setHeader(HttpHeader.CACHE_CONTROL, null); - setHeader(HttpHeader.CONTENT_TYPE, null); - setHeader(HttpHeader.CONTENT_LENGTH, null); - - setStatus(code); - - Request request = _channel.getRequest(); - Throwable cause = (Throwable)request.getAttribute(Dispatcher.ERROR_EXCEPTION); - if (message == null) - { - _reason = HttpStatus.getMessage(code); - message = cause == null ? _reason : cause.toString(); - } - else - _reason = message; - - // If we are allowed to have a body, then produce the error page. - if (code != SC_NO_CONTENT && code != SC_NOT_MODIFIED && - code != SC_PARTIAL_CONTENT && code >= SC_OK) - { - ContextHandler.Context context = request.getErrorContext(); - ContextHandler contextHandler = context == null ? _channel.getState().getContextHandler() : context.getContextHandler(); - request.setAttribute(RequestDispatcher.ERROR_STATUS_CODE, code); - request.setAttribute(RequestDispatcher.ERROR_MESSAGE, message); - request.setAttribute(RequestDispatcher.ERROR_REQUEST_URI, request.getRequestURI()); - request.setAttribute(RequestDispatcher.ERROR_SERVLET_NAME, request.getServletName()); - ErrorHandler errorHandler = ErrorHandler.getErrorHandler(_channel.getServer(), contextHandler); - if (errorHandler != null) - errorHandler.handle(null, request, request, this); - } - if (!request.isAsyncStarted()) - closeOutput(); } /** @@ -653,8 +617,11 @@ public class Response implements HttpServletResponse throw new IllegalArgumentException(); if (!isIncluding()) { + // Null the reason only if the status is different. This allows + // a specific reason to be sent with setStatusWithReason followed by sendError. + if (_status != sc) + _reason = null; _status = sc; - _reason = null; } } @@ -717,6 +684,11 @@ public class Response implements HttpServletResponse return _outputType == OutputType.STREAM; } + public boolean isWritingOrStreaming() + { + return isWriting() || isStreaming(); + } + @Override public PrintWriter getWriter() throws IOException { @@ -825,21 +797,15 @@ public class Response implements HttpServletResponse public void closeOutput() throws IOException { - switch (_outputType) - { - case WRITER: - _writer.close(); - if (!_out.isClosed()) - _out.close(); - break; - case STREAM: - if (!_out.isClosed()) - getOutputStream().close(); - break; - default: - if (!_out.isClosed()) - _out.close(); - } + if (_outputType == OutputType.WRITER) + _writer.close(); + if (!_out.isClosed()) + _out.close(); + } + + public void closeOutput(Callback callback) + { + _out.close((_outputType == OutputType.WRITER) ? _writer : _out, callback); } public long getLongContentLength() @@ -1029,19 +995,20 @@ public class Response implements HttpServletResponse @Override public void reset() { - reset(false); - } - - public void reset(boolean preserveCookies) - { - resetForForward(); _status = 200; _reason = null; + _out.resetBuffer(); + _outputType = OutputType.NONE; _contentLength = -1; + _contentType = null; + _mimeType = null; + _characterEncoding = null; + _encodingFrom = EncodingFrom.NOT_SET; - List cookies = preserveCookies ? _fields.getFields(HttpHeader.SET_COOKIE) : null; + // Clear all response headers _fields.clear(); + // recreate necessary connection related fields for (String value : _channel.getRequest().getHttpFields().getCSV(HttpHeader.CONNECTION, false)) { HttpHeaderValue cb = HttpHeaderValue.CACHE.get(value); @@ -1064,21 +1031,57 @@ public class Response implements HttpServletResponse } } - if (preserveCookies) - cookies.forEach(_fields::add); - else + // recreate session cookies + Request request = getHttpChannel().getRequest(); + HttpSession session = request.getSession(false); + if (session != null && session.isNew()) { - Request request = getHttpChannel().getRequest(); - HttpSession session = request.getSession(false); - if (session != null && session.isNew()) + SessionHandler sh = request.getSessionHandler(); + if (sh != null) { - SessionHandler sh = request.getSessionHandler(); - if (sh != null) - { - HttpCookie c = sh.getSessionCookie(session, request.getContextPath(), request.isSecure()); - if (c != null) - addCookie(c); - } + HttpCookie c = sh.getSessionCookie(session, request.getContextPath(), request.isSecure()); + if (c != null) + addCookie(c); + } + } + } + + public void resetContent() + { + _out.resetBuffer(); + _outputType = OutputType.NONE; + _contentLength = -1; + _contentType = null; + _mimeType = null; + _characterEncoding = null; + _encodingFrom = EncodingFrom.NOT_SET; + + // remove the content related response headers and keep all others + for (Iterator i = getHttpFields().iterator(); i.hasNext(); ) + { + HttpField field = i.next(); + if (field.getHeader() == null) + continue; + + switch (field.getHeader()) + { + case CONTENT_TYPE: + case CONTENT_LENGTH: + case CONTENT_ENCODING: + case CONTENT_LANGUAGE: + case CONTENT_RANGE: + case CONTENT_MD5: + case CONTENT_LOCATION: + case TRANSFER_ENCODING: + case CACHE_CONTROL: + case LAST_MODIFIED: + case EXPIRES: + case ETAG: + case DATE: + case VARY: + i.remove(); + continue; + default: } } } @@ -1093,6 +1096,7 @@ public class Response implements HttpServletResponse public void resetBuffer() { _out.resetBuffer(); + _out.reopen(); } public void setTrailers(Supplier trailers) @@ -1133,6 +1137,9 @@ public class Response implements HttpServletResponse @Override public boolean isCommitted() { + // If we are in sendError state, we pretend to be committed + if (_channel.isSendError()) + return true; return _channel.isCommitted(); } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java index 72ac6bf52b7..93e3ff024bb 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java @@ -485,10 +485,16 @@ public class Server extends HandlerWrapper implements Attributes if (HttpMethod.OPTIONS.is(request.getMethod()) || "*".equals(target)) { if (!HttpMethod.OPTIONS.is(request.getMethod())) + { + request.setHandled(true); response.sendError(HttpStatus.BAD_REQUEST_400); - handleOptions(request, response); - if (!request.isHandled()) - handle(target, request, request, response); + } + else + { + handleOptions(request, response); + if (!request.isHandled()) + handle(target, request, request, response); + } } else handle(target, request, request, response); diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java index e11f4047912..4297a04a031 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java @@ -1106,7 +1106,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu case UNAVAILABLE: baseRequest.setHandled(true); response.sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE); - return true; + return false; default: if ((DispatcherType.REQUEST.equals(dispatch) && baseRequest.isHandled())) return false; @@ -1141,8 +1141,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu if (oldContext != _scontext) { // check the target. - if (DispatcherType.REQUEST.equals(dispatch) || DispatcherType.ASYNC.equals(dispatch) || - DispatcherType.ERROR.equals(dispatch) && baseRequest.getHttpChannelState().isAsync()) + if (DispatcherType.REQUEST.equals(dispatch) || DispatcherType.ASYNC.equals(dispatch)) { if (_compactPath) target = URIUtil.compactPath(target); @@ -1279,29 +1278,11 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu if (new_context) requestInitialized(baseRequest, request); - switch (dispatch) + if (dispatch == DispatcherType.REQUEST && isProtectedTarget(target)) { - case REQUEST: - if (isProtectedTarget(target)) - { - response.sendError(HttpServletResponse.SC_NOT_FOUND); - baseRequest.setHandled(true); - return; - } - break; - - case ERROR: - // If this is already a dispatch to an error page, proceed normally - if (Boolean.TRUE.equals(baseRequest.getAttribute(Dispatcher.__ERROR_DISPATCH))) - break; - - // We can just call doError here. If there is no error page, then one will - // be generated. If there is an error page, then a RequestDispatcher will be - // used to route the request through appropriate filters etc. - doError(target, baseRequest, request, response); - return; - default: - break; + baseRequest.setHandled(true); + response.sendError(HttpServletResponse.SC_NOT_FOUND); + return; } nextHandle(target, baseRequest, request, response); diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java index 09a7738f868..14700779b34 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java @@ -19,28 +19,29 @@ package org.eclipse.jetty.server.handler; import java.io.IOException; +import java.io.OutputStreamWriter; import java.io.PrintWriter; -import java.io.StringWriter; import java.io.Writer; +import java.nio.BufferOverflowException; import java.nio.ByteBuffer; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; import java.util.List; import javax.servlet.RequestDispatcher; -import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import org.eclipse.jetty.http.HttpFields; import org.eclipse.jetty.http.HttpHeader; -import org.eclipse.jetty.http.HttpMethod; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.MimeTypes; import org.eclipse.jetty.http.QuotedQualityCSV; +import org.eclipse.jetty.io.ByteBufferOutputStream; import org.eclipse.jetty.server.Dispatcher; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.util.BufferUtil; +import org.eclipse.jetty.util.QuotedStringTokenizer; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; @@ -54,10 +55,13 @@ import org.eclipse.jetty.util.log.Logger; */ public class ErrorHandler extends AbstractHandler { + // TODO This classes API needs to be majorly refactored/cleanup in jetty-10 private static final Logger LOG = Log.getLogger(ErrorHandler.class); public static final String ERROR_PAGE = "org.eclipse.jetty.server.error_page"; + public static final String ERROR_CONTEXT = "org.eclipse.jetty.server.error_context"; boolean _showStacks = true; + boolean _disableStacks = false; boolean _showMessageInTitle = true; String _cacheControl = "must-revalidate,no-cache,no-store"; @@ -65,6 +69,19 @@ public class ErrorHandler extends AbstractHandler { } + public boolean errorPageForMethod(String method) + { + switch (method) + { + case "GET": + case "POST": + case "HEAD": + return true; + default: + return false; + } + } + /* * @see org.eclipse.jetty.server.server.Handler#handle(javax.servlet.http.HttpServletRequest, javax.servlet.http.HttpServletResponse, int) */ @@ -77,65 +94,14 @@ public class ErrorHandler extends AbstractHandler @Override public void doError(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException { - String method = request.getMethod(); - if (!HttpMethod.GET.is(method) && !HttpMethod.POST.is(method) && !HttpMethod.HEAD.is(method)) - { - baseRequest.setHandled(true); - return; - } + String cacheControl = getCacheControl(); + if (cacheControl != null) + response.setHeader(HttpHeader.CACHE_CONTROL.asString(), cacheControl); - if (this instanceof ErrorPageMapper) - { - String errorPage = ((ErrorPageMapper)this).getErrorPage(request); - if (errorPage != null) - { - String oldErrorPage = (String)request.getAttribute(ERROR_PAGE); - ContextHandler.Context context = baseRequest.getErrorContext(); - if (context == null) - context = ContextHandler.getCurrentContext(); - if (context == null) - { - LOG.warn("No ServletContext for error page {}", errorPage); - } - else if (oldErrorPage != null && oldErrorPage.equals(errorPage)) - { - LOG.warn("Error page loop {}", errorPage); - } - else - { - request.setAttribute(ERROR_PAGE, errorPage); - - Dispatcher dispatcher = (Dispatcher)context.getRequestDispatcher(errorPage); - try - { - if (LOG.isDebugEnabled()) - LOG.debug("error page dispatch {}->{}", errorPage, dispatcher); - if (dispatcher != null) - { - dispatcher.error(request, response); - return; - } - LOG.warn("No error page found " + errorPage); - } - catch (ServletException e) - { - LOG.warn(Log.EXCEPTION, e); - return; - } - } - } - else - { - if (LOG.isDebugEnabled()) - { - LOG.debug("No Error Page mapping for request({} {}) (using default)", request.getMethod(), request.getRequestURI()); - } - } - } - - if (_cacheControl != null) - response.setHeader(HttpHeader.CACHE_CONTROL.asString(), _cacheControl); - generateAcceptableResponse(baseRequest, request, response, response.getStatus(), baseRequest.getResponse().getReason()); + String message = (String)request.getAttribute(Dispatcher.ERROR_MESSAGE); + if (message == null) + message = baseRequest.getResponse().getReason(); + generateAcceptableResponse(baseRequest, request, response, response.getStatus(), message); } /** @@ -144,7 +110,7 @@ public class ErrorHandler extends AbstractHandler * acceptable to the user-agent. The Accept header is evaluated in * quality order and the method * {@link #generateAcceptableResponse(Request, HttpServletRequest, HttpServletResponse, int, String, String)} - * is called for each mimetype until {@link Request#isHandled()} is true.

+ * is called for each mimetype until the response is written to or committed.

* * @param baseRequest The base request * @param request The servlet request (may be wrapped) @@ -167,11 +133,10 @@ public class ErrorHandler extends AbstractHandler for (String mimeType : acceptable) { generateAcceptableResponse(baseRequest, request, response, code, message, mimeType); - if (response.isCommitted() || baseRequest.getResponse().isWriting() || baseRequest.getResponse().isStreaming()) + if (response.isCommitted() || baseRequest.getResponse().isWritingOrStreaming()) break; } } - baseRequest.getResponse().closeOutput(); } /** @@ -192,6 +157,7 @@ public class ErrorHandler extends AbstractHandler * @return A {@link Writer} if there is a known acceptable charset or null * @throws IOException if a Writer cannot be returned */ + @Deprecated protected Writer getAcceptableWriter(Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException { @@ -225,33 +191,132 @@ public class ErrorHandler extends AbstractHandler *

This method is called for each mime type in the users agent's * Accept header, until {@link Request#isHandled()} is true and a * response of the appropriate type is generated. + *

+ *

The default implementation handles "text/html", "text/*" and "*/*". + * The method can be overridden to handle other types. Implementations must + * immediate produce a response and may not be async. + *

* * @param baseRequest The base request * @param request The servlet request (may be wrapped) * @param response The response (may be wrapped) * @param code the http error code * @param message the http error message - * @param mimeType The mimetype to generate (may be */*or other wildcard) + * @param contentType The mimetype to generate (may be */*or other wildcard) * @throws IOException if a response cannot be generated */ - protected void generateAcceptableResponse(Request baseRequest, HttpServletRequest request, HttpServletResponse response, int code, String message, String mimeType) + protected void generateAcceptableResponse(Request baseRequest, HttpServletRequest request, HttpServletResponse response, int code, String message, String contentType) throws IOException { - switch (mimeType) + // We can generate an acceptable contentType, but can we generate an acceptable charset? + // TODO refactor this in jetty-10 to be done in the other calling loop + Charset charset = null; + List acceptable = baseRequest.getHttpFields().getQualityCSV(HttpHeader.ACCEPT_CHARSET); + if (!acceptable.isEmpty()) + { + for (String name : acceptable) + { + if ("*".equals(name)) + { + charset = StandardCharsets.UTF_8; + break; + } + + try + { + charset = Charset.forName(name); + } + catch (Exception e) + { + LOG.ignore(e); + } + } + if (charset == null) + return; + } + + MimeTypes.Type type; + switch (contentType) { case "text/html": case "text/*": case "*/*": + type = MimeTypes.Type.TEXT_HTML; + if (charset == null) + charset = StandardCharsets.ISO_8859_1; + break; + + case "text/json": + case "application/json": + type = MimeTypes.Type.TEXT_JSON; + if (charset == null) + charset = StandardCharsets.UTF_8; + break; + + case "text/plain": + type = MimeTypes.Type.TEXT_PLAIN; + if (charset == null) + charset = StandardCharsets.ISO_8859_1; + break; + + default: + return; + } + + // write into the response aggregate buffer and flush it asynchronously. + while (true) + { + try { - baseRequest.setHandled(true); - Writer writer = getAcceptableWriter(baseRequest, request, response); - if (writer != null) + // TODO currently the writer used here is of fixed size, so a large + // TODO error page may cause a BufferOverflow. In which case we try + // TODO again with stacks disabled. If it still overflows, it is + // TODO written without a body. + ByteBuffer buffer = baseRequest.getResponse().getHttpOutput().acquireBuffer(); + ByteBufferOutputStream out = new ByteBufferOutputStream(buffer); + PrintWriter writer = new PrintWriter(new OutputStreamWriter(out, charset)); + + switch (type) { - response.setContentType(MimeTypes.Type.TEXT_HTML.asString()); - handleErrorPage(request, writer, code, message); + case TEXT_HTML: + response.setContentType(MimeTypes.Type.TEXT_HTML.asString()); + response.setCharacterEncoding(charset.name()); + handleErrorPage(request, writer, code, message); + break; + case TEXT_JSON: + response.setContentType(contentType); + writeErrorJson(request, writer, code, message); + break; + case TEXT_PLAIN: + response.setContentType(MimeTypes.Type.TEXT_PLAIN.asString()); + response.setCharacterEncoding(charset.name()); + writeErrorPlain(request, writer, code, message); + break; + default: + throw new IllegalStateException(); } + + writer.flush(); + break; + } + catch (BufferOverflowException e) + { + LOG.warn("Error page too large: {} {} {}", code, message, request); + if (LOG.isDebugEnabled()) + LOG.warn(e); + baseRequest.getResponse().resetContent(); + if (!_disableStacks) + { + LOG.info("Disabling showsStacks for " + this); + _disableStacks = true; + continue; + } + break; } } + + // Do an asynchronous completion. + baseRequest.getHttpChannel().sendResponseAndComplete(); } protected void handleErrorPage(HttpServletRequest request, Writer writer, int code, String message) @@ -278,12 +343,13 @@ public class ErrorHandler extends AbstractHandler { writer.write("\n"); writer.write("Error "); - writer.write(Integer.toString(code)); - - if (_showMessageInTitle) + // TODO this code is duplicated in writeErrorPageMessage + String status = Integer.toString(code); + writer.write(status); + if (message != null && !message.equals(status)) { writer.write(' '); - write(writer, message); + writer.write(StringUtil.sanitizeXmlString(message)); } writer.write("\n"); } @@ -294,7 +360,7 @@ public class ErrorHandler extends AbstractHandler String uri = request.getRequestURI(); writeErrorPageMessage(request, writer, code, message, uri); - if (showStacks) + if (showStacks && !_disableStacks) writeErrorPageStacks(request, writer); Request.getBaseRequest(request).getHttpChannel().getHttpConfiguration() @@ -305,29 +371,97 @@ public class ErrorHandler extends AbstractHandler throws IOException { writer.write("

HTTP ERROR "); + String status = Integer.toString(code); + writer.write(status); + if (message != null && !message.equals(status)) + { + writer.write(' '); + writer.write(StringUtil.sanitizeXmlString(message)); + } + writer.write("

\n"); + writer.write("\n"); + htmlRow(writer, "URI", uri); + htmlRow(writer, "STATUS", status); + htmlRow(writer, "MESSAGE", message); + htmlRow(writer, "SERVLET", request.getAttribute(Dispatcher.ERROR_SERVLET_NAME)); + Throwable cause = (Throwable)request.getAttribute(Dispatcher.ERROR_EXCEPTION); + while (cause != null) + { + htmlRow(writer, "CAUSED BY", cause); + cause = cause.getCause(); + } + writer.write("
\n"); + } + + private void htmlRow(Writer writer, String tag, Object value) + throws IOException + { + writer.write(""); + writer.write(tag); + writer.write(":"); + if (value == null) + writer.write("-"); + else + writer.write(StringUtil.sanitizeXmlString(value.toString())); + writer.write("\n"); + } + + private void writeErrorPlain(HttpServletRequest request, PrintWriter writer, int code, String message) + { + writer.write("HTTP ERROR "); writer.write(Integer.toString(code)); - writer.write("\n

Problem accessing "); - write(writer, uri); - writer.write(". Reason:\n

    ");
-        write(writer, message);
-        writer.write("

"); + writer.write(' '); + writer.write(StringUtil.sanitizeXmlString(message)); + writer.write("\n"); + writer.printf("URI: %s%n", request.getRequestURI()); + writer.printf("STATUS: %s%n", code); + writer.printf("MESSAGE: %s%n", message); + writer.printf("SERVLET: %s%n", request.getAttribute(Dispatcher.ERROR_SERVLET_NAME)); + Throwable cause = (Throwable)request.getAttribute(Dispatcher.ERROR_EXCEPTION); + while (cause != null) + { + writer.printf("CAUSED BY %s%n", cause); + if (_showStacks && !_disableStacks) + cause.printStackTrace(writer); + cause = cause.getCause(); + } + } + + private void writeErrorJson(HttpServletRequest request, PrintWriter writer, int code, String message) + { + writer + .append("{\n") + .append(" url: \"").append(request.getRequestURI()).append("\",\n") + .append(" status: \"").append(Integer.toString(code)).append("\",\n") + .append(" message: ").append(QuotedStringTokenizer.quote(message)).append(",\n"); + Object servlet = request.getAttribute(Dispatcher.ERROR_SERVLET_NAME); + if (servlet != null) + writer.append("servlet: \"").append(servlet.toString()).append("\",\n"); + Throwable cause = (Throwable)request.getAttribute(Dispatcher.ERROR_EXCEPTION); + int c = 0; + while (cause != null) + { + writer.append(" cause").append(Integer.toString(c++)).append(": ") + .append(QuotedStringTokenizer.quote(cause.toString())).append(",\n"); + cause = cause.getCause(); + } + writer.append("}"); } protected void writeErrorPageStacks(HttpServletRequest request, Writer writer) throws IOException { Throwable th = (Throwable)request.getAttribute(RequestDispatcher.ERROR_EXCEPTION); - while (th != null) + if (_showStacks && th != null) { - writer.write("

Caused by:

");
-            StringWriter sw = new StringWriter();
-            PrintWriter pw = new PrintWriter(sw);
-            th.printStackTrace(pw);
-            pw.flush();
-            write(writer, sw.getBuffer().toString());
+            PrintWriter pw = writer instanceof PrintWriter ? (PrintWriter)writer : new PrintWriter(writer);
+            pw.write("
");
+            while (th != null)
+            {
+                th.printStackTrace(pw);
+                th = th.getCause();
+            }
             writer.write("
\n");
-
-            th = th.getCause();
         }
     }
 
diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ShutdownHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ShutdownHandler.java
index 24a1258b61d..ed5affa51c9 100644
--- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ShutdownHandler.java
+++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ShutdownHandler.java
@@ -197,8 +197,9 @@ public class ShutdownHandler extends HandlerWrapper
             connector.shutdown();
         }
 
-        response.sendError(200, "Connectors closed, commencing full shutdown");
         baseRequest.setHandled(true);
+        response.setStatus(200);
+        response.flushBuffer();
 
         final Server server = getServer();
         new Thread()
diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/AbstractHttpTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/AbstractHttpTest.java
index ce95a87b203..d554043205c 100644
--- a/jetty-server/src/test/java/org/eclipse/jetty/server/AbstractHttpTest.java
+++ b/jetty-server/src/test/java/org/eclipse/jetty/server/AbstractHttpTest.java
@@ -43,9 +43,7 @@ import static org.hamcrest.Matchers.is;
 
 public abstract class AbstractHttpTest
 {
-    private static final Set __noBodyCodes = new HashSet<>(Arrays.asList(new String[]{
-        "100", "101", "102", "204", "304"
-    }));
+    private static final Set __noBodyCodes = new HashSet<>(Arrays.asList("100", "101", "102", "204", "304"));
 
     protected static Server server;
     protected static ServerConnector connector;
@@ -87,10 +85,10 @@ public abstract class AbstractHttpTest
                 HttpTester.parseResponse(input, response);
 
                 if (httpVersion.is("HTTP/1.1") &&
-                        response.isComplete() &&
-                        response.get("content-length") == null &&
-                        response.get("transfer-encoding") == null &&
-                        !__noBodyCodes.contains(response.getStatus()))
+                    response.isComplete() &&
+                    response.get("content-length") == null &&
+                    response.get("transfer-encoding") == null &&
+                    !__noBodyCodes.contains(response.getStatus()))
                     assertThat("If HTTP/1.1 response doesn't contain transfer-encoding or content-length headers, " +
                         "it should contain connection:close", response.get("connection"), is("close"));
                 return response;
diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncCompletionTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncCompletionTest.java
new file mode 100644
index 00000000000..a14a6bcd09f
--- /dev/null
+++ b/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncCompletionTest.java
@@ -0,0 +1,221 @@
+//
+//  ========================================================================
+//  Copyright (c) 1995-2019 Mort Bay Consulting Pty. Ltd.
+//  ------------------------------------------------------------------------
+//  All rights reserved. This program and the accompanying materials
+//  are made available under the terms of the Eclipse Public License v1.0
+//  and Apache License v2.0 which accompanies this distribution.
+//
+//      The Eclipse Public License is available at
+//      http://www.eclipse.org/legal/epl-v10.html
+//
+//      The Apache License v2.0 is available at
+//      http://www.opensource.org/licenses/apache2.0.php
+//
+//  You may elect to redistribute this code under either of these licenses.
+//  ========================================================================
+//
+
+package org.eclipse.jetty.server;
+
+import java.io.IOException;
+import java.io.OutputStream;
+import java.net.Socket;
+import java.nio.ByteBuffer;
+import java.nio.channels.SelectionKey;
+import java.nio.channels.SocketChannel;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.Exchanger;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.stream.Stream;
+
+import org.eclipse.jetty.http.HttpCompliance;
+import org.eclipse.jetty.http.HttpTester;
+import org.eclipse.jetty.io.ChannelEndPoint;
+import org.eclipse.jetty.io.Connection;
+import org.eclipse.jetty.io.EndPoint;
+import org.eclipse.jetty.io.ManagedSelector;
+import org.eclipse.jetty.io.SocketChannelEndPoint;
+import org.eclipse.jetty.util.Callback;
+import org.eclipse.jetty.util.thread.Scheduler;
+import org.hamcrest.Matchers;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.is;
+
+/**
+ * Extended Server Tester.
+ */
+public class AsyncCompletionTest extends HttpServerTestFixture
+{
+    private static final Exchanger X = new Exchanger<>();
+    private static final AtomicBoolean COMPLETE = new AtomicBoolean();
+
+    private static class DelayedCallback extends Callback.Nested
+    {
+        private CompletableFuture _delay = new CompletableFuture<>();
+
+        public DelayedCallback(Callback callback)
+        {
+            super(callback);
+        }
+
+        @Override
+        public void succeeded()
+        {
+            _delay.complete(null);
+        }
+
+        @Override
+        public void failed(Throwable x)
+        {
+            _delay.completeExceptionally(x);
+        }
+
+        public void proceed()
+        {
+            try
+            {
+                _delay.get(10, TimeUnit.SECONDS);
+                getCallback().succeeded();
+            }
+            catch(Throwable th)
+            {
+                th.printStackTrace();
+                getCallback().failed(th);
+            }
+        }
+    }
+
+
+    @BeforeEach
+    public void init() throws Exception
+    {
+        COMPLETE.set(false);
+
+        startServer(new ServerConnector(_server, new HttpConnectionFactory()
+        {
+            @Override
+            public Connection newConnection(Connector connector, EndPoint endPoint)
+            {
+                return configure(new ExtendedHttpConnection(getHttpConfiguration(), connector, endPoint), connector, endPoint);
+            }
+        })
+        {
+            @Override
+            protected ChannelEndPoint newEndPoint(SocketChannel channel, ManagedSelector selectSet, SelectionKey key) throws IOException
+            {
+                return new ExtendedEndPoint(channel, selectSet, key, getScheduler());
+            }
+        });
+    }
+
+    private static class ExtendedEndPoint extends SocketChannelEndPoint
+    {
+        public ExtendedEndPoint(SocketChannel channel, ManagedSelector selector, SelectionKey key, Scheduler scheduler)
+        {
+            super(channel, selector, key, scheduler);
+        }
+
+        @Override
+        public void write(Callback callback, ByteBuffer... buffers) throws IllegalStateException
+        {
+            DelayedCallback delay = new DelayedCallback(callback);
+            super.write(delay, buffers);
+            try
+            {
+                X.exchange(delay);
+            }
+            catch (InterruptedException e)
+            {
+                throw new RuntimeException(e);
+            }
+        }
+    }
+
+    private static class ExtendedHttpConnection extends HttpConnection
+    {
+        public ExtendedHttpConnection(HttpConfiguration config, Connector connector, EndPoint endPoint)
+        {
+            super(config, connector, endPoint, HttpCompliance.RFC7230_LEGACY, false);
+        }
+
+        @Override
+        public void onCompleted()
+        {
+            COMPLETE.compareAndSet(false,true);
+            super.onCompleted();
+        }
+    }
+
+    // Tests from here use these parameters
+    public static Stream tests()
+    {
+        List tests = new ArrayList<>();
+        tests.add(new Object[]{new HelloWorldHandler(), 200, "Hello world"});
+        tests.add(new Object[]{new SendErrorHandler(499,"Test async sendError"), 499, "Test async sendError"});
+        return tests.stream().map(Arguments::of);
+    }
+
+    @ParameterizedTest
+    @MethodSource("tests")
+    public void testAsyncCompletion(Handler handler, int status, String message) throws Exception
+    {
+        configureServer(handler);
+
+        int base = _threadPool.getBusyThreads();
+        try (Socket client = newSocket(_serverURI.getHost(), _serverURI.getPort()))
+        {
+            OutputStream os = client.getOutputStream();
+
+            // write the request
+            os.write("GET / HTTP/1.0\r\n\r\n".getBytes(StandardCharsets.ISO_8859_1));
+            os.flush();
+
+            // The write should happen but the callback is delayed
+            HttpTester.Response response = HttpTester.parseResponse(client.getInputStream());
+            assertThat(response, Matchers.notNullValue());
+            assertThat(response.getStatus(), is(status));
+            String content = response.getContent();
+            assertThat(content, containsString(message));
+
+            // Check that a thread is held busy in write
+            assertThat(_threadPool.getBusyThreads(), Matchers.greaterThan(base));
+
+            // Getting the Delayed callback will free the thread
+            DelayedCallback delay = X.exchange(null, 10, TimeUnit.SECONDS);
+
+            // wait for threads to return to base level
+            long end = System.nanoTime() + TimeUnit.SECONDS.toNanos(10);
+            while(_threadPool.getBusyThreads() != base)
+            {
+                if (System.nanoTime() > end)
+                    throw new TimeoutException();
+                Thread.sleep(10);
+            }
+
+            // We are now asynchronously waiting!
+            assertThat(COMPLETE.get(), is(false));
+
+            // proceed with the completion
+            delay.proceed();
+
+            while(!COMPLETE.get())
+            {
+                if (System.nanoTime() > end)
+                    throw new TimeoutException();
+                Thread.sleep(10);
+            }
+        }
+    }
+}
diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java
index 0f5369036ae..7e396b072b2 100644
--- a/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java
+++ b/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java
@@ -30,7 +30,6 @@ import org.eclipse.jetty.http.HttpField;
 import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpTester;
 import org.eclipse.jetty.server.handler.AbstractHandler;
-import org.eclipse.jetty.server.handler.ErrorHandler;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
@@ -53,43 +52,6 @@ public class ErrorHandlerTest
         server = new Server();
         connector = new LocalConnector(server);
         server.addConnector(connector);
-        server.addBean(new ErrorHandler()
-        {
-            @Override
-            protected void generateAcceptableResponse(
-                Request baseRequest,
-                HttpServletRequest request,
-                HttpServletResponse response,
-                int code,
-                String message,
-                String mimeType) throws IOException
-            {
-                switch (mimeType)
-                {
-                    case "text/json":
-                    case "application/json":
-                    {
-                        baseRequest.setHandled(true);
-                        response.setContentType(mimeType);
-                        response.getWriter()
-                            .append("{")
-                            .append("code: \"").append(Integer.toString(code)).append("\",")
-                            .append("message: \"").append(message).append('"')
-                            .append("}");
-                        break;
-                    }
-                    case "text/plain":
-                    {
-                        baseRequest.setHandled(true);
-                        response.setContentType("text/plain");
-                        response.getOutputStream().print(response.getContentType());
-                        break;
-                    }
-                    default:
-                        super.generateAcceptableResponse(baseRequest, request, response, code, message, mimeType);
-                }
-            }
-        });
 
         server.setHandler(new AbstractHandler()
         {
diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpInputAsyncStateTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpInputAsyncStateTest.java
index 9c667a9b6c8..fcabf1acfe9 100644
--- a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpInputAsyncStateTest.java
+++ b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpInputAsyncStateTest.java
@@ -190,6 +190,10 @@ public class HttpInputAsyncStateTest
                     __history.add("COMPLETE");
                     break;
 
+                case READ_REGISTER:
+                    _state.getHttpChannel().onAsyncWaitForContent();
+                    break;
+
                 default:
                     fail("Bad Action: " + action);
             }
diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpManyWaysToAsyncCommitBadBehaviourTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpManyWaysToAsyncCommitBadBehaviourTest.java
deleted file mode 100644
index 336079e4874..00000000000
--- a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpManyWaysToAsyncCommitBadBehaviourTest.java
+++ /dev/null
@@ -1,133 +0,0 @@
-//
-//  ========================================================================
-//  Copyright (c) 1995-2019 Mort Bay Consulting Pty. Ltd.
-//  ------------------------------------------------------------------------
-//  All rights reserved. This program and the accompanying materials
-//  are made available under the terms of the Eclipse Public License v1.0
-//  and Apache License v2.0 which accompanies this distribution.
-//
-//      The Eclipse Public License is available at
-//      http://www.eclipse.org/legal/epl-v10.html
-//
-//      The Apache License v2.0 is available at
-//      http://www.opensource.org/licenses/apache2.0.php
-//
-//  You may elect to redistribute this code under either of these licenses.
-//  ========================================================================
-//
-
-package org.eclipse.jetty.server;
-
-import java.io.IOException;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.concurrent.BrokenBarrierException;
-import java.util.concurrent.CyclicBarrier;
-import java.util.concurrent.TimeUnit;
-import java.util.concurrent.TimeoutException;
-import java.util.stream.Stream;
-import javax.servlet.AsyncContext;
-import javax.servlet.DispatcherType;
-import javax.servlet.ServletException;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
-
-import org.eclipse.jetty.http.HttpTester;
-import org.eclipse.jetty.http.HttpVersion;
-import org.junit.jupiter.params.ParameterizedTest;
-import org.junit.jupiter.params.provider.Arguments;
-import org.junit.jupiter.params.provider.MethodSource;
-
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.is;
-
-//TODO: reset buffer tests
-//TODO: add protocol specific tests for connection: close and/or chunking
-
-public class HttpManyWaysToAsyncCommitBadBehaviourTest extends AbstractHttpTest
-{
-    private final String contextAttribute = getClass().getName() + ".asyncContext";
-
-    public static Stream httpVersions()
-    {
-        // boolean dispatch - if true we dispatch, otherwise we complete
-        final boolean DISPATCH = true;
-        final boolean COMPLETE = false;
-
-        List ret = new ArrayList<>();
-        ret.add(Arguments.of(HttpVersion.HTTP_1_0, DISPATCH));
-        ret.add(Arguments.of(HttpVersion.HTTP_1_0, COMPLETE));
-        ret.add(Arguments.of(HttpVersion.HTTP_1_1, DISPATCH));
-        ret.add(Arguments.of(HttpVersion.HTTP_1_1, COMPLETE));
-        return ret.stream();
-    }
-
-    @ParameterizedTest
-    @MethodSource("httpVersions")
-    public void testHandlerSetsHandledAndWritesSomeContent(HttpVersion httpVersion, boolean dispatch) throws Exception
-    {
-        server.setHandler(new SetHandledWriteSomeDataHandler(false, dispatch));
-        server.start();
-
-        HttpTester.Response response = executeRequest(httpVersion);
-
-        assertThat("response code is 500", response.getStatus(), is(500));
-    }
-
-    private class SetHandledWriteSomeDataHandler extends ThrowExceptionOnDemandHandler
-    {
-        private final boolean dispatch;
-
-        private SetHandledWriteSomeDataHandler(boolean throwException, boolean dispatch)
-        {
-            super(throwException);
-            this.dispatch = dispatch;
-        }
-
-        @Override
-        public void doNonErrorHandle(String target, Request baseRequest, final HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
-        {
-            final CyclicBarrier resumeBarrier = new CyclicBarrier(1);
-
-            if (baseRequest.getDispatcherType() == DispatcherType.ERROR)
-            {
-                response.sendError(500);
-                return;
-            }
-
-            if (request.getAttribute(contextAttribute) == null)
-            {
-                final AsyncContext asyncContext = baseRequest.startAsync();
-                new Thread(new Runnable()
-                {
-                    @Override
-                    public void run()
-                    {
-                        try
-                        {
-                            asyncContext.getResponse().getWriter().write("foobar");
-                            if (dispatch)
-                                asyncContext.dispatch();
-                            else
-                                asyncContext.complete();
-                            resumeBarrier.await(5, TimeUnit.SECONDS);
-                        }
-                        catch (IOException | TimeoutException | InterruptedException | BrokenBarrierException e)
-                        {
-                            e.printStackTrace();
-                        }
-                    }
-                }).run();
-            }
-            try
-            {
-                resumeBarrier.await(5, TimeUnit.SECONDS);
-            }
-            catch (InterruptedException | BrokenBarrierException | TimeoutException e)
-            {
-                e.printStackTrace();
-            }
-            throw new TestCommitException();
-        }
-    }
-}
diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpManyWaysToAsyncCommitTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpManyWaysToAsyncCommitTest.java
index ca5ebddc616..82d9b06bc3c 100644
--- a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpManyWaysToAsyncCommitTest.java
+++ b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpManyWaysToAsyncCommitTest.java
@@ -21,6 +21,9 @@ package org.eclipse.jetty.server;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
 import java.util.stream.Stream;
 import javax.servlet.AsyncContext;
 import javax.servlet.ServletException;
@@ -31,13 +34,16 @@ import javax.servlet.http.HttpServletResponse;
 import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpTester;
 import org.eclipse.jetty.http.HttpVersion;
+import org.eclipse.jetty.util.log.StacklessLogging;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
 
 import static org.eclipse.jetty.http.HttpFieldsMatchers.containsHeaderValue;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.nullValue;
 
 //TODO: reset buffer tests
@@ -51,51 +57,72 @@ public class HttpManyWaysToAsyncCommitTest extends AbstractHttpTest
         // boolean dispatch - if true we dispatch, otherwise we complete
         final boolean DISPATCH = true;
         final boolean COMPLETE = false;
+        final boolean IN_WAIT = true;
+        final boolean WHILE_DISPATCHED = false;
 
         List ret = new ArrayList<>();
-        ret.add(Arguments.of(HttpVersion.HTTP_1_0, DISPATCH));
-        ret.add(Arguments.of(HttpVersion.HTTP_1_1, DISPATCH));
-        ret.add(Arguments.of(HttpVersion.HTTP_1_0, COMPLETE));
-        ret.add(Arguments.of(HttpVersion.HTTP_1_1, COMPLETE));
+        ret.add(Arguments.of(HttpVersion.HTTP_1_0, DISPATCH, IN_WAIT));
+        ret.add(Arguments.of(HttpVersion.HTTP_1_1, DISPATCH, IN_WAIT));
+        ret.add(Arguments.of(HttpVersion.HTTP_1_0, COMPLETE, IN_WAIT));
+        ret.add(Arguments.of(HttpVersion.HTTP_1_1, COMPLETE, IN_WAIT));
+        ret.add(Arguments.of(HttpVersion.HTTP_1_0, DISPATCH, WHILE_DISPATCHED));
+        ret.add(Arguments.of(HttpVersion.HTTP_1_1, DISPATCH, WHILE_DISPATCHED));
+        ret.add(Arguments.of(HttpVersion.HTTP_1_0, COMPLETE, WHILE_DISPATCHED));
+        ret.add(Arguments.of(HttpVersion.HTTP_1_1, COMPLETE, WHILE_DISPATCHED));
         return ret.stream();
     }
 
     @ParameterizedTest
     @MethodSource("httpVersion")
-    public void testHandlerDoesNotSetHandled(HttpVersion httpVersion, boolean dispatch) throws Exception
+    public void testHandlerDoesNotSetHandled(HttpVersion httpVersion, boolean dispatch, boolean inWait) throws Exception
     {
-        DoesNotSetHandledHandler handler = new DoesNotSetHandledHandler(false, dispatch);
+        DoesNotSetHandledHandler handler = new DoesNotSetHandledHandler(false, dispatch, inWait);
         server.setHandler(handler);
         server.start();
 
         HttpTester.Response response = executeRequest(httpVersion);
 
-        assertThat("response code", response.getStatus(), is(404));
-        assertThat("no exceptions", handler.failure(), is(nullValue()));
+        assertThat(response.getStatus(), is(404));
+        assertThat(handler.failure(), is(nullValue()));
     }
 
     @ParameterizedTest
     @MethodSource("httpVersion")
-    public void testHandlerDoesNotSetHandledAndThrow(HttpVersion httpVersion, boolean dispatch) throws Exception
+    public void testHandlerDoesNotSetHandledAndThrow(HttpVersion httpVersion, boolean dispatch, boolean inWait) throws Exception
     {
-        DoesNotSetHandledHandler handler = new DoesNotSetHandledHandler(true, dispatch);
+        DoesNotSetHandledHandler handler = new DoesNotSetHandledHandler(true, dispatch, inWait);
         server.setHandler(handler);
         server.start();
 
-        HttpTester.Response response = executeRequest(httpVersion);
+        HttpTester.Response response;
+        if (inWait)
+        {
+            // exception thrown and handled before any async processing
+            response = executeRequest(httpVersion);
+        }
+        else
+        {
+            // exception thrown after async processing, so cannot be handled
+            try (StacklessLogging log = new StacklessLogging(HttpChannelState.class))
+            {
+                response = executeRequest(httpVersion);
+            }
+        }
 
-        assertThat("response code", response.getStatus(), is(500));
-        assertThat("no exceptions", handler.failure(), is(nullValue()));
+        assertThat(response.getStatus(), is(500));
+        assertThat(handler.failure(), is(nullValue()));
     }
 
     private class DoesNotSetHandledHandler extends ThrowExceptionOnDemandHandler
     {
         private final boolean dispatch;
+        private final boolean inWait;
 
-        private DoesNotSetHandledHandler(boolean throwException, boolean dispatch)
+        private DoesNotSetHandledHandler(boolean throwException, boolean dispatch, boolean inWait)
         {
             super(throwException);
             this.dispatch = dispatch;
+            this.inWait = inWait;
         }
 
         @Override
@@ -105,17 +132,13 @@ public class HttpManyWaysToAsyncCommitTest extends AbstractHttpTest
             {
                 final AsyncContext asyncContext = baseRequest.startAsync();
                 request.setAttribute(contextAttribute, asyncContext);
-                new Thread(new Runnable()
+                runAsync(baseRequest, inWait, () ->
                 {
-                    @Override
-                    public void run()
-                    {
-                        if (dispatch)
-                            asyncContext.dispatch();
-                        else
-                            asyncContext.complete();
-                    }
-                }).run();
+                    if (dispatch)
+                        asyncContext.dispatch();
+                    else
+                        asyncContext.complete();
+                });
             }
             super.doNonErrorHandle(target, baseRequest, request, response);
         }
@@ -123,42 +146,57 @@ public class HttpManyWaysToAsyncCommitTest extends AbstractHttpTest
 
     @ParameterizedTest
     @MethodSource("httpVersion")
-    public void testHandlerSetsHandledTrueOnly(HttpVersion httpVersion, boolean dispatch) throws Exception
+    public void testHandlerSetsHandledTrueOnly(HttpVersion httpVersion, boolean dispatch, boolean inWait) throws Exception
     {
-        OnlySetHandledHandler handler = new OnlySetHandledHandler(false, dispatch);
+        OnlySetHandledHandler handler = new OnlySetHandledHandler(false, dispatch, inWait);
         server.setHandler(handler);
         server.start();
 
         HttpTester.Response response = executeRequest(httpVersion);
 
-        assertThat("response code", response.getStatus(), is(200));
+        assertThat(response.getStatus(), is(200));
         if (httpVersion.is("HTTP/1.1"))
             assertThat(response, containsHeaderValue(HttpHeader.CONTENT_LENGTH, "0"));
-        assertThat("no exceptions", handler.failure(), is(nullValue()));
+        assertThat(handler.failure(), is(nullValue()));
     }
 
     @ParameterizedTest
     @MethodSource("httpVersion")
-    public void testHandlerSetsHandledTrueOnlyAndThrow(HttpVersion httpVersion, boolean dispatch) throws Exception
+    public void testHandlerSetsHandledTrueOnlyAndThrow(HttpVersion httpVersion, boolean dispatch, boolean inWait) throws Exception
     {
-        OnlySetHandledHandler handler = new OnlySetHandledHandler(true, dispatch);
+        OnlySetHandledHandler handler = new OnlySetHandledHandler(true, dispatch, inWait);
         server.setHandler(handler);
         server.start();
 
-        HttpTester.Response response = executeRequest(httpVersion);
+        HttpTester.Response response;
+        if (inWait)
+        {
+            // exception thrown and handled before any async processing
+            response = executeRequest(httpVersion);
+        }
+        else
+        {
+            // exception thrown after async processing, so cannot be handled
+            try (StacklessLogging log = new StacklessLogging(HttpChannelState.class))
+            {
+                response = executeRequest(httpVersion);
+            }
+        }
 
-        assertThat("response code", response.getStatus(), is(500));
-        assertThat("no exceptions", handler.failure(), is(nullValue()));
+        assertThat(response.getStatus(), is(500));
+        assertThat(handler.failure(), is(nullValue()));
     }
 
     private class OnlySetHandledHandler extends ThrowExceptionOnDemandHandler
     {
         private final boolean dispatch;
+        private final boolean inWait;
 
-        private OnlySetHandledHandler(boolean throwException, boolean dispatch)
+        private OnlySetHandledHandler(boolean throwException, boolean dispatch, boolean inWait)
         {
             super(throwException);
             this.dispatch = dispatch;
+            this.inWait = inWait;
         }
 
         @Override
@@ -168,17 +206,13 @@ public class HttpManyWaysToAsyncCommitTest extends AbstractHttpTest
             {
                 final AsyncContext asyncContext = baseRequest.startAsync();
                 request.setAttribute(contextAttribute, asyncContext);
-                new Thread(new Runnable()
+                runAsync(baseRequest, inWait, () ->
                 {
-                    @Override
-                    public void run()
-                    {
-                        if (dispatch)
-                            asyncContext.dispatch();
-                        else
-                            asyncContext.complete();
-                    }
-                }).run();
+                    if (dispatch)
+                        asyncContext.dispatch();
+                    else
+                        asyncContext.complete();
+                });
             }
             baseRequest.setHandled(true);
             super.doNonErrorHandle(target, baseRequest, request, response);
@@ -187,41 +221,55 @@ public class HttpManyWaysToAsyncCommitTest extends AbstractHttpTest
 
     @ParameterizedTest
     @MethodSource("httpVersion")
-    public void testHandlerSetsHandledAndWritesSomeContent(HttpVersion httpVersion, boolean dispatch) throws Exception
+    public void testHandlerSetsHandledAndWritesSomeContent(HttpVersion httpVersion, boolean dispatch, boolean inWait) throws Exception
     {
-        SetHandledWriteSomeDataHandler handler = new SetHandledWriteSomeDataHandler(false, dispatch);
+        SetHandledWriteSomeDataHandler handler = new SetHandledWriteSomeDataHandler(false, dispatch, inWait);
         server.setHandler(handler);
         server.start();
 
         HttpTester.Response response = executeRequest(httpVersion);
 
-        assertThat("response code", response.getStatus(), is(200));
+        assertThat(response.getStatus(), is(200));
         assertThat(response, containsHeaderValue(HttpHeader.CONTENT_LENGTH, "6"));
-        assertThat("no exceptions", handler.failure(), is(nullValue()));
+        assertThat(handler.failure(), is(nullValue()));
     }
 
     @ParameterizedTest
     @MethodSource("httpVersion")
-    public void testHandlerSetsHandledAndWritesSomeContentAndThrow(HttpVersion httpVersion, boolean dispatch) throws Exception
+    public void testHandlerSetsHandledAndWritesSomeContentAndThrow(HttpVersion httpVersion, boolean dispatch, boolean inWait) throws Exception
     {
-        SetHandledWriteSomeDataHandler handler = new SetHandledWriteSomeDataHandler(true, dispatch);
+        SetHandledWriteSomeDataHandler handler = new SetHandledWriteSomeDataHandler(true, dispatch, inWait);
         server.setHandler(handler);
         server.start();
+        HttpTester.Response response;
+        if (inWait)
+        {
+            // exception thrown and handled before any async processing
+            response = executeRequest(httpVersion);
+        }
+        else
+        {
+            // exception thrown after async processing, so cannot be handled
+            try (StacklessLogging log = new StacklessLogging(HttpChannelState.class))
+            {
+                response = executeRequest(httpVersion);
+            }
+        }
 
-        HttpTester.Response response = executeRequest(httpVersion);
-
-        assertThat("response code", response.getStatus(), is(500));
-        assertThat("no exceptions", handler.failure(), is(nullValue()));
+        assertThat(response.getStatus(), is(500));
+        assertThat(handler.failure(), is(nullValue()));
     }
 
     private class SetHandledWriteSomeDataHandler extends ThrowExceptionOnDemandHandler
     {
         private final boolean dispatch;
+        private final boolean inWait;
 
-        private SetHandledWriteSomeDataHandler(boolean throwException, boolean dispatch)
+        private SetHandledWriteSomeDataHandler(boolean throwException, boolean dispatch, boolean inWait)
         {
             super(throwException);
             this.dispatch = dispatch;
+            this.inWait = inWait;
         }
 
         @Override
@@ -231,25 +279,21 @@ public class HttpManyWaysToAsyncCommitTest extends AbstractHttpTest
             {
                 final AsyncContext asyncContext = baseRequest.startAsync();
                 request.setAttribute(contextAttribute, asyncContext);
-                new Thread(new Runnable()
+                runAsync(baseRequest, inWait, () ->
                 {
-                    @Override
-                    public void run()
+                    try
                     {
-                        try
-                        {
-                            asyncContext.getResponse().getWriter().write("foobar");
-                            if (dispatch)
-                                asyncContext.dispatch();
-                            else
-                                asyncContext.complete();
-                        }
-                        catch (IOException e)
-                        {
-                            markFailed(e);
-                        }
+                        asyncContext.getResponse().getWriter().write("foobar");
+                        if (dispatch)
+                            asyncContext.dispatch();
+                        else
+                            asyncContext.complete();
                     }
-                }).run();
+                    catch (IOException e)
+                    {
+                        markFailed(e);
+                    }
+                });
             }
             baseRequest.setHandled(true);
             super.doNonErrorHandle(target, baseRequest, request, response);
@@ -258,44 +302,55 @@ public class HttpManyWaysToAsyncCommitTest extends AbstractHttpTest
 
     @ParameterizedTest
     @MethodSource("httpVersion")
-    public void testHandlerExplicitFlush(HttpVersion httpVersion, boolean dispatch) throws Exception
+    public void testHandlerExplicitFlush(HttpVersion httpVersion, boolean dispatch, boolean inWait) throws Exception
     {
-        ExplicitFlushHandler handler = new ExplicitFlushHandler(false, dispatch);
+        ExplicitFlushHandler handler = new ExplicitFlushHandler(false, dispatch, inWait);
         server.setHandler(handler);
         server.start();
 
         HttpTester.Response response = executeRequest(httpVersion);
 
-        assertThat("response code", response.getStatus(), is(200));
-        assertThat("no exceptions", handler.failure(), is(nullValue()));
+        assertThat(response.getStatus(), is(200));
+        assertThat(handler.failure(), is(nullValue()));
         if (httpVersion.is("HTTP/1.1"))
             assertThat(response, containsHeaderValue(HttpHeader.TRANSFER_ENCODING, "chunked"));
     }
 
     @ParameterizedTest
     @MethodSource("httpVersion")
-    public void testHandlerExplicitFlushAndThrow(HttpVersion httpVersion, boolean dispatch) throws Exception
+    public void testHandlerExplicitFlushAndThrow(HttpVersion httpVersion, boolean dispatch, boolean inWait) throws Exception
     {
-        ExplicitFlushHandler handler = new ExplicitFlushHandler(true, dispatch);
+        ExplicitFlushHandler handler = new ExplicitFlushHandler(true, dispatch, inWait);
         server.setHandler(handler);
         server.start();
 
         HttpTester.Response response = executeRequest(httpVersion);
 
-        assertThat("response code", response.getStatus(), is(200));
-        assertThat("no exceptions", handler.failure(), is(nullValue()));
-        if (httpVersion.is("HTTP/1.1"))
-            assertThat(response, containsHeaderValue(HttpHeader.TRANSFER_ENCODING, "chunked"));
+        if (inWait)
+        {
+            // throw happens before flush
+            assertThat(response.getStatus(), is(500));
+        }
+        else
+        {
+            // flush happens before throw
+            assertThat(response.getStatus(), is(200));
+            if (httpVersion.is("HTTP/1.1"))
+                assertThat(response, containsHeaderValue(HttpHeader.TRANSFER_ENCODING, "chunked"));
+        }
+        assertThat(handler.failure(), is(nullValue()));
     }
 
     private class ExplicitFlushHandler extends ThrowExceptionOnDemandHandler
     {
         private final boolean dispatch;
+        private final boolean inWait;
 
-        private ExplicitFlushHandler(boolean throwException, boolean dispatch)
+        private ExplicitFlushHandler(boolean throwException, boolean dispatch, boolean inWait)
         {
             super(throwException);
             this.dispatch = dispatch;
+            this.inWait = inWait;
         }
 
         @Override
@@ -305,27 +360,23 @@ public class HttpManyWaysToAsyncCommitTest extends AbstractHttpTest
             {
                 final AsyncContext asyncContext = baseRequest.startAsync();
                 request.setAttribute(contextAttribute, asyncContext);
-                new Thread(new Runnable()
+                runAsync(baseRequest, inWait, () ->
                 {
-                    @Override
-                    public void run()
+                    try
                     {
-                        try
-                        {
-                            ServletResponse asyncContextResponse = asyncContext.getResponse();
-                            asyncContextResponse.getWriter().write("foobar");
-                            asyncContextResponse.flushBuffer();
-                            if (dispatch)
-                                asyncContext.dispatch();
-                            else
-                                asyncContext.complete();
-                        }
-                        catch (IOException e)
-                        {
-                            markFailed(e);
-                        }
+                        ServletResponse asyncContextResponse = asyncContext.getResponse();
+                        asyncContextResponse.getWriter().write("foobar");
+                        asyncContextResponse.flushBuffer();
+                        if (dispatch)
+                            asyncContext.dispatch();
+                        else
+                            asyncContext.complete();
                     }
-                }).run();
+                    catch (IOException e)
+                    {
+                        markFailed(e);
+                    }
+                });
             }
             baseRequest.setHandled(true);
             super.doNonErrorHandle(target, baseRequest, request, response);
@@ -334,44 +385,55 @@ public class HttpManyWaysToAsyncCommitTest extends AbstractHttpTest
 
     @ParameterizedTest
     @MethodSource("httpVersion")
-    public void testHandledAndFlushWithoutContent(HttpVersion httpVersion, boolean dispatch) throws Exception
+    public void testHandledAndFlushWithoutContent(HttpVersion httpVersion, boolean dispatch, boolean inWait) throws Exception
     {
-        SetHandledAndFlushWithoutContentHandler handler = new SetHandledAndFlushWithoutContentHandler(false, dispatch);
+        SetHandledAndFlushWithoutContentHandler handler = new SetHandledAndFlushWithoutContentHandler(false, dispatch, inWait);
         server.setHandler(handler);
         server.start();
 
         HttpTester.Response response = executeRequest(httpVersion);
 
-        assertThat("response code", response.getStatus(), is(200));
-        assertThat("no exceptions", handler.failure(), is(nullValue()));
+        assertThat(response.getStatus(), is(200));
+        assertThat(handler.failure(), is(nullValue()));
         if (httpVersion.is("HTTP/1.1"))
             assertThat(response, containsHeaderValue(HttpHeader.TRANSFER_ENCODING, "chunked"));
     }
 
     @ParameterizedTest
     @MethodSource("httpVersion")
-    public void testHandledAndFlushWithoutContentAndThrow(HttpVersion httpVersion, boolean dispatch) throws Exception
+    public void testHandledAndFlushWithoutContentAndThrow(HttpVersion httpVersion, boolean dispatch, boolean inWait) throws Exception
     {
-        SetHandledAndFlushWithoutContentHandler handler = new SetHandledAndFlushWithoutContentHandler(true, dispatch);
+        SetHandledAndFlushWithoutContentHandler handler = new SetHandledAndFlushWithoutContentHandler(true, dispatch, inWait);
         server.setHandler(handler);
         server.start();
 
         HttpTester.Response response = executeRequest(httpVersion);
 
-        assertThat("response code", response.getStatus(), is(200));
-        assertThat("no exceptions", handler.failure(), is(nullValue()));
-        if (httpVersion.is("HTTP/1.1"))
-            assertThat(response, containsHeaderValue(HttpHeader.TRANSFER_ENCODING, "chunked"));
+        if (inWait)
+        {
+            // throw happens before async behaviour, so is handled
+            assertThat(response.getStatus(), is(500));
+        }
+        else
+        {
+            assertThat(response.getStatus(), is(200));
+            if (httpVersion.is("HTTP/1.1"))
+                assertThat(response, containsHeaderValue(HttpHeader.TRANSFER_ENCODING, "chunked"));
+        }
+
+        assertThat(handler.failure(), is(nullValue()));
     }
 
     private class SetHandledAndFlushWithoutContentHandler extends ThrowExceptionOnDemandHandler
     {
         private final boolean dispatch;
+        private final boolean inWait;
 
-        private SetHandledAndFlushWithoutContentHandler(boolean throwException, boolean dispatch)
+        private SetHandledAndFlushWithoutContentHandler(boolean throwException, boolean dispatch, boolean inWait)
         {
             super(throwException);
             this.dispatch = dispatch;
+            this.inWait = inWait;
         }
 
         @Override
@@ -381,25 +443,21 @@ public class HttpManyWaysToAsyncCommitTest extends AbstractHttpTest
             {
                 final AsyncContext asyncContext = baseRequest.startAsync();
                 request.setAttribute(contextAttribute, asyncContext);
-                new Thread(new Runnable()
+                runAsync(baseRequest, inWait, () ->
                 {
-                    @Override
-                    public void run()
+                    try
                     {
-                        try
-                        {
-                            asyncContext.getResponse().flushBuffer();
-                            if (dispatch)
-                                asyncContext.dispatch();
-                            else
-                                asyncContext.complete();
-                        }
-                        catch (IOException e)
-                        {
-                            markFailed(e);
-                        }
+                        asyncContext.getResponse().flushBuffer();
+                        if (dispatch)
+                            asyncContext.dispatch();
+                        else
+                            asyncContext.complete();
                     }
-                }).run();
+                    catch (IOException e)
+                    {
+                        markFailed(e);
+                    }
+                });
             }
             baseRequest.setHandled(true);
             super.doNonErrorHandle(target, baseRequest, request, response);
@@ -408,16 +466,16 @@ public class HttpManyWaysToAsyncCommitTest extends AbstractHttpTest
 
     @ParameterizedTest
     @MethodSource("httpVersion")
-    public void testWriteFlushWriteMore(HttpVersion httpVersion, boolean dispatch) throws Exception
+    public void testWriteFlushWriteMore(HttpVersion httpVersion, boolean dispatch, boolean inWait) throws Exception
     {
-        WriteFlushWriteMoreHandler handler = new WriteFlushWriteMoreHandler(false, dispatch);
+        WriteFlushWriteMoreHandler handler = new WriteFlushWriteMoreHandler(false, dispatch, inWait);
         server.setHandler(handler);
         server.start();
 
         HttpTester.Response response = executeRequest(httpVersion);
 
-        assertThat("response code", response.getStatus(), is(200));
-        assertThat("no exceptions", handler.failure(), is(nullValue()));
+        assertThat(response.getStatus(), is(200));
+        assertThat(handler.failure(), is(nullValue()));
 
         // HTTP/1.0 does not do chunked.  it will just send content and close
         if (httpVersion.is("HTTP/1.1"))
@@ -426,28 +484,39 @@ public class HttpManyWaysToAsyncCommitTest extends AbstractHttpTest
 
     @ParameterizedTest
     @MethodSource("httpVersion")
-    public void testWriteFlushWriteMoreAndThrow(HttpVersion httpVersion, boolean dispatch) throws Exception
+    public void testWriteFlushWriteMoreAndThrow(HttpVersion httpVersion, boolean dispatch, boolean inWait) throws Exception
     {
-        WriteFlushWriteMoreHandler handler = new WriteFlushWriteMoreHandler(true, dispatch);
+        WriteFlushWriteMoreHandler handler = new WriteFlushWriteMoreHandler(true, dispatch, inWait);
         server.setHandler(handler);
         server.start();
 
         HttpTester.Response response = executeRequest(httpVersion);
 
-        assertThat("response code", response.getStatus(), is(200));
-        assertThat("no exceptions", handler.failure(), is(nullValue()));
-        if (httpVersion.is("HTTP/1.1"))
-            assertThat(response, containsHeaderValue(HttpHeader.TRANSFER_ENCODING, "chunked"));
+        if (inWait)
+        {
+            // The exception is thrown before we do any writing or async operations, so it delivered as onError and then
+            // dispatched.
+            assertThat(response.getStatus(), is(500));
+        }
+        else
+        {
+            assertThat(response.getStatus(), is(200));
+            if (httpVersion.is("HTTP/1.1"))
+                assertThat(response, containsHeaderValue(HttpHeader.TRANSFER_ENCODING, "chunked"));
+        }
+        assertThat(handler.failure(), is(nullValue()));
     }
 
     private class WriteFlushWriteMoreHandler extends ThrowExceptionOnDemandHandler
     {
         private final boolean dispatch;
+        private final boolean inWait;
 
-        private WriteFlushWriteMoreHandler(boolean throwException, boolean dispatch)
+        private WriteFlushWriteMoreHandler(boolean throwException, boolean dispatch, boolean inWait)
         {
             super(throwException);
             this.dispatch = dispatch;
+            this.inWait = inWait;
         }
 
         @Override
@@ -457,28 +526,24 @@ public class HttpManyWaysToAsyncCommitTest extends AbstractHttpTest
             {
                 final AsyncContext asyncContext = baseRequest.startAsync();
                 request.setAttribute(contextAttribute, asyncContext);
-                new Thread(new Runnable()
+                runAsync(baseRequest, inWait, () ->
                 {
-                    @Override
-                    public void run()
+                    try
                     {
-                        try
-                        {
-                            ServletResponse asyncContextResponse = asyncContext.getResponse();
-                            asyncContextResponse.getWriter().write("foo");
-                            asyncContextResponse.flushBuffer();
-                            asyncContextResponse.getWriter().write("bar");
-                            if (dispatch)
-                                asyncContext.dispatch();
-                            else
-                                asyncContext.complete();
-                        }
-                        catch (IOException e)
-                        {
-                            markFailed(e);
-                        }
+                        ServletResponse asyncContextResponse = asyncContext.getResponse();
+                        asyncContextResponse.getWriter().write("foo");
+                        asyncContextResponse.flushBuffer();
+                        asyncContextResponse.getWriter().write("bar");
+                        if (dispatch)
+                            asyncContext.dispatch();
+                        else
+                            asyncContext.complete();
                     }
-                }).run();
+                    catch (IOException e)
+                    {
+                        markFailed(e);
+                    }
+                });
             }
             baseRequest.setHandled(true);
             super.doNonErrorHandle(target, baseRequest, request, response);
@@ -487,47 +552,58 @@ public class HttpManyWaysToAsyncCommitTest extends AbstractHttpTest
 
     @ParameterizedTest
     @MethodSource("httpVersion")
-    public void testBufferOverflow(HttpVersion httpVersion, boolean dispatch) throws Exception
+    public void testBufferOverflow(HttpVersion httpVersion, boolean dispatch, boolean inWait) throws Exception
     {
-        OverflowHandler handler = new OverflowHandler(false, dispatch);
+        OverflowHandler handler = new OverflowHandler(false, dispatch, inWait);
         server.setHandler(handler);
         server.start();
 
         HttpTester.Response response = executeRequest(httpVersion);
 
-        assertThat("response code", response.getStatus(), is(200));
+        assertThat(response.getStatus(), is(200));
         assertThat(response.getContent(), is("foobar"));
         if (httpVersion.is("HTTP/1.1"))
             assertThat(response, containsHeaderValue(HttpHeader.TRANSFER_ENCODING, "chunked"));
-        assertThat("no exceptions", handler.failure(), is(nullValue()));
+        assertThat(handler.failure(), is(nullValue()));
     }
 
     @ParameterizedTest
     @MethodSource("httpVersion")
-    public void testBufferOverflowAndThrow(HttpVersion httpVersion, boolean dispatch) throws Exception
+    public void testBufferOverflowAndThrow(HttpVersion httpVersion, boolean dispatch, boolean inWait) throws Exception
     {
-        OverflowHandler handler = new OverflowHandler(true, dispatch);
+        OverflowHandler handler = new OverflowHandler(true, dispatch, inWait);
         server.setHandler(handler);
         server.start();
 
         HttpTester.Response response = executeRequest(httpVersion);
 
-        // Buffer size is too small, so the content is written directly producing a 200 response
-        assertThat("response code", response.getStatus(), is(200));
-        assertThat(response.getContent(), is("foobar"));
-        if (httpVersion.is("HTTP/1.1"))
-            assertThat(response, containsHeaderValue(HttpHeader.TRANSFER_ENCODING, "chunked"));
-        assertThat("no exceptions", handler.failure(), is(nullValue()));
+        // Buffer size smaller than content, so writing will commit response.
+        // If this happens before the exception is thrown we get a 200, else a 500 is produced
+        if (inWait)
+        {
+            assertThat(response.getStatus(), is(500));
+            assertThat(response.getContent(), containsString("TestCommitException: Thrown by test"));
+        }
+        else
+        {
+            assertThat(response.getStatus(), is(200));
+            assertThat(response.getContent(), is("foobar"));
+            if (httpVersion.is("HTTP/1.1"))
+                assertThat(response, containsHeaderValue(HttpHeader.TRANSFER_ENCODING, "chunked"));
+            assertThat(handler.failure(), is(nullValue()));
+        }
     }
 
     private class OverflowHandler extends ThrowExceptionOnDemandHandler
     {
         private final boolean dispatch;
+        private final boolean inWait;
 
-        private OverflowHandler(boolean throwException, boolean dispatch)
+        private OverflowHandler(boolean throwException, boolean dispatch, boolean inWait)
         {
             super(throwException);
             this.dispatch = dispatch;
+            this.inWait = inWait;
         }
 
         @Override
@@ -537,27 +613,23 @@ public class HttpManyWaysToAsyncCommitTest extends AbstractHttpTest
             {
                 final AsyncContext asyncContext = baseRequest.startAsync();
                 request.setAttribute(contextAttribute, asyncContext);
-                new Thread(new Runnable()
+                runAsync(baseRequest, inWait, () ->
                 {
-                    @Override
-                    public void run()
+                    try
                     {
-                        try
-                        {
-                            ServletResponse asyncContextResponse = asyncContext.getResponse();
-                            asyncContextResponse.setBufferSize(3);
-                            asyncContextResponse.getWriter().write("foobar");
-                            if (dispatch)
-                                asyncContext.dispatch();
-                            else
-                                asyncContext.complete();
-                        }
-                        catch (IOException e)
-                        {
-                            markFailed(e);
-                        }
+                        ServletResponse asyncContextResponse = asyncContext.getResponse();
+                        asyncContextResponse.setBufferSize(3);
+                        asyncContextResponse.getWriter().write("foobar");
+                        if (dispatch)
+                            asyncContext.dispatch();
+                        else
+                            asyncContext.complete();
                     }
-                }).run();
+                    catch (IOException e)
+                    {
+                        markFailed(e);
+                    }
+                });
             }
             baseRequest.setHandled(true);
             super.doNonErrorHandle(target, baseRequest, request, response);
@@ -566,45 +638,54 @@ public class HttpManyWaysToAsyncCommitTest extends AbstractHttpTest
 
     @ParameterizedTest
     @MethodSource("httpVersion")
-    public void testSetContentLengthAndWriteExactlyThatAmountOfBytes(HttpVersion httpVersion, boolean dispatch) throws Exception
+    public void testSetContentLengthAndWriteExactlyThatAmountOfBytes(HttpVersion httpVersion, boolean dispatch, boolean inWait) throws Exception
     {
-        SetContentLengthAndWriteThatAmountOfBytesHandler handler = new SetContentLengthAndWriteThatAmountOfBytesHandler(false, dispatch);
+        SetContentLengthAndWriteThatAmountOfBytesHandler handler = new SetContentLengthAndWriteThatAmountOfBytesHandler(false, dispatch, inWait);
         server.setHandler(handler);
         server.start();
 
         HttpTester.Response response = executeRequest(httpVersion);
 
-        assertThat("response code", response.getStatus(), is(200));
-        assertThat("response body", response.getContent(), is("foo"));
+        assertThat(response.getStatus(), is(200));
+        assertThat(response.getContent(), is("foo"));
         assertThat(response, containsHeaderValue(HttpHeader.CONTENT_LENGTH, "3"));
-        assertThat("no exceptions", handler.failure(), is(nullValue()));
+        assertThat(handler.failure(), is(nullValue()));
     }
 
     @ParameterizedTest
     @MethodSource("httpVersion")
-    public void testSetContentLengthAndWriteExactlyThatAmountOfBytesAndThrow(HttpVersion httpVersion, boolean dispatch) throws Exception
+    public void testSetContentLengthAndWriteExactlyThatAmountOfBytesAndThrow(HttpVersion httpVersion, boolean dispatch, boolean inWait) throws Exception
     {
-        SetContentLengthAndWriteThatAmountOfBytesHandler handler = new SetContentLengthAndWriteThatAmountOfBytesHandler(true, dispatch);
+        SetContentLengthAndWriteThatAmountOfBytesHandler handler = new SetContentLengthAndWriteThatAmountOfBytesHandler(true, dispatch, inWait);
         server.setHandler(handler);
         server.start();
 
         HttpTester.Response response = executeRequest(httpVersion);
 
-        //TODO: should we expect 500 here?
-        assertThat("response code", response.getStatus(), is(200));
-        assertThat("response body", response.getContent(), is("foo"));
-        assertThat(response, containsHeaderValue(HttpHeader.CONTENT_LENGTH, "3"));
-        assertThat("no exceptions", handler.failure(), is(nullValue()));
+        if (inWait)
+        {
+            // too late!
+            assertThat(response.getStatus(), is(500));
+        }
+        else
+        {
+            assertThat(response.getStatus(), is(200));
+            assertThat(response.getContent(), is("foo"));
+            assertThat(response, containsHeaderValue(HttpHeader.CONTENT_LENGTH, "3"));
+        }
+        assertThat(handler.failure(), is(nullValue()));
     }
 
     private class SetContentLengthAndWriteThatAmountOfBytesHandler extends ThrowExceptionOnDemandHandler
     {
         private final boolean dispatch;
+        private final boolean inWait;
 
-        private SetContentLengthAndWriteThatAmountOfBytesHandler(boolean throwException, boolean dispatch)
+        private SetContentLengthAndWriteThatAmountOfBytesHandler(boolean throwException, boolean dispatch, boolean inWait)
         {
             super(throwException);
             this.dispatch = dispatch;
+            this.inWait = inWait;
         }
 
         @Override
@@ -614,27 +695,23 @@ public class HttpManyWaysToAsyncCommitTest extends AbstractHttpTest
             {
                 final AsyncContext asyncContext = baseRequest.startAsync();
                 request.setAttribute(contextAttribute, asyncContext);
-                new Thread(new Runnable()
+                runAsync(baseRequest, inWait, () ->
                 {
-                    @Override
-                    public void run()
+                    try
                     {
-                        try
-                        {
-                            ServletResponse asyncContextResponse = asyncContext.getResponse();
-                            asyncContextResponse.setContentLength(3);
-                            asyncContextResponse.getWriter().write("foo");
-                            if (dispatch)
-                                asyncContext.dispatch();
-                            else
-                                asyncContext.complete();
-                        }
-                        catch (IOException e)
-                        {
-                            markFailed(e);
-                        }
+                        ServletResponse asyncContextResponse = asyncContext.getResponse();
+                        asyncContextResponse.setContentLength(3);
+                        asyncContextResponse.getWriter().write("foo");
+                        if (dispatch)
+                            asyncContext.dispatch();
+                        else
+                            asyncContext.complete();
                     }
-                }).run();
+                    catch (IOException e)
+                    {
+                        markFailed(e);
+                    }
+                });
             }
             baseRequest.setHandled(true);
             super.doNonErrorHandle(target, baseRequest, request, response);
@@ -643,46 +720,55 @@ public class HttpManyWaysToAsyncCommitTest extends AbstractHttpTest
 
     @ParameterizedTest
     @MethodSource("httpVersion")
-    public void testSetContentLengthAndWriteMoreBytes(HttpVersion httpVersion, boolean dispatch) throws Exception
+    public void testSetContentLengthAndWriteMoreBytes(HttpVersion httpVersion, boolean dispatch, boolean inWait) throws Exception
     {
-        SetContentLengthAndWriteMoreBytesHandler handler = new SetContentLengthAndWriteMoreBytesHandler(false, dispatch);
+        SetContentLengthAndWriteMoreBytesHandler handler = new SetContentLengthAndWriteMoreBytesHandler(false, dispatch, inWait);
         server.setHandler(handler);
         server.start();
 
         HttpTester.Response response = executeRequest(httpVersion);
 
-        assertThat("response code", response.getStatus(), is(200));
+        assertThat(response.getStatus(), is(200));
         // jetty truncates the body when content-length is reached.! This is correct and desired behaviour?
-        assertThat("response body", response.getContent(), is("foo"));
+        assertThat(response.getContent(), is("foo"));
         assertThat(response, containsHeaderValue(HttpHeader.CONTENT_LENGTH, "3"));
-        assertThat("no exceptions", handler.failure(), is(nullValue()));
+        assertThat(handler.failure(), is(nullValue()));
     }
 
     @ParameterizedTest
     @MethodSource("httpVersion")
-    public void testSetContentLengthAndWriteMoreAndThrow(HttpVersion httpVersion, boolean dispatch) throws Exception
+    public void testSetContentLengthAndWriteMoreAndThrow(HttpVersion httpVersion, boolean dispatch, boolean inWait) throws Exception
     {
-        SetContentLengthAndWriteMoreBytesHandler handler = new SetContentLengthAndWriteMoreBytesHandler(true, dispatch);
+        SetContentLengthAndWriteMoreBytesHandler handler = new SetContentLengthAndWriteMoreBytesHandler(true, dispatch, inWait);
         server.setHandler(handler);
         server.start();
 
         HttpTester.Response response = executeRequest(httpVersion);
 
-        // TODO: we throw before response is committed. should we expect 500?
-        assertThat("response code", response.getStatus(), is(200));
-        assertThat("response body", response.getContent(), is("foo"));
-        assertThat(response, containsHeaderValue(HttpHeader.CONTENT_LENGTH, "3"));
-        assertThat("no exceptions", handler.failure(), is(nullValue()));
+        if (inWait)
+        {
+            // too late!
+            assertThat(response.getStatus(), is(500));
+        }
+        else
+        {
+            assertThat(response.getStatus(), is(200));
+            assertThat(response.getContent(), is("foo"));
+            assertThat(response, containsHeaderValue(HttpHeader.CONTENT_LENGTH, "3"));
+        }
+        assertThat(handler.failure(), is(nullValue()));
     }
 
     private class SetContentLengthAndWriteMoreBytesHandler extends ThrowExceptionOnDemandHandler
     {
         private final boolean dispatch;
+        private final boolean inWait;
 
-        private SetContentLengthAndWriteMoreBytesHandler(boolean throwException, boolean dispatch)
+        private SetContentLengthAndWriteMoreBytesHandler(boolean throwException, boolean dispatch, boolean inWait)
         {
             super(throwException);
             this.dispatch = dispatch;
+            this.inWait = inWait;
         }
 
         @Override
@@ -692,27 +778,23 @@ public class HttpManyWaysToAsyncCommitTest extends AbstractHttpTest
             {
                 final AsyncContext asyncContext = baseRequest.startAsync();
                 request.setAttribute(contextAttribute, asyncContext);
-                new Thread(new Runnable()
+                runAsync(baseRequest, inWait, () ->
                 {
-                    @Override
-                    public void run()
+                    try
                     {
-                        try
-                        {
-                            ServletResponse asyncContextResponse = asyncContext.getResponse();
-                            asyncContextResponse.setContentLength(3);
-                            asyncContextResponse.getWriter().write("foobar");
-                            if (dispatch)
-                                asyncContext.dispatch();
-                            else
-                                asyncContext.complete();
-                        }
-                        catch (IOException e)
-                        {
-                            markFailed(e);
-                        }
+                        ServletResponse asyncContextResponse = asyncContext.getResponse();
+                        asyncContextResponse.setContentLength(3);
+                        asyncContextResponse.getWriter().write("foobar");
+                        if (dispatch)
+                            asyncContext.dispatch();
+                        else
+                            asyncContext.complete();
                     }
-                }).run();
+                    catch (IOException e)
+                    {
+                        markFailed(e);
+                    }
+                });
             }
             baseRequest.setHandled(true);
             super.doNonErrorHandle(target, baseRequest, request, response);
@@ -721,41 +803,50 @@ public class HttpManyWaysToAsyncCommitTest extends AbstractHttpTest
 
     @ParameterizedTest
     @MethodSource("httpVersion")
-    public void testWriteAndSetContentLength(HttpVersion httpVersion, boolean dispatch) throws Exception
+    public void testWriteAndSetContentLength(HttpVersion httpVersion, boolean dispatch, boolean inWait) throws Exception
     {
-        WriteAndSetContentLengthHandler handler = new WriteAndSetContentLengthHandler(false, dispatch);
+        WriteAndSetContentLengthHandler handler = new WriteAndSetContentLengthHandler(false, dispatch, inWait);
         server.setHandler(handler);
         server.start();
 
         HttpTester.Response response = executeRequest(httpVersion);
 
-        assertThat("response code", response.getStatus(), is(200));
-        assertThat("no exceptions", handler.failure(), is(nullValue()));
+        assertThat(response.getStatus(), is(200));
+        assertThat(handler.failure(), is(nullValue()));
         //TODO: jetty ignores setContentLength and sends transfer-encoding header. Correct?
     }
 
     @ParameterizedTest
     @MethodSource("httpVersion")
-    public void testWriteAndSetContentLengthAndThrow(HttpVersion httpVersion, boolean dispatch) throws Exception
+    public void testWriteAndSetContentLengthAndThrow(HttpVersion httpVersion, boolean dispatch, boolean inWait) throws Exception
     {
-        WriteAndSetContentLengthHandler handler = new WriteAndSetContentLengthHandler(true, dispatch);
+        WriteAndSetContentLengthHandler handler = new WriteAndSetContentLengthHandler(true, dispatch, inWait);
         server.setHandler(handler);
         server.start();
 
         HttpTester.Response response = executeRequest(httpVersion);
-
-        assertThat("response code", response.getStatus(), is(200));
-        assertThat("no exceptions", handler.failure(), is(nullValue()));
+        if (inWait)
+        {
+            // too late
+            assertThat(response.getStatus(), is(500));
+        }
+        else
+        {
+            assertThat(response.getStatus(), is(200));
+        }
+        assertThat(handler.failure(), is(nullValue()));
     }
 
     private class WriteAndSetContentLengthHandler extends ThrowExceptionOnDemandHandler
     {
         private final boolean dispatch;
+        private final boolean inWait;
 
-        private WriteAndSetContentLengthHandler(boolean throwException, boolean dispatch)
+        private WriteAndSetContentLengthHandler(boolean throwException, boolean dispatch, boolean inWait)
         {
             super(throwException);
             this.dispatch = dispatch;
+            this.inWait = inWait;
         }
 
         @Override
@@ -765,27 +856,23 @@ public class HttpManyWaysToAsyncCommitTest extends AbstractHttpTest
             {
                 final AsyncContext asyncContext = baseRequest.startAsync();
                 request.setAttribute(contextAttribute, asyncContext);
-                new Thread(new Runnable()
+                runAsync(baseRequest, inWait, () ->
                 {
-                    @Override
-                    public void run()
+                    try
                     {
-                        try
-                        {
-                            ServletResponse asyncContextResponse = asyncContext.getResponse();
-                            asyncContextResponse.getWriter().write("foo");
-                            asyncContextResponse.setContentLength(3); // This should commit the response
-                            if (dispatch)
-                                asyncContext.dispatch();
-                            else
-                                asyncContext.complete();
-                        }
-                        catch (IOException e)
-                        {
-                            markFailed(e);
-                        }
+                        ServletResponse asyncContextResponse = asyncContext.getResponse();
+                        asyncContextResponse.getWriter().write("foo");
+                        asyncContextResponse.setContentLength(3); // This should commit the response
+                        if (dispatch)
+                            asyncContext.dispatch();
+                        else
+                            asyncContext.complete();
                     }
-                }).run();
+                    catch (IOException e)
+                    {
+                        markFailed(e);
+                    }
+                });
             }
             baseRequest.setHandled(true);
             super.doNonErrorHandle(target, baseRequest, request, response);
@@ -794,42 +881,52 @@ public class HttpManyWaysToAsyncCommitTest extends AbstractHttpTest
 
     @ParameterizedTest
     @MethodSource("httpVersion")
-    public void testWriteAndSetContentLengthTooSmall(HttpVersion httpVersion, boolean dispatch) throws Exception
+    public void testWriteAndSetContentLengthTooSmall(HttpVersion httpVersion, boolean dispatch, boolean inWait) throws Exception
     {
-        WriteAndSetContentLengthTooSmallHandler handler = new WriteAndSetContentLengthTooSmallHandler(false, dispatch);
+        WriteAndSetContentLengthTooSmallHandler handler = new WriteAndSetContentLengthTooSmallHandler(false, dispatch, inWait);
         server.setHandler(handler);
         server.start();
 
         HttpTester.Response response = executeRequest(httpVersion);
 
-        // Setting a content-length too small throws an IllegalStateException
-        assertThat("response code", response.getStatus(), is(500));
-        assertThat("no exceptions", handler.failure(), is(nullValue()));
+        // Setting a content-length too small throws an IllegalStateException,
+        // but only in the async handler, which completes or dispatches anyway
+        assertThat(response.getStatus(), is(200));
+        assertThat(handler.failure(), not(is(nullValue())));
     }
 
     @ParameterizedTest
     @MethodSource("httpVersion")
-    public void testWriteAndSetContentLengthTooSmallAndThrow(HttpVersion httpVersion, boolean dispatch) throws Exception
+    public void testWriteAndSetContentLengthTooSmallAndThrow(HttpVersion httpVersion, boolean dispatch, boolean inWait) throws Exception
     {
-        WriteAndSetContentLengthTooSmallHandler handler = new WriteAndSetContentLengthTooSmallHandler(true, dispatch);
+        WriteAndSetContentLengthTooSmallHandler handler = new WriteAndSetContentLengthTooSmallHandler(true, dispatch, inWait);
         server.setHandler(handler);
         server.start();
 
-        HttpTester.Response response = executeRequest(httpVersion);
+        HttpTester.Response response;
+        try (StacklessLogging stackless = new StacklessLogging(HttpChannelState.class))
+        {
+            response = executeRequest(httpVersion);
+        }
 
-        // Setting a content-length too small throws an IllegalStateException
-        assertThat("response code", response.getStatus(), is(500));
-        assertThat("no exceptions", handler.failure(), is(nullValue()));
+        assertThat(response.getStatus(), is(500));
+
+        if (!inWait)
+            assertThat(handler.failure(), not(is(nullValue())));
+        else
+            assertThat(handler.failure(), is(nullValue()));
     }
 
     private class WriteAndSetContentLengthTooSmallHandler extends ThrowExceptionOnDemandHandler
     {
         private final boolean dispatch;
+        private final boolean inWait;
 
-        private WriteAndSetContentLengthTooSmallHandler(boolean throwException, boolean dispatch)
+        private WriteAndSetContentLengthTooSmallHandler(boolean throwException, boolean dispatch, boolean inWait)
         {
             super(throwException);
             this.dispatch = dispatch;
+            this.inWait = inWait;
         }
 
         @Override
@@ -839,30 +936,93 @@ public class HttpManyWaysToAsyncCommitTest extends AbstractHttpTest
             {
                 final AsyncContext asyncContext = baseRequest.startAsync();
                 request.setAttribute(contextAttribute, asyncContext);
-                new Thread(new Runnable()
+                runAsync(baseRequest, inWait, () ->
                 {
-                    @Override
-                    public void run()
+                    try
                     {
-                        try
-                        {
-                            ServletResponse asyncContextResponse = asyncContext.getResponse();
-                            asyncContextResponse.getWriter().write("foobar");
-                            asyncContextResponse.setContentLength(3);
-                            if (dispatch)
-                                asyncContext.dispatch();
-                            else
-                                asyncContext.complete();
-                        }
-                        catch (IOException e)
-                        {
-                            markFailed(e);
-                        }
+                        ServletResponse asyncContextResponse = asyncContext.getResponse();
+                        asyncContextResponse.getWriter().write("foobar");
+                        asyncContextResponse.setContentLength(3);
                     }
-                }).run();
+                    catch (Throwable e)
+                    {
+                        markFailed(e);
+                        if (dispatch)
+                            asyncContext.dispatch();
+                        else
+                            asyncContext.complete();
+                    }
+                });
             }
             baseRequest.setHandled(true);
             super.doNonErrorHandle(target, baseRequest, request, response);
         }
     }
+
+    private void runAsyncInAsyncWait(Request request, Runnable task)
+    {
+        server.getThreadPool().execute(() ->
+        {
+            long end = System.nanoTime() + TimeUnit.SECONDS.toNanos(10);
+            try
+            {
+                while (System.nanoTime() < end)
+                {
+                    switch (request.getHttpChannelState().getState())
+                    {
+                        case WAITING:
+                            task.run();
+                            return;
+
+                        case HANDLING:
+                            Thread.sleep(100);
+                            continue;
+
+                        default:
+                            request.getHttpChannel().abort(new IllegalStateException());
+                            return;
+                    }
+                }
+                request.getHttpChannel().abort(new TimeoutException());
+            }
+            catch (InterruptedException e)
+            {
+                e.printStackTrace();
+            }
+        });
+    }
+
+    private void runAsyncWhileDispatched(Runnable task)
+    {
+        CountDownLatch ran = new CountDownLatch(1);
+
+        server.getThreadPool().execute(() ->
+        {
+            try
+            {
+                task.run();
+            }
+            finally
+            {
+                ran.countDown();
+            }
+        });
+
+        try
+        {
+            ran.await(10, TimeUnit.SECONDS);
+        }
+        catch (InterruptedException e)
+        {
+            throw new RuntimeException(e);
+        }
+    }
+
+    private void runAsync(Request request, boolean inWait, Runnable task)
+    {
+        if (inWait)
+            runAsyncInAsyncWait(request, task);
+        else
+            runAsyncWhileDispatched(task);
+    }
 }
diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java
index 3102db8873d..fd687f3b7a5 100644
--- a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java
+++ b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java
@@ -65,13 +65,22 @@ import static org.junit.jupiter.api.Assertions.assertTrue;
 
 public abstract class HttpServerTestBase extends HttpServerTestFixture
 {
-    private static final String REQUEST1_HEADER = "POST / HTTP/1.0\n" + "Host: localhost\n" + "Content-Type: text/xml; charset=utf-8\n" + "Connection: close\n" + "Content-Length: ";
+    private static final String REQUEST1_HEADER = "POST / HTTP/1.0\n" +
+        "Host: localhost\n" +
+        "Content-Type: text/xml; charset=utf-8\n" +
+        "Connection: close\n" +
+        "Content-Length: ";
     private static final String REQUEST1_CONTENT = "\n" +
-            "\n" +
-            "";
+        "\n" +
+        "";
     private static final String REQUEST1 = REQUEST1_HEADER + REQUEST1_CONTENT.getBytes().length + "\n\n" + REQUEST1_CONTENT;
 
-    private static final String RESPONSE1 = "HTTP/1.1 200 OK\n" + "Content-Length: 13\n" + "Server: Jetty(" + Server.getVersion() + ")\n" + "\n" + "Hello world\n";
+    private static final String RESPONSE1 = "HTTP/1.1 200 OK\n" +
+        "Content-Length: 13\n" +
+        "Server: Jetty(" + Server.getVersion() + ")\n" +
+        "\n" +
+        "Hello world\n";
 
     // Break the request up into three pieces, splitting the header.
     private static final String FRAGMENT1 = REQUEST1.substring(0, 16);
@@ -104,7 +113,7 @@ public abstract class HttpServerTestBase extends HttpServerTestFixture
             "            73\n" +
             "        \n" +
             "    \n" +
-                "\n";
+            "\n";
     protected static final String RESPONSE2 =
         "HTTP/1.1 200 OK\n" +
             "Content-Type: text/xml;charset=iso-8859-1\n" +
@@ -143,9 +152,9 @@ public abstract class HttpServerTestBase extends HttpServerTestFixture
             OutputStream os = client.getOutputStream();
 
             os.write(("OPTIONS * HTTP/1.1\r\n" +
-                    "Host: " + _serverURI.getHost() + "\r\n" +
-                    "Connection: close\r\n" +
-                    "\r\n").getBytes(StandardCharsets.ISO_8859_1));
+                "Host: " + _serverURI.getHost() + "\r\n" +
+                "Connection: close\r\n" +
+                "\r\n").getBytes(StandardCharsets.ISO_8859_1));
             os.flush();
 
             // Read the response.
@@ -154,15 +163,20 @@ public abstract class HttpServerTestBase extends HttpServerTestFixture
             assertThat(response, Matchers.containsString("HTTP/1.1 200 OK"));
             assertThat(response, Matchers.containsString("Allow: GET"));
         }
+    }
 
+    @Test
+    public void testGETStar() throws Exception
+    {
+        configureServer(new OptionsHandler());
         try (Socket client = newSocket(_serverURI.getHost(), _serverURI.getPort()))
         {
             OutputStream os = client.getOutputStream();
 
             os.write(("GET * HTTP/1.1\r\n" +
-                    "Host: " + _serverURI.getHost() + "\r\n" +
-                    "Connection: close\r\n" +
-                    "\r\n").getBytes(StandardCharsets.ISO_8859_1));
+                "Host: " + _serverURI.getHost() + "\r\n" +
+                "Connection: close\r\n" +
+                "\r\n").getBytes(StandardCharsets.ISO_8859_1));
             os.flush();
 
             // Read the response.
@@ -434,27 +448,22 @@ public abstract class HttpServerTestBase extends HttpServerTestFixture
         try (Socket client = newSocket(_serverURI.getHost(), _serverURI.getPort()))
         {
             OutputStream os = client.getOutputStream();
-            //@checkstyle-disable-check : IllegalTokenText
-            os.write(("GET /R2 HTTP/1.1\015\012" +
-                "Host: localhost\015\012" +
-                "Transfer-Encoding: chunked\015\012" +
-                "Content-Type: text/plain\015\012" +
-                "Connection: close\015\012" +
-                "\015\012").getBytes());
-            //@checkstyle-enable-check : IllegalTokenText
+
+            os.write(("GET /R2 HTTP/1.1\r\n" +
+                "Host: localhost\r\n" +
+                "Transfer-Encoding: chunked\r\n" +
+                "Content-Type: text/plain\r\n" +
+                "Connection: close\r\n" +
+                "\r\n").getBytes());
             os.flush();
             Thread.sleep(1000);
             os.write(("5").getBytes());
             Thread.sleep(1000);
-            //@checkstyle-disable-check : IllegalTokenText
-            os.write(("\015\012").getBytes());
-            //@checkstyle-enable-check : IllegalTokenText
+            os.write(("\r\n").getBytes());
             os.flush();
             Thread.sleep(1000);
-            //@checkstyle-disable-check : IllegalTokenText
-            os.write(("ABCDE\015\012" +
-                "0;\015\012\015\012").getBytes());
-            //@checkstyle-enable-check : IllegalTokenText
+            os.write(("ABCDE\r\n" +
+                "0;\r\n\r\n").getBytes());
             os.flush();
 
             // Read the response.
@@ -472,14 +481,14 @@ public abstract class HttpServerTestBase extends HttpServerTestFixture
         {
             OutputStream os = client.getOutputStream();
             //@checkstyle-disable-check : IllegalTokenText
-            os.write(("GET /R2 HTTP/1.1\015\012" +
-                "Host: localhost\015\012" +
-                "Content-Length: 5\015\012" +
-                "Content-Type: text/plain\015\012" +
-                "Connection: close\015\012" +
-                "\015\012" +
-                "ABCDE\015\012" +
-                "\015\012"
+            os.write(("GET /R2 HTTP/1.1\r\n" +
+                "Host: localhost\r\n" +
+                "Content-Length: 5\r\n" +
+                "Content-Type: text/plain\r\n" +
+                "Connection: close\r\n" +
+                "\r\n" +
+                "ABCDE\r\n" +
+                "\r\n"
             //@checkstyle-enable-check : IllegalTokenText
             ).getBytes());
             os.flush();
@@ -1136,26 +1145,26 @@ public abstract class HttpServerTestBase extends HttpServerTestFixture
 
             //@checkstyle-disable-check : IllegalTokenText
             os.write((
-                "POST /R1 HTTP/1.1\015\012" +
+                "POST /R1 HTTP/1.1\r\n" +
                     "Host: " + _serverURI.getHost() + ":" + _serverURI.getPort() + "\r\n" +
                     "content-type: text/plain; charset=utf-8\r\n" +
                     "content-length: 10\r\n" +
-                    "\015\012" +
+                    "\r\n" +
                     "123456789\n" +
 
-                    "HEAD /R2 HTTP/1.1\015\012" +
-                    "Host: " + _serverURI.getHost() + ":" + _serverURI.getPort() + "\015\012" +
+                    "HEAD /R2 HTTP/1.1\r\n" +
+                    "Host: " + _serverURI.getHost() + ":" + _serverURI.getPort() + "\r\n" +
                     "content-type: text/plain; charset=utf-8\r\n" +
                     "content-length: 10\r\n" +
-                    "\015\012" +
+                    "\r\n" +
                     "ABCDEFGHI\n" +
 
-                    "POST /R3 HTTP/1.1\015\012" +
-                    "Host: " + _serverURI.getHost() + ":" + _serverURI.getPort() + "\015\012" +
+                    "POST /R3 HTTP/1.1\r\n" +
+                    "Host: " + _serverURI.getHost() + ":" + _serverURI.getPort() + "\r\n" +
                     "content-type: text/plain; charset=utf-8\r\n" +
                     "content-length: 10\r\n" +
-                    "Connection: close\015\012" +
-                    "\015\012" +
+                    "Connection: close\r\n" +
+                    "\r\n" +
                     "abcdefghi\n"
             //@checkstyle-enable-check : IllegalTokenText
             ).getBytes(StandardCharsets.ISO_8859_1));
@@ -1553,12 +1562,11 @@ public abstract class HttpServerTestBase extends HttpServerTestFixture
             {
                 try
                 {
-                    byte[] bytes = (
-                        "GET / HTTP/1.1\r\n" +
-                            "Host: localhost\r\n" +
-                            "Content-Length: " + cl + "\r\n" +
-                            "\r\n" +
-                            content).getBytes(StandardCharsets.ISO_8859_1);
+                    byte[] bytes = ("GET / HTTP/1.1\r\n" +
+                        "Host: localhost\r\n" +
+                        "Content-Length: " + cl + "\r\n" +
+                        "\r\n" +
+                        content).getBytes(StandardCharsets.ISO_8859_1);
 
                     for (int i = 0; i < REQS; i++)
                     {
diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestFixture.java b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestFixture.java
index 96a17208dd5..bd009b30d24 100644
--- a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestFixture.java
+++ b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestFixture.java
@@ -40,7 +40,8 @@ import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 
 public class HttpServerTestFixture
-{    // Useful constants
+{
+    // Useful constants
     protected static final long PAUSE = 10L;
     protected static final int LOOPS = 50;
 
@@ -186,6 +187,31 @@ public class HttpServerTestFixture
         }
     }
 
+
+    protected static class SendErrorHandler extends AbstractHandler
+    {
+        private final int code;
+        private final String message;
+
+        public SendErrorHandler()
+        {
+            this(500, null);
+        }
+
+        public SendErrorHandler(int code, String message)
+        {
+            this.code = code;
+            this.message = message;
+        }
+
+        @Override
+        public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
+        {
+            baseRequest.setHandled(true);
+            response.sendError(code, message);
+        }
+    }
+
     protected static class ReadExactHandler extends AbstractHandler.ErrorDispatchHandler
     {
         private int expected;
diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/LocalAsyncContextTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/LocalAsyncContextTest.java
index 81c89ae2331..001b7438437 100644
--- a/jetty-server/src/test/java/org/eclipse/jetty/server/LocalAsyncContextTest.java
+++ b/jetty-server/src/test/java/org/eclipse/jetty/server/LocalAsyncContextTest.java
@@ -32,6 +32,8 @@ import javax.servlet.http.HttpServletResponse;
 
 import org.eclipse.jetty.server.handler.HandlerWrapper;
 import org.eclipse.jetty.server.session.SessionHandler;
+import org.eclipse.jetty.util.log.Log;
+import org.eclipse.jetty.util.log.Logger;
 import org.hamcrest.Matchers;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
@@ -42,6 +44,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals;
 
 public class LocalAsyncContextTest
 {
+    public static final Logger LOG = Log.getLogger(LocalAsyncContextTest.class);
     protected Server _server;
     protected SuspendHandler _handler;
     protected Connector _connector;
@@ -232,6 +235,7 @@ public class LocalAsyncContextTest
 
     private synchronized String process(String content) throws Exception
     {
+        LOG.debug("TEST process: {}", content);
         reset();
         String request = "GET / HTTP/1.1\r\n" +
             "Host: localhost\r\n" +
@@ -305,6 +309,7 @@ public class LocalAsyncContextTest
         @Override
         public void handle(String target, final Request baseRequest, final HttpServletRequest request, final HttpServletResponse response) throws IOException, ServletException
         {
+            LOG.debug("handle {} {}", baseRequest.getDispatcherType(), baseRequest);
             if (DispatcherType.REQUEST.equals(baseRequest.getDispatcherType()))
             {
                 if (_read > 0)
diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java
index a872ebdc824..4cb4f63a48a 100644
--- a/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java
+++ b/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java
@@ -35,6 +35,8 @@ import java.util.Enumeration;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Locale;
+import java.util.stream.Stream;
+import javax.servlet.RequestDispatcher;
 import javax.servlet.ServletException;
 import javax.servlet.ServletOutputStream;
 import javax.servlet.http.Cookie;
@@ -70,6 +72,8 @@ import org.hamcrest.Matchers;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
 
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.hamcrest.MatcherAssert.assertThat;
@@ -664,65 +668,41 @@ public class ResponseTest
         assertEquals("foo/bar; other=pq charset=utf-8 other=xyz;charset=utf-16", response.getContentType());
     }
 
-    @Test
-    public void testStatusCodes() throws Exception
+    public static Stream sendErrorTestCodes()
     {
-        Response response = getResponse();
-
-        response.sendError(404);
-        assertEquals(404, response.getStatus());
-        assertEquals("Not Found", response.getReason());
-
-        response = getResponse();
-
-        response.sendError(500, "Database Error");
-        assertEquals(500, response.getStatus());
-        assertEquals("Database Error", response.getReason());
-        assertEquals("must-revalidate,no-cache,no-store", response.getHeader(HttpHeader.CACHE_CONTROL.asString()));
-
-        response = getResponse();
-
-        response.setStatus(200);
-        assertEquals(200, response.getStatus());
-        assertEquals(null, response.getReason());
-
-        response = getResponse();
-
-        response.sendError(406, "Super Nanny");
-        assertEquals(406, response.getStatus());
-        assertEquals("Super Nanny", response.getReason());
-        assertEquals("must-revalidate,no-cache,no-store", response.getHeader(HttpHeader.CACHE_CONTROL.asString()));
+        List data = new ArrayList<>();
+        data.add(new Object[]{404, null, "Not Found"});
+        data.add(new Object[]{500, "Database Error", "Database Error"});
+        data.add(new Object[]{406, "Super Nanny", "Super Nanny"});
+        return data.stream();
     }
 
-    @Test
-    public void testStatusCodesNoErrorHandler() throws Exception
+    @ParameterizedTest
+    @MethodSource(value = "sendErrorTestCodes")
+    public void testStatusCodes(int code, String message, String expectedMessage) throws Exception
+    {
+        Response response = getResponse();
+        assertThat(response.getHttpChannel().getState().handling(), is(HttpChannelState.Action.DISPATCH));
+
+        if (message == null)
+            response.sendError(code);
+        else
+            response.sendError(code, message);
+
+        assertTrue(response.getHttpOutput().isClosed());
+        assertEquals(code, response.getStatus());
+        assertEquals(null, response.getReason());
+        assertEquals(expectedMessage, response.getHttpChannel().getRequest().getAttribute(RequestDispatcher.ERROR_MESSAGE));
+        assertThat(response.getHttpChannel().getState().unhandle(), is(HttpChannelState.Action.SEND_ERROR));
+        assertThat(response.getHttpChannel().getState().unhandle(), is(HttpChannelState.Action.COMPLETE));
+    }
+
+    @ParameterizedTest
+    @MethodSource(value = "sendErrorTestCodes")
+    public void testStatusCodesNoErrorHandler(int code, String message, String expectedMessage) throws Exception
     {
         _server.removeBean(_server.getBean(ErrorHandler.class));
-        Response response = getResponse();
-
-        response.sendError(404);
-        assertEquals(404, response.getStatus());
-        assertEquals("Not Found", response.getReason());
-
-        response = getResponse();
-
-        response.sendError(500, "Database Error");
-        assertEquals(500, response.getStatus());
-        assertEquals("Database Error", response.getReason());
-        assertThat(response.getHeader(HttpHeader.CACHE_CONTROL.asString()), Matchers.nullValue());
-
-        response = getResponse();
-
-        response.setStatus(200);
-        assertEquals(200, response.getStatus());
-        assertEquals(null, response.getReason());
-
-        response = getResponse();
-
-        response.sendError(406, "Super Nanny");
-        assertEquals(406, response.getStatus());
-        assertEquals("Super Nanny", response.getReason());
-        assertThat(response.getHeader(HttpHeader.CACHE_CONTROL.asString()), Matchers.nullValue());
+        testStatusCodes(code, message, expectedMessage);
     }
 
     @Test
@@ -898,7 +878,7 @@ public class ResponseTest
         assertTrue(!response.isCommitted());
         assertTrue(!writer.checkError());
         writer.print("");
-        assertTrue(!writer.checkError());
+        // assertTrue(!writer.checkError());  // TODO check if this is correct? checkout does an open check and the print above closes
         assertTrue(response.isCommitted());
     }
 
@@ -1032,7 +1012,7 @@ public class ResponseTest
     }
 
     @Test
-    public void testCookiesWithReset() throws Exception
+    public void testResetContent() throws Exception
     {
         Response response = getResponse();
 
@@ -1048,9 +1028,27 @@ public class ResponseTest
         cookie2.setPath("/path");
         response.addCookie(cookie2);
 
-        //keep the cookies
-        response.reset(true);
+        response.setContentType("some/type");
+        response.setContentLength(3);
+        response.setHeader(HttpHeader.EXPIRES,"never");
 
+        response.setHeader("SomeHeader", "SomeValue");
+
+        response.getOutputStream();
+
+        // reset the content
+        response.resetContent();
+
+        // check content is nulled
+        assertThat(response.getContentType(), nullValue());
+        assertThat(response.getContentLength(), is(-1L));
+        assertThat(response.getHeader(HttpHeader.EXPIRES.asString()), nullValue());
+        response.getWriter();
+
+        // check arbitrary header still set
+        assertThat(response.getHeader("SomeHeader"), is("SomeValue"));
+
+        // check cookies are still there
         Enumeration set = response.getHttpFields().getValues("Set-Cookie");
 
         assertNotNull(set);
diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/NcsaRequestLogTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/NcsaRequestLogTest.java
index 6964b7c9ef9..e854f3f44f1 100644
--- a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/NcsaRequestLogTest.java
+++ b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/NcsaRequestLogTest.java
@@ -37,6 +37,7 @@ import org.eclipse.jetty.server.AbstractNCSARequestLog;
 import org.eclipse.jetty.server.CustomRequestLog;
 import org.eclipse.jetty.server.Handler;
 import org.eclipse.jetty.server.HttpChannel;
+import org.eclipse.jetty.server.HttpChannelState;
 import org.eclipse.jetty.server.LocalConnector;
 import org.eclipse.jetty.server.Request;
 import org.eclipse.jetty.server.RequestLog;
@@ -44,6 +45,7 @@ import org.eclipse.jetty.server.Response;
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.util.BlockingArrayQueue;
 import org.eclipse.jetty.util.component.AbstractLifeCycle;
+import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.StacklessLogging;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Assertions;
@@ -390,7 +392,7 @@ public class NcsaRequestLogTest
             data.add(new Object[]{logType, new IOExceptionPartialHandler(), "/ioex", "\"GET /ioex HTTP/1.0\" 200"});
             data.add(new Object[]{logType, new RuntimeExceptionHandler(), "/rtex", "\"GET /rtex HTTP/1.0\" 500"});
             data.add(new Object[]{logType, new BadMessageHandler(), "/bad", "\"GET /bad HTTP/1.0\" 499"});
-            data.add(new Object[]{logType, new AbortHandler(), "/bad", "\"GET /bad HTTP/1.0\" 488"});
+            data.add(new Object[]{logType, new AbortHandler(), "/bad", "\"GET /bad HTTP/1.0\" 500"});
             data.add(new Object[]{logType, new AbortPartialHandler(), "/bad", "\"GET /bad HTTP/1.0\" 200"});
         }
 
@@ -517,7 +519,9 @@ public class NcsaRequestLogTest
         startServer();
         makeRequest(requestPath);
 
-        expectedLogEntry = "\"GET " + requestPath + " HTTP/1.0\" 200";
+        // If we abort, we can't write a 200 error page
+        if (!(testHandler instanceof AbortHandler))
+            expectedLogEntry = expectedLogEntry.replaceFirst(" [1-9][0-9][0-9]", " 200");
         assertRequestLog(expectedLogEntry, _log);
     }
 
@@ -577,6 +581,10 @@ public class NcsaRequestLogTest
                         {
                             try
                             {
+                                while (baseRequest.getHttpChannel().getState().getState() != HttpChannelState.State.WAITING)
+                                {
+                                    Thread.sleep(10);
+                                }
                                 baseRequest.setHandled(false);
                                 testHandler.handle(target, baseRequest, request, response);
                                 if (!baseRequest.isHandled())
@@ -584,18 +592,21 @@ public class NcsaRequestLogTest
                             }
                             catch (BadMessageException bad)
                             {
-                                response.sendError(bad.getCode());
+                                response.sendError(bad.getCode(), bad.getReason());
                             }
                             catch (Exception e)
                             {
-                                response.sendError(500);
+                                response.sendError(500, e.toString());
                             }
                         }
-                        catch (Throwable th)
+                        catch (IOException | IllegalStateException th)
                         {
-                            throw new RuntimeException(th);
+                            Log.getLog().ignore(th);
+                        }
+                        finally
+                        {
+                            ac.complete();
                         }
-                        ac.complete();
                     });
                 }
             }
diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/SecuredRedirectHandlerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/SecuredRedirectHandlerTest.java
index 4dd9dfbfa1a..73d5193f262 100644
--- a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/SecuredRedirectHandlerTest.java
+++ b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/SecuredRedirectHandlerTest.java
@@ -266,6 +266,7 @@ public class SecuredRedirectHandlerTest
         {
             if (!"/".equals(target))
             {
+                baseRequest.setHandled(true);
                 response.sendError(404);
                 return;
             }
diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SniSslConnectionFactoryTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SniSslConnectionFactoryTest.java
index 5c68b9ef802..27c5d89251e 100644
--- a/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SniSslConnectionFactoryTest.java
+++ b/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SniSslConnectionFactoryTest.java
@@ -20,7 +20,6 @@ package org.eclipse.jetty.server.ssl;
 
 import java.io.File;
 import java.io.FileNotFoundException;
-import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.net.Socket;
@@ -42,6 +41,7 @@ import javax.net.ssl.SSLSocketFactory;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
+import org.eclipse.jetty.http.HttpTester;
 import org.eclipse.jetty.http.HttpVersion;
 import org.eclipse.jetty.io.Connection;
 import org.eclipse.jetty.io.EndPoint;
@@ -55,8 +55,8 @@ import org.eclipse.jetty.server.ServerConnector;
 import org.eclipse.jetty.server.SocketCustomizationListener;
 import org.eclipse.jetty.server.SslConnectionFactory;
 import org.eclipse.jetty.server.handler.AbstractHandler;
+import org.eclipse.jetty.server.handler.ErrorHandler;
 import org.eclipse.jetty.util.IO;
-import org.eclipse.jetty.util.Utf8StringBuilder;
 import org.eclipse.jetty.util.ssl.SslContextFactory;
 import org.hamcrest.Matchers;
 import org.junit.jupiter.api.AfterEach;
@@ -65,9 +65,8 @@ import org.junit.jupiter.api.Test;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsString;
-import static org.hamcrest.Matchers.startsWith;
+import static org.hamcrest.Matchers.is;
 import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertTrue;
 
 public class SniSslConnectionFactoryTest
 {
@@ -81,23 +80,23 @@ public class SniSslConnectionFactoryTest
     {
         _server = new Server();
 
-        HttpConfiguration http_config = new HttpConfiguration();
-        http_config.setSecureScheme("https");
-        http_config.setSecurePort(8443);
-        http_config.setOutputBufferSize(32768);
-        _httpsConfiguration = new HttpConfiguration(http_config);
+        HttpConfiguration httpConfig = new HttpConfiguration();
+        httpConfig.setSecureScheme("https");
+        httpConfig.setSecurePort(8443);
+        httpConfig.setOutputBufferSize(32768);
+        _httpsConfiguration = new HttpConfiguration(httpConfig);
         SecureRequestCustomizer src = new SecureRequestCustomizer();
         src.setSniHostCheck(true);
         _httpsConfiguration.addCustomizer(src);
-        _httpsConfiguration.addCustomizer((connector, httpConfig, request) ->
+        _httpsConfiguration.addCustomizer((connector, hc, request) ->
         {
             EndPoint endp = request.getHttpChannel().getEndPoint();
             if (endp instanceof SslConnection.DecryptedEndPoint)
             {
                 try
                 {
-                    SslConnection.DecryptedEndPoint ssl_endp = (SslConnection.DecryptedEndPoint)endp;
-                    SslConnection sslConnection = ssl_endp.getSslConnection();
+                    SslConnection.DecryptedEndPoint sslEndp = (SslConnection.DecryptedEndPoint)endp;
+                    SslConnection sslConnection = sslEndp.getSslConnection();
                     SSLEngine sslEngine = sslConnection.getSSLEngine();
                     SSLSession session = sslEngine.getSession();
                     for (Certificate c : session.getLocalCertificates())
@@ -224,6 +223,7 @@ public class SniSslConnectionFactoryTest
     public void testSameConnectionRequestsForManyDomains() throws Exception
     {
         start("src/test/resources/keystore_sni.p12");
+        _server.setErrorHandler(new ErrorHandler());
 
         SslContextFactory clientContextFactory = new SslContextFactory.Client(true);
         clientContextFactory.start();
@@ -246,8 +246,8 @@ public class SniSslConnectionFactoryTest
             output.flush();
 
             InputStream input = sslSocket.getInputStream();
-            String response = response(input);
-            assertTrue(response.startsWith("HTTP/1.1 200 "));
+            HttpTester.Response response = HttpTester.parseResponse(input);
+            assertThat(response.getStatus(), is(200));
 
             // Same socket, send a request for a different domain but same alias.
             request =
@@ -256,9 +256,8 @@ public class SniSslConnectionFactoryTest
                     "\r\n";
             output.write(request.getBytes(StandardCharsets.UTF_8));
             output.flush();
-
-            response = response(input);
-            assertTrue(response.startsWith("HTTP/1.1 200 "));
+            response = HttpTester.parseResponse(input);
+            assertThat(response.getStatus(), is(200));
 
             // Same socket, send a request for a different domain but different alias.
             request =
@@ -268,9 +267,9 @@ public class SniSslConnectionFactoryTest
             output.write(request.getBytes(StandardCharsets.UTF_8));
             output.flush();
 
-            response = response(input);
-            assertThat(response, startsWith("HTTP/1.1 400 "));
-            assertThat(response, containsString("Host does not match SNI"));
+            response = HttpTester.parseResponse(input);
+            assertThat(response.getStatus(), is(400));
+            assertThat(response.getContent(), containsString("Host does not match SNI"));
         }
         finally
         {
@@ -303,8 +302,8 @@ public class SniSslConnectionFactoryTest
             output.flush();
 
             InputStream input = sslSocket.getInputStream();
-            String response = response(input);
-            assertTrue(response.startsWith("HTTP/1.1 200 "));
+            HttpTester.Response response = HttpTester.parseResponse(input);
+            assertThat(response.getStatus(), is(200));
 
             // Now, on the same socket, send a request for a different valid domain.
             request =
@@ -314,8 +313,8 @@ public class SniSslConnectionFactoryTest
             output.write(request.getBytes(StandardCharsets.UTF_8));
             output.flush();
 
-            response = response(input);
-            assertTrue(response.startsWith("HTTP/1.1 200 "));
+            response = HttpTester.parseResponse(input);
+            assertThat(response.getStatus(), is(200));
 
             // Now make a request for an invalid domain for this connection.
             request =
@@ -325,9 +324,9 @@ public class SniSslConnectionFactoryTest
             output.write(request.getBytes(StandardCharsets.UTF_8));
             output.flush();
 
-            response = response(input);
-            assertTrue(response.startsWith("HTTP/1.1 400 "));
-            assertThat(response, Matchers.containsString("Host does not match SNI"));
+            response = HttpTester.parseResponse(input);
+            assertThat(response.getStatus(), is(400));
+            assertThat(response.getContent(), containsString("Host does not match SNI"));
         }
         finally
         {
@@ -335,22 +334,6 @@ public class SniSslConnectionFactoryTest
         }
     }
 
-    private String response(InputStream input) throws IOException
-    {
-        Utf8StringBuilder buffer = new Utf8StringBuilder();
-        int crlfs = 0;
-        while (true)
-        {
-            int read = input.read();
-            assertTrue(read >= 0);
-            buffer.append((byte)read);
-            crlfs = (read == '\r' || read == '\n') ? crlfs + 1 : 0;
-            if (crlfs == 4)
-                break;
-        }
-        return buffer.toString();
-    }
-
     private String getResponse(String host, String cn) throws Exception
     {
         String response = getResponse(host, host, cn);
diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncContextTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncContextTest.java
index 1ae360927b5..6cc49bd60da 100644
--- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncContextTest.java
+++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncContextTest.java
@@ -50,6 +50,7 @@ import org.junit.jupiter.api.Test;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.not;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 /**
@@ -478,7 +479,7 @@ public class AsyncContextTest
 
         assertThat("error servlet", responseBody, containsString("ERROR: /error"));
         assertThat("error servlet", responseBody, containsString("PathInfo= /500"));
-        assertThat("error servlet", responseBody, containsString("EXCEPTION: java.lang.RuntimeException: TEST"));
+        assertThat("error servlet", responseBody, not(containsString("EXCEPTION: ")));
     }
 
     private class DispatchingRunnable implements Runnable
@@ -552,7 +553,7 @@ public class AsyncContextTest
                     @Override
                     public void onTimeout(AsyncEvent event) throws IOException
                     {
-                        throw new RuntimeException("TEST");
+                        throw new RuntimeException("BAD EXPIRE");
                     }
 
                     @Override
diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncListenerTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncListenerTest.java
index 50fc3a3f2b1..6d6bb5824ed 100644
--- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncListenerTest.java
+++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncListenerTest.java
@@ -32,6 +32,7 @@ import javax.servlet.http.HttpServletResponse;
 
 import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.http.HttpTester;
+import org.eclipse.jetty.io.QuietException;
 import org.eclipse.jetty.server.LocalConnector;
 import org.eclipse.jetty.server.QuietServletException;
 import org.eclipse.jetty.server.Server;
@@ -42,6 +43,7 @@ import org.junit.jupiter.api.Test;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.not;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
 public class AsyncListenerTest
@@ -140,7 +142,7 @@ public class AsyncListenerTest
         test_StartAsync_Throw_OnError(event ->
         {
             HttpServletResponse response = (HttpServletResponse)event.getAsyncContext().getResponse();
-            response.sendError(HttpStatus.BAD_GATEWAY_502);
+            response.sendError(HttpStatus.BAD_GATEWAY_502, "Message!!!");
         });
         String httpResponse = connector.getResponse(
             "GET /ctx/path HTTP/1.1\r\n" +
@@ -148,7 +150,8 @@ public class AsyncListenerTest
                 "Connection: close\r\n" +
                 "\r\n");
         assertThat(httpResponse, containsString("HTTP/1.1 502 "));
-        assertThat(httpResponse, containsString(TestRuntimeException.class.getName()));
+        assertThat(httpResponse, containsString("Message!!!"));
+        assertThat(httpResponse, not(containsString(TestRuntimeException.class.getName())));
     }
 
     @Test
@@ -191,7 +194,7 @@ public class AsyncListenerTest
             protected void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
             {
                 AsyncContext asyncContext = request.startAsync();
-                asyncContext.setTimeout(0);
+                asyncContext.setTimeout(10000);
                 asyncContext.addListener(new AsyncListenerAdapter()
                 {
                     @Override
@@ -268,7 +271,8 @@ public class AsyncListenerTest
                 "Connection: close\r\n" +
                 "\r\n");
         assertThat(httpResponse, containsString("HTTP/1.1 500 "));
-        assertThat(httpResponse, containsString(TestRuntimeException.class.getName()));
+        assertThat(httpResponse, containsString("AsyncContext timeout"));
+        assertThat(httpResponse, not(containsString(TestRuntimeException.class.getName())));
     }
 
     @Test
@@ -292,6 +296,7 @@ public class AsyncListenerTest
         {
             HttpServletResponse response = (HttpServletResponse)event.getAsyncContext().getResponse();
             response.sendError(HttpStatus.BAD_GATEWAY_502);
+            event.getAsyncContext().complete();
         });
         String httpResponse = connector.getResponse(
             "GET / HTTP/1.1\r\n" +
@@ -384,7 +389,7 @@ public class AsyncListenerTest
             protected void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
             {
                 AsyncContext asyncContext = request.startAsync();
-                asyncContext.setTimeout(0);
+                asyncContext.setTimeout(10000);
                 asyncContext.addListener(new AsyncListenerAdapter()
                 {
                     @Override
@@ -447,7 +452,7 @@ public class AsyncListenerTest
     }
 
     // Unique named RuntimeException to help during debugging / assertions.
-    public static class TestRuntimeException extends RuntimeException
+    public static class TestRuntimeException extends RuntimeException implements QuietException
     {
     }
 
diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncServletIOTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncServletIOTest.java
index 3ea4c804437..b80394f61f6 100644
--- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncServletIOTest.java
+++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncServletIOTest.java
@@ -52,6 +52,7 @@ import org.eclipse.jetty.server.HttpConfiguration;
 import org.eclipse.jetty.server.HttpConnectionFactory;
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.server.ServerConnector;
+import org.eclipse.jetty.util.BufferUtil;
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.Logger;
 import org.eclipse.jetty.util.thread.QueuedThreadPool;
@@ -305,6 +306,7 @@ public class AsyncServletIOTest
             request.append(s).append("w=").append(w);
             s = '&';
         }
+        LOG.debug("process {} {}", request.toString(), BufferUtil.toDetailString(BufferUtil.toBuffer(content)));
 
         request.append(" HTTP/1.1\r\n")
             .append("Host: localhost\r\n")
@@ -816,13 +818,15 @@ public class AsyncServletIOTest
             // wait until server is ready
             _servletStolenAsyncRead.ready.await();
             final CountDownLatch wait = new CountDownLatch(1);
-
+            final CountDownLatch held = new CountDownLatch(1);
             // Stop any dispatches until we want them
+
             UnaryOperator old = _wQTP.wrapper.getAndSet(r ->
                 () ->
                 {
                     try
                     {
+                        held.countDown();
                         wait.await();
                         r.run();
                     }
@@ -836,7 +840,9 @@ public class AsyncServletIOTest
             // We are an unrelated thread, let's mess with the input stream
             ServletInputStream sin = _servletStolenAsyncRead.listener.in;
             sin.setReadListener(_servletStolenAsyncRead.listener);
+
             // thread should be dispatched to handle, but held by our wQTP wait.
+            assertTrue(held.await(10, TimeUnit.SECONDS));
 
             // Let's steal our read
             assertTrue(sin.isReady());
diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncServletTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncServletTest.java
index d197aa3a57b..571d8d76a7a 100644
--- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncServletTest.java
+++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncServletTest.java
@@ -265,7 +265,6 @@ public class AsyncServletTest
             "start",
             "onTimeout",
             "error",
-            "onError",
             "ERROR /ctx/error/custom",
             "!initial",
             "onComplete"));
@@ -273,44 +272,6 @@ public class AsyncServletTest
         assertContains("ERROR DISPATCH", response);
     }
 
-    @Test
-    public void testStartOnTimeoutErrorComplete() throws Exception
-    {
-        String response = process("start=200&timeout=error&error=complete", null);
-        assertThat(response, startsWith("HTTP/1.1 200 OK"));
-        assertThat(__history, contains(
-            "REQUEST /ctx/path/info",
-            "initial",
-            "start",
-            "onTimeout",
-            "error",
-            "onError",
-            "complete",
-            "onComplete"));
-
-        assertContains("COMPLETED", response);
-    }
-
-    @Test
-    public void testStartOnTimeoutErrorDispatch() throws Exception
-    {
-        String response = process("start=200&timeout=error&error=dispatch", null);
-        assertThat(response, startsWith("HTTP/1.1 200 OK"));
-        assertThat(__history, contains(
-            "REQUEST /ctx/path/info",
-            "initial",
-            "start",
-            "onTimeout",
-            "error",
-            "onError",
-            "dispatch",
-            "ASYNC /ctx/path/info",
-            "!initial",
-            "onComplete"));
-
-        assertContains("DISPATCHED", response);
-    }
-
     @Test
     public void testStartOnTimeoutComplete() throws Exception
     {
@@ -526,8 +487,10 @@ public class AsyncServletTest
             "onStartAsync",
             "start",
             "onTimeout",
+            "ERROR /ctx/path/error",
+            "!initial",
             "onComplete")); // Error Page Loop!
-        assertContains("HTTP ERROR 500", response);
+        assertContains("AsyncContext timeout", response);
     }
 
     @Test
diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/CustomRequestLogTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/CustomRequestLogTest.java
index a422cf0480f..3c7ba9a7a55 100644
--- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/CustomRequestLogTest.java
+++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/CustomRequestLogTest.java
@@ -85,7 +85,8 @@ public class CustomRequestLogTest
 
         _connector.getResponse("GET /context/servlet/info HTTP/1.0\n\n");
         String log = _entries.poll(5, TimeUnit.SECONDS);
-        assertThat(log, is("Filename: " + _tmpDir + File.separator + "servlet" + File.separator + "info"));
+        String expected = new File(_tmpDir + File.separator + "servlet" + File.separator + "info").getCanonicalPath();
+        assertThat(log, is("Filename: " + expected));
     }
 
     @Test
diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ErrorPageTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ErrorPageTest.java
index 899cc3d0169..0d8db7f6e55 100644
--- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ErrorPageTest.java
+++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ErrorPageTest.java
@@ -20,8 +20,22 @@ package org.eclipse.jetty.servlet;
 
 import java.io.IOException;
 import java.io.PrintWriter;
+import java.nio.charset.StandardCharsets;
+import java.util.EnumSet;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.servlet.AsyncContext;
+import javax.servlet.DispatcherType;
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
 import javax.servlet.Servlet;
 import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
 import javax.servlet.http.HttpServlet;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
@@ -29,8 +43,12 @@ import javax.servlet.http.HttpServletResponse;
 import org.eclipse.jetty.http.BadMessageException;
 import org.eclipse.jetty.server.Dispatcher;
 import org.eclipse.jetty.server.HttpChannel;
+import org.eclipse.jetty.server.HttpChannelState;
 import org.eclipse.jetty.server.LocalConnector;
+import org.eclipse.jetty.server.Request;
 import org.eclipse.jetty.server.Server;
+import org.eclipse.jetty.server.handler.HandlerWrapper;
+import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.StacklessLogging;
 import org.hamcrest.Matchers;
 import org.junit.jupiter.api.AfterEach;
@@ -40,43 +58,71 @@ import org.junit.jupiter.api.Test;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.not;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 public class ErrorPageTest
 {
     private Server _server;
     private LocalConnector _connector;
     private StacklessLogging _stackless;
+    private static CountDownLatch __asyncSendErrorCompleted;
+    private ErrorPageErrorHandler _errorPageErrorHandler;
 
     @BeforeEach
     public void init() throws Exception
     {
         _server = new Server();
         _connector = new LocalConnector(_server);
+        _server.addConnector(_connector);
+
         ServletContextHandler context = new ServletContextHandler(ServletContextHandler.NO_SECURITY | ServletContextHandler.NO_SESSIONS);
 
-        _server.addConnector(_connector);
         _server.setHandler(context);
 
         context.setContextPath("/");
 
+        context.addFilter(SingleDispatchFilter.class, "/*", EnumSet.allOf(DispatcherType.class));
+
         context.addServlet(DefaultServlet.class, "/");
         context.addServlet(FailServlet.class, "/fail/*");
         context.addServlet(FailClosedServlet.class, "/fail-closed/*");
         context.addServlet(ErrorServlet.class, "/error/*");
         context.addServlet(AppServlet.class, "/app/*");
         context.addServlet(LongerAppServlet.class, "/longer.app/*");
+        context.addServlet(SyncSendErrorServlet.class, "/sync/*");
+        context.addServlet(AsyncSendErrorServlet.class, "/async/*");
+        context.addServlet(NotEnoughServlet.class, "/notenough/*");
+        context.addServlet(DeleteServlet.class, "/delete/*");
+        context.addServlet(ErrorAndStatusServlet.class, "/error-and-status/*");
 
-        ErrorPageErrorHandler error = new ErrorPageErrorHandler();
-        context.setErrorHandler(error);
-        error.addErrorPage(599, "/error/599");
-        error.addErrorPage(400, "/error/400");
+        HandlerWrapper noopHandler = new HandlerWrapper()
+        {
+            @Override
+            public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
+            {
+                if (target.startsWith("/noop"))
+                    return;
+                else
+                    super.handle(target, baseRequest, request, response);
+            }
+        };
+        context.insertHandler(noopHandler);
+
+        _errorPageErrorHandler = new ErrorPageErrorHandler();
+        context.setErrorHandler(_errorPageErrorHandler);
+        _errorPageErrorHandler.addErrorPage(595, "/error/595");
+        _errorPageErrorHandler.addErrorPage(597, "/sync");
+        _errorPageErrorHandler.addErrorPage(599, "/error/599");
+        _errorPageErrorHandler.addErrorPage(400, "/error/400");
         // error.addErrorPage(500,"/error/500");
-        error.addErrorPage(IllegalStateException.class.getCanonicalName(), "/error/TestException");
-        error.addErrorPage(BadMessageException.class, "/error/BadMessageException");
-        error.addErrorPage(ErrorPageErrorHandler.GLOBAL_ERROR_PAGE, "/error/GlobalErrorPage");
+        _errorPageErrorHandler.addErrorPage(IllegalStateException.class.getCanonicalName(), "/error/TestException");
+        _errorPageErrorHandler.addErrorPage(BadMessageException.class, "/error/BadMessageException");
+        _errorPageErrorHandler.addErrorPage(ErrorPageErrorHandler.GLOBAL_ERROR_PAGE, "/error/GlobalErrorPage");
 
         _server.start();
         _stackless = new StacklessLogging(ServletHandler.class);
+
+        __asyncSendErrorCompleted = new CountDownLatch(1);
     }
 
     @AfterEach
@@ -87,6 +133,101 @@ public class ErrorPageTest
         _server.join();
     }
 
+    @Test
+    void testErrorOverridesStatus() throws Exception
+    {
+        String response = _connector.getResponse("GET /error-and-status/anything HTTP/1.0\r\n\r\n");
+        assertThat(response, Matchers.containsString("HTTP/1.1 594 594"));
+        assertThat(response, Matchers.containsString("ERROR_PAGE: /GlobalErrorPage"));
+        assertThat(response, Matchers.containsString("ERROR_MESSAGE: custom get error"));
+        assertThat(response, Matchers.containsString("ERROR_CODE: 594"));
+        assertThat(response, Matchers.containsString("ERROR_EXCEPTION: null"));
+        assertThat(response, Matchers.containsString("ERROR_EXCEPTION_TYPE: null"));
+        assertThat(response, Matchers.containsString("ERROR_SERVLET: org.eclipse.jetty.servlet.ErrorPageTest$ErrorAndStatusServlet-"));
+        assertThat(response, Matchers.containsString("ERROR_REQUEST_URI: /error-and-status/anything"));
+    }
+
+    @Test
+    void testHttp204CannotHaveBody() throws Exception
+    {
+        String response = _connector.getResponse("GET /fail/code?code=204 HTTP/1.0\r\n\r\n");
+        assertThat(response, Matchers.containsString("HTTP/1.1 204 No Content"));
+        assertThat(response, not(Matchers.containsString("DISPATCH: ")));
+        assertThat(response, not(Matchers.containsString("ERROR_PAGE: ")));
+        assertThat(response, not(Matchers.containsString("ERROR_CODE: ")));
+        assertThat(response, not(Matchers.containsString("ERROR_EXCEPTION: ")));
+        assertThat(response, not(Matchers.containsString("ERROR_EXCEPTION_TYPE: ")));
+        assertThat(response, not(Matchers.containsString("ERROR_SERVLET: ")));
+        assertThat(response, not(Matchers.containsString("ERROR_REQUEST_URI: ")));
+    }
+
+    @Test
+    void testDeleteCannotHaveBody() throws Exception
+    {
+        String response = _connector.getResponse("DELETE /delete/anything HTTP/1.0\r\n\r\n");
+        assertThat(response, Matchers.containsString("HTTP/1.1 595 595"));
+        assertThat(response, not(Matchers.containsString("DISPATCH: ")));
+        assertThat(response, not(Matchers.containsString("ERROR_PAGE: ")));
+        assertThat(response, not(Matchers.containsString("ERROR_MESSAGE: ")));
+        assertThat(response, not(Matchers.containsString("ERROR_CODE: ")));
+        assertThat(response, not(Matchers.containsString("ERROR_EXCEPTION: ")));
+        assertThat(response, not(Matchers.containsString("ERROR_EXCEPTION_TYPE: ")));
+        assertThat(response, not(Matchers.containsString("ERROR_SERVLET: ")));
+        assertThat(response, not(Matchers.containsString("ERROR_REQUEST_URI: ")));
+
+        assertThat(response, not(containsString("This shouldn't be seen")));
+    }
+
+    @Test
+    void testGenerateAcceptableResponse_noAcceptHeader() throws Exception
+    {
+        // no global error page here
+        _errorPageErrorHandler.getErrorPages().remove(ErrorPageErrorHandler.GLOBAL_ERROR_PAGE);
+
+        String response = _connector.getResponse("GET /fail/code?code=598 HTTP/1.0\r\n\r\n");
+        assertThat(response, Matchers.containsString("HTTP/1.1 598 598"));
+        assertThat(response, Matchers.containsString("Error 598"));
+        assertThat(response, Matchers.containsString("<h2>HTTP ERROR 598"));
+        assertThat(response, Matchers.containsString("/fail/code"));
+    }
+
+    @Test
+    void testGenerateAcceptableResponse_htmlAcceptHeader() throws Exception
+    {
+        // no global error page here
+        _errorPageErrorHandler.getErrorPages().remove(ErrorPageErrorHandler.GLOBAL_ERROR_PAGE);
+
+        // even when text/html is not the 1st content type, a html error page should still be generated
+        String response = _connector.getResponse("GET /fail/code?code=598 HTTP/1.0\r\n" +
+            "Accept: application/bytes,text/html\r\n\r\n");
+        assertThat(response, Matchers.containsString("HTTP/1.1 598 598"));
+        assertThat(response, Matchers.containsString("<title>Error 598"));
+        assertThat(response, Matchers.containsString("<h2>HTTP ERROR 598"));
+        assertThat(response, Matchers.containsString("/fail/code"));
+    }
+
+    @Test
+    void testGenerateAcceptableResponse_noHtmlAcceptHeader() throws Exception
+    {
+        // no global error page here
+        _errorPageErrorHandler.getErrorPages().remove(ErrorPageErrorHandler.GLOBAL_ERROR_PAGE);
+
+        String response = _connector.getResponse("GET /fail/code?code=598 HTTP/1.0\r\n" +
+            "Accept: application/bytes\r\n\r\n");
+        assertThat(response, Matchers.containsString("HTTP/1.1 598 598"));
+        assertThat(response, not(Matchers.containsString("<title>Error 598")));
+        assertThat(response, not(Matchers.containsString("<h2>HTTP ERROR 598")));
+        assertThat(response, not(Matchers.containsString("/fail/code")));
+    }
+
+    @Test
+    void testNestedSendErrorDoesNotLoop() throws Exception
+    {
+        String response = _connector.getResponse("GET /fail/code?code=597 HTTP/1.0\r\n\r\n");
+        assertThat(response, Matchers.containsString("HTTP/1.1 597 597"));
+        assertThat(response, not(Matchers.containsString("time this error page is being accessed")));
+    }
+
     @Test
     public void testSendErrorClosedResponse() throws Exception
     {
@@ -167,7 +308,7 @@ public class ErrorPageTest
         try (StacklessLogging ignore = new StacklessLogging(Dispatcher.class))
         {
             String response = _connector.getResponse("GET /app?baa=%88%A4 HTTP/1.0\r\n\r\n");
-            assertThat(response, Matchers.containsString("HTTP/1.1 400 Bad query encoding"));
+            assertThat(response, Matchers.containsString("HTTP/1.1 400 Bad Request"));
             assertThat(response, Matchers.containsString("ERROR_PAGE: /BadMessageException"));
             assertThat(response, Matchers.containsString("ERROR_MESSAGE: Bad query encoding"));
             assertThat(response, Matchers.containsString("ERROR_CODE: 400"));
@@ -179,6 +320,94 @@ public class ErrorPageTest
         }
     }
 
+    @Test
+    public void testAsyncErrorPageDSC() throws Exception
+    {
+        try (StacklessLogging ignore = new StacklessLogging(Dispatcher.class))
+        {
+            String response = _connector.getResponse("GET /async/info?mode=DSC HTTP/1.0\r\n\r\n");
+            assertThat(response, Matchers.containsString("HTTP/1.1 599 599"));
+            assertThat(response, Matchers.containsString("ERROR_PAGE: /599"));
+            assertThat(response, Matchers.containsString("ERROR_CODE: 599"));
+            assertThat(response, Matchers.containsString("ERROR_EXCEPTION: null"));
+            assertThat(response, Matchers.containsString("ERROR_EXCEPTION_TYPE: null"));
+            assertThat(response, Matchers.containsString("ERROR_SERVLET: org.eclipse.jetty.servlet.ErrorPageTest$AsyncSendErrorServlet-"));
+            assertThat(response, Matchers.containsString("ERROR_REQUEST_URI: /async/info"));
+            assertTrue(__asyncSendErrorCompleted.await(10, TimeUnit.SECONDS));
+        }
+    }
+
+    @Test
+    public void testAsyncErrorPageSDC() throws Exception
+    {
+        try (StacklessLogging ignore = new StacklessLogging(Dispatcher.class))
+        {
+            String response = _connector.getResponse("GET /async/info?mode=SDC HTTP/1.0\r\n\r\n");
+            assertThat(response, Matchers.containsString("HTTP/1.1 599 599"));
+            assertThat(response, Matchers.containsString("ERROR_PAGE: /599"));
+            assertThat(response, Matchers.containsString("ERROR_CODE: 599"));
+            assertThat(response, Matchers.containsString("ERROR_EXCEPTION: null"));
+            assertThat(response, Matchers.containsString("ERROR_EXCEPTION_TYPE: null"));
+            assertThat(response, Matchers.containsString("ERROR_SERVLET: org.eclipse.jetty.servlet.ErrorPageTest$AsyncSendErrorServlet-"));
+            assertThat(response, Matchers.containsString("ERROR_REQUEST_URI: /async/info"));
+            assertTrue(__asyncSendErrorCompleted.await(10, TimeUnit.SECONDS));
+        }
+    }
+
+    @Test
+    public void testAsyncErrorPageSCD() throws Exception
+    {
+        try (StacklessLogging ignore = new StacklessLogging(Dispatcher.class))
+        {
+            String response = _connector.getResponse("GET /async/info?mode=SCD HTTP/1.0\r\n\r\n");
+            assertThat(response, Matchers.containsString("HTTP/1.1 599 599"));
+            assertThat(response, Matchers.containsString("ERROR_PAGE: /599"));
+            assertThat(response, Matchers.containsString("ERROR_CODE: 599"));
+            assertThat(response, Matchers.containsString("ERROR_EXCEPTION: null"));
+            assertThat(response, Matchers.containsString("ERROR_EXCEPTION_TYPE: null"));
+            assertThat(response, Matchers.containsString("ERROR_SERVLET: org.eclipse.jetty.servlet.ErrorPageTest$AsyncSendErrorServlet-"));
+            assertThat(response, Matchers.containsString("ERROR_REQUEST_URI: /async/info"));
+            assertTrue(__asyncSendErrorCompleted.await(10, TimeUnit.SECONDS));
+        }
+    }
+
+    @Test
+    public void testNoop() throws Exception
+    {
+        String response = _connector.getResponse("GET /noop/info HTTP/1.0\r\n\r\n");
+        assertThat(response, Matchers.containsString("HTTP/1.1 404 Not Found"));
+        assertThat(response, Matchers.containsString("DISPATCH: ERROR"));
+        assertThat(response, Matchers.containsString("ERROR_PAGE: /GlobalErrorPage"));
+        assertThat(response, Matchers.containsString("ERROR_CODE: 404"));
+        assertThat(response, Matchers.containsString("ERROR_EXCEPTION: null"));
+        assertThat(response, Matchers.containsString("ERROR_EXCEPTION_TYPE: null"));
+        assertThat(response, Matchers.containsString("ERROR_SERVLET: org.eclipse.jetty.servlet.DefaultServlet-"));
+        assertThat(response, Matchers.containsString("ERROR_REQUEST_URI: /noop/info"));
+    }
+
+    @Test
+    public void testNotEnough() throws Exception
+    {
+        String response = _connector.getResponse("GET /notenough/info HTTP/1.0\r\n\r\n");
+        assertThat(response, Matchers.containsString("HTTP/1.1 500 Server Error"));
+        assertThat(response, Matchers.containsString("DISPATCH: ERROR"));
+        assertThat(response, Matchers.containsString("ERROR_PAGE: /GlobalErrorPage"));
+        assertThat(response, Matchers.containsString("ERROR_CODE: 500"));
+        assertThat(response, Matchers.containsString("ERROR_EXCEPTION: null"));
+        assertThat(response, Matchers.containsString("ERROR_EXCEPTION_TYPE: null"));
+        assertThat(response, Matchers.containsString("ERROR_SERVLET: org.eclipse.jetty.servlet.ErrorPageTest$NotEnoughServlet-"));
+        assertThat(response, Matchers.containsString("ERROR_REQUEST_URI: /notenough/info"));
+    }
+
+    @Test
+    public void testNotEnoughCommitted() throws Exception
+    {
+        String response = _connector.getResponse("GET /notenough/info?commit=true HTTP/1.0\r\n\r\n");
+        assertThat(response, Matchers.containsString("HTTP/1.1 200 OK"));
+        assertThat(response, Matchers.containsString("Content-Length: 1000"));
+        assertThat(response, Matchers.endsWith("SomeBytes"));
+    }
+
     public static class AppServlet extends HttpServlet implements Servlet
     {
         @Override
@@ -198,6 +427,112 @@ public class ErrorPageTest
         }
     }
 
+    public static class SyncSendErrorServlet extends HttpServlet implements Servlet
+    {
+        public static final AtomicInteger COUNTER = new AtomicInteger();
+
+        @Override
+        protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
+        {
+            int count = COUNTER.incrementAndGet();
+
+            PrintWriter writer = response.getWriter();
+            writer.println("this is the " + count + " time this error page is being accessed");
+            response.sendError(597, "loop #" + count);
+        }
+    }
+
+    public static class AsyncSendErrorServlet extends HttpServlet implements Servlet
+    {
+        @Override
+        protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
+        {
+            try
+            {
+                final CountDownLatch hold = new CountDownLatch(1);
+                final String mode = request.getParameter("mode");
+                switch(mode)
+                {
+                    case "DSC":
+                    case "SDC":
+                    case "SCD":
+                        break;
+                    default:
+                        throw new IllegalStateException(mode);
+                }
+
+                final boolean lateComplete = "true".equals(request.getParameter("latecomplete"));
+                AsyncContext async = request.startAsync();
+                async.start(() ->
+                {
+                    try
+                    {
+                        switch(mode)
+                        {
+                            case "SDC":
+                                response.sendError(599);
+                                break;
+                            case "SCD":
+                                response.sendError(599);
+                                async.complete();
+                                break;
+                            default:
+                                break;
+                        }
+
+                        // Complete after original servlet
+                        hold.countDown();
+
+                        // Wait until request async waiting
+                        while (Request.getBaseRequest(request).getHttpChannelState().getState() == HttpChannelState.State.HANDLING)
+                        {
+                            try
+                            {
+                                Thread.sleep(10);
+                            }
+                            catch (InterruptedException e)
+                            {
+                                e.printStackTrace();
+                            }
+                        }
+                        try
+                        {
+                            switch (mode)
+                            {
+                                case "DSC":
+                                    response.sendError(599);
+                                    async.complete();
+                                    break;
+                                case "SDC":
+                                    async.complete();
+                                    break;
+                                default:
+                                    break;
+                            }
+                        }
+                        catch(IllegalStateException e)
+                        {
+                            Log.getLog().ignore(e);
+                        }
+                        finally
+                        {
+                            __asyncSendErrorCompleted.countDown();
+                        }
+                    }
+                    catch (IOException e)
+                    {
+                        Log.getLog().warn(e);
+                    }
+                });
+                hold.await();
+            }
+            catch (InterruptedException e)
+            {
+                throw new ServletException(e);
+            }
+        }
+    }
+
     public static class FailServlet extends HttpServlet implements Servlet
     {
         @Override
@@ -225,16 +560,51 @@ public class ErrorPageTest
             }
             catch (Throwable ignore)
             {
-                // no opEchoSocket
+                Log.getLog().ignore(ignore);
             }
         }
     }
 
+    public static class ErrorAndStatusServlet extends HttpServlet implements Servlet
+    {
+        @Override
+        protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
+        {
+            response.sendError(594, "custom get error");
+            response.setStatus(200);
+        }
+    }
+
+    public static class DeleteServlet extends HttpServlet implements Servlet
+    {
+        @Override
+        protected void doDelete(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
+        {
+            response.getWriter().append("This shouldn't be seen");
+            response.sendError(595, "custom delete");
+        }
+    }
+
+    public static class NotEnoughServlet extends HttpServlet implements Servlet
+    {
+        @Override
+        protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
+        {
+            response.setContentLength(1000);
+            response.getOutputStream().write("SomeBytes".getBytes(StandardCharsets.UTF_8));
+            if (Boolean.parseBoolean(request.getParameter("commit")))
+                response.flushBuffer();
+        }
+    }
+
     public static class ErrorServlet extends HttpServlet implements Servlet
     {
         @Override
-        protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
+        protected void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
         {
+            if (request.getDispatcherType() != DispatcherType.ERROR && request.getDispatcherType() != DispatcherType.ASYNC)
+                throw new IllegalStateException("Bad Dispatcher Type " + request.getDispatcherType());
+
             PrintWriter writer = response.getWriter();
             writer.println("DISPATCH: " + request.getDispatcherType().name());
             writer.println("ERROR_PAGE: " + request.getPathInfo());
@@ -247,4 +617,55 @@ public class ErrorPageTest
             writer.println("getParameterMap()= " + request.getParameterMap());
         }
     }
+
+    public static class SingleDispatchFilter implements Filter
+    {
+        ConcurrentMap<Integer, Thread> dispatches = new ConcurrentHashMap<>();
+
+        @Override
+        public void init(FilterConfig filterConfig) throws ServletException
+        {
+
+        }
+
+        @Override
+        public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException
+        {
+            final Integer key = request.hashCode();
+            Thread current = Thread.currentThread();
+            final Thread existing = dispatches.putIfAbsent(key, current);
+            if (existing != null && existing != current)
+            {
+                System.err.println("DOUBLE DISPATCH OF REQUEST!!!!!!!!!!!!!!!!!!");
+                System.err.println("Thread " + existing + " :");
+                for (StackTraceElement element : existing.getStackTrace())
+                {
+                    System.err.println("\tat " + element);
+                }
+                IllegalStateException ex = new IllegalStateException();
+                ex.printStackTrace();
+                response.flushBuffer();
+                throw ex;
+            }
+
+            try
+            {
+                chain.doFilter(request, response);
+            }
+            finally
+            {
+                if (existing == null)
+                {
+                    if (!dispatches.remove(key, current))
+                        throw new IllegalStateException();
+                }
+            }
+        }
+
+        @Override
+        public void destroy()
+        {
+
+        }
+    }
 }
diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerTest.java
index cf8e7d59261..1362ac6f4d4 100644
--- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerTest.java
+++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerTest.java
@@ -153,7 +153,10 @@ public class GzipHandlerTest
             response.setHeader("ETag", __contentETag);
             String ifnm = req.getHeader("If-None-Match");
             if (ifnm != null && ifnm.equals(__contentETag))
-                response.sendError(304);
+            {
+                response.setStatus(304);
+                response.flushBuffer();
+            }
             else
             {
                 PrintWriter writer = response.getWriter();
diff --git a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/DoSFilter.java b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/DoSFilter.java
index fcdff1bdec8..d15f57dc3cc 100644
--- a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/DoSFilter.java
+++ b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/DoSFilter.java
@@ -501,7 +501,16 @@ public class DoSFilter implements Filter
         {
             if (LOG.isDebugEnabled())
                 LOG.debug("Timing out {}", request);
-            response.sendError(HttpStatus.SERVICE_UNAVAILABLE_503);
+            try
+            {
+                response.sendError(HttpStatus.SERVICE_UNAVAILABLE_503);
+            }
+            catch (IllegalStateException ise)
+            {
+                LOG.ignore(ise);
+                // abort instead
+                response.sendError(-1);
+            }
         }
         catch (Throwable x)
         {
diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java b/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java
index a0eceef6307..452c1be848c 100644
--- a/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java
+++ b/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java
@@ -440,6 +440,7 @@ public class BufferUtil
      *
      * @param to Buffer is flush mode
      * @param b byte to append
+     * @throws BufferOverflowException if unable to append buffer due to space limits
      */
     public static void append(ByteBuffer to, byte b)
     {
@@ -1103,20 +1104,20 @@ public class BufferUtil
             for (int i = 0; i < buffer.position(); i++)
             {
                 appendContentChar(buf, buffer.get(i));
-                if (i == 16 && buffer.position() > 32)
+                if (i == 8 && buffer.position() > 16)
                 {
                     buf.append("...");
-                    i = buffer.position() - 16;
+                    i = buffer.position() - 8;
                 }
             }
             buf.append("<<<");
             for (int i = buffer.position(); i < buffer.limit(); i++)
             {
                 appendContentChar(buf, buffer.get(i));
-                if (i == buffer.position() + 16 && buffer.limit() > buffer.position() + 32)
+                if (i == buffer.position() + 24 && buffer.limit() > buffer.position() + 48)
                 {
                     buf.append("...");
-                    i = buffer.limit() - 16;
+                    i = buffer.limit() - 24;
                 }
             }
             buf.append(">>>");
@@ -1125,10 +1126,10 @@ public class BufferUtil
             for (int i = limit; i < buffer.capacity(); i++)
             {
                 appendContentChar(buf, buffer.get(i));
-                if (i == limit + 16 && buffer.capacity() > limit + 32)
+                if (i == limit + 8 && buffer.capacity() > limit + 16)
                 {
                     buf.append("...");
-                    i = buffer.capacity() - 16;
+                    i = buffer.capacity() - 8;
                 }
             }
             buffer.limit(limit);
diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java
index c2d77760c08..41f02b80e8b 100644
--- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java
+++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java
@@ -171,10 +171,11 @@ public class QueuedThreadPool extends ContainerLifeCycle implements SizedThreadP
         if (LOG.isDebugEnabled())
             LOG.debug("Stopping {}", this);
 
+        super.doStop();
+
         removeBean(_tryExecutor);
         _tryExecutor = TryExecutor.NO_TRY;
 
-        super.doStop();
 
         // Signal the Runner threads that we are stopping
         int threads = _counts.getAndSetHi(Integer.MIN_VALUE);
diff --git a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/WebSocketConnectionStatsTest.java b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/WebSocketConnectionStatsTest.java
index 41e79c01b86..148720950c4 100644
--- a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/WebSocketConnectionStatsTest.java
+++ b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/WebSocketConnectionStatsTest.java
@@ -176,4 +176,4 @@ public class WebSocketConnectionStatsTest
         assertThat("stats.sendBytes", statistics.getSentBytes(), is(expectedSent));
         assertThat("stats.receivedBytes", statistics.getReceivedBytes(), is(expectedReceived));
     }
-}
\ No newline at end of file
+}
diff --git a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/WebSocketNegotiationTest.java b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/WebSocketNegotiationTest.java
index 9c23e097ccb..27ec3c2c45a 100644
--- a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/WebSocketNegotiationTest.java
+++ b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/WebSocketNegotiationTest.java
@@ -115,7 +115,7 @@ public class WebSocketNegotiationTest
         client.getOutputStream().write(upgradeRequest.getBytes(ISO_8859_1));
         String response = getUpgradeResponse(client.getInputStream());
 
-        assertThat(response, containsString("400 Missing request header 'Sec-WebSocket-Key'"));
+        assertThat(response, containsString("400 "));
     }
 
     protected static HttpFields newUpgradeRequest(String extensions)
diff --git a/jetty-websocket/websocket-server/src/test/java/org/eclipse/jetty/websocket/server/WebSocketInvalidVersionTest.java b/jetty-websocket/websocket-server/src/test/java/org/eclipse/jetty/websocket/server/WebSocketInvalidVersionTest.java
index 105de580dc2..36989de4607 100644
--- a/jetty-websocket/websocket-server/src/test/java/org/eclipse/jetty/websocket/server/WebSocketInvalidVersionTest.java
+++ b/jetty-websocket/websocket-server/src/test/java/org/eclipse/jetty/websocket/server/WebSocketInvalidVersionTest.java
@@ -89,6 +89,6 @@ public class WebSocketInvalidVersionTest
             connFut.get(Timeouts.CONNECT, Timeouts.CONNECT_UNIT);
         });
         assertThat(x.getCause(), instanceOf(UpgradeException.class));
-        assertThat(x.getMessage(), containsString("400 Unsupported websocket version specification"));
+        assertThat(x.getMessage(), containsString("400 "));
     }
 }
diff --git a/tests/test-distribution/src/test/java/org/eclipse/jetty/tests/distribution/BadAppTests.java b/tests/test-distribution/src/test/java/org/eclipse/jetty/tests/distribution/BadAppTests.java
index 190bc381743..9248a2dc5e4 100644
--- a/tests/test-distribution/src/test/java/org/eclipse/jetty/tests/distribution/BadAppTests.java
+++ b/tests/test-distribution/src/test/java/org/eclipse/jetty/tests/distribution/BadAppTests.java
@@ -108,8 +108,8 @@ public class BadAppTests extends AbstractDistributionTest
                 startHttpClient();
                 ContentResponse response = client.GET("http://localhost:" + port + "/badapp/");
                 assertEquals(HttpStatus.SERVICE_UNAVAILABLE_503, response.getStatus());
-                assertThat(response.getContentAsString(), containsString("Unavailable"));
-                assertThat(response.getContentAsString(), containsString("Problem accessing /badapp/"));
+                assertThat(response.getContentAsString(), containsString("<h2>HTTP ERROR 503 Service Unavailable</h2>"));
+                assertThat(response.getContentAsString(), containsString("<tr><th>URI:</th><td>/badapp/</td></tr>"));
             }
         }
     }
@@ -148,8 +148,8 @@ public class BadAppTests extends AbstractDistributionTest
                 startHttpClient();
                 ContentResponse response = client.GET("http://localhost:" + port + "/badapp/");
                 assertEquals(HttpStatus.SERVICE_UNAVAILABLE_503, response.getStatus());
-                assertThat(response.getContentAsString(), containsString("Unavailable"));
-                assertThat(response.getContentAsString(), containsString("Problem accessing /badapp/"));
+                assertThat(response.getContentAsString(), containsString("<h2>HTTP ERROR 503 Service Unavailable</h2>"));
+                assertThat(response.getContentAsString(), containsString("<tr><th>URI:</th><td>/badapp/</td></tr>"));
             }
         }
     }