From dd0d270ba26e163d41fd2b06e1ea13afe1ca6aac Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Wed, 29 Nov 2017 12:39:06 -0500 Subject: [PATCH] Improve error handling when response is committed ResponseStatusExceptionHandler lets the error through if it can't change the status while HttpWebHandlerAdapter logs a more helpful message (including status code) but without a full stack trace. Issue: SPR-16231 --- .../server/adapter/HttpWebHandlerAdapter.java | 16 ++++++++++------ .../handler/ResponseStatusExceptionHandler.java | 10 ++++++---- .../ResponseStatusExceptionHandlerTests.java | 7 +++---- 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/server/adapter/HttpWebHandlerAdapter.java b/spring-web/src/main/java/org/springframework/web/server/adapter/HttpWebHandlerAdapter.java index 8335302a7cc..6cc5215a21d 100644 --- a/spring-web/src/main/java/org/springframework/web/server/adapter/HttpWebHandlerAdapter.java +++ b/spring-web/src/main/java/org/springframework/web/server/adapter/HttpWebHandlerAdapter.java @@ -71,7 +71,7 @@ public class HttpWebHandlerAdapter extends WebHandlerDecorator implements HttpHa *

TODO: * This definition is currently duplicated between HttpWebHandlerAdapter * and AbstractSockJsSession. It is a candidate for a common utility class. - * @see #indicatesDisconnectedClient(Throwable) + * @see #isDisconnectedClientError(Throwable) */ private static final Set DISCONNECTED_CLIENT_EXCEPTIONS = new HashSet<>(Arrays.asList("ClientAbortException", "EOFException", "EofException")); @@ -158,8 +158,7 @@ public class HttpWebHandlerAdapter extends WebHandlerDecorator implements HttpHa ServerWebExchange exchange = createExchange(request, response); return getDelegate().handle(exchange) .onErrorResume(ex -> { - response.setStatusCode(HttpStatus.INTERNAL_SERVER_ERROR); - logHandleFailure(ex); + handleFailure(response, ex); return Mono.empty(); }) .then(Mono.defer(response::setComplete)); @@ -170,8 +169,9 @@ public class HttpWebHandlerAdapter extends WebHandlerDecorator implements HttpHa getCodecConfigurer(), getLocaleContextResolver()); } - private void logHandleFailure(Throwable ex) { - if (indicatesDisconnectedClient(ex)) { + private void handleFailure(ServerHttpResponse response, Throwable ex) { + boolean statusCodeChanged = response.setStatusCode(HttpStatus.INTERNAL_SERVER_ERROR); + if (isDisconnectedClientError(ex)) { if (disconnectedClientLogger.isTraceEnabled()) { disconnectedClientLogger.trace("Looks like the client has gone away", ex); } @@ -181,12 +181,16 @@ public class HttpWebHandlerAdapter extends WebHandlerDecorator implements HttpHa "' to TRACE level.)"); } } + else if (!statusCodeChanged) { + logger.error("Unhandled failure: " + ex.getMessage() + ", " + + "response already committed with status=" + response.getStatusCode()); + } else { logger.error("Failed to handle request", ex); } } - private boolean indicatesDisconnectedClient(Throwable ex) { + private boolean isDisconnectedClientError(Throwable ex) { String message = NestedExceptionUtils.getMostSpecificCause(ex).getMessage(); message = (message != null ? message.toLowerCase() : ""); String className = ex.getClass().getSimpleName(); diff --git a/spring-web/src/main/java/org/springframework/web/server/handler/ResponseStatusExceptionHandler.java b/spring-web/src/main/java/org/springframework/web/server/handler/ResponseStatusExceptionHandler.java index 81fe316e3c9..9d937d60c2b 100644 --- a/spring-web/src/main/java/org/springframework/web/server/handler/ResponseStatusExceptionHandler.java +++ b/spring-web/src/main/java/org/springframework/web/server/handler/ResponseStatusExceptionHandler.java @@ -20,6 +20,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import reactor.core.publisher.Mono; +import org.springframework.http.HttpStatus; import org.springframework.web.server.ResponseStatusException; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.server.WebExceptionHandler; @@ -39,10 +40,11 @@ public class ResponseStatusExceptionHandler implements WebExceptionHandler { @Override public Mono handle(ServerWebExchange exchange, Throwable ex) { if (ex instanceof ResponseStatusException) { - // Response may be committed but we'll try.. - logger.debug(ex.getMessage()); - exchange.getResponse().setStatusCode(((ResponseStatusException) ex).getStatus()); - return exchange.getResponse().setComplete(); + HttpStatus status = ((ResponseStatusException) ex).getStatus(); + if (exchange.getResponse().setStatusCode(status)) { + logger.trace(ex.getMessage()); + return exchange.getResponse().setComplete(); + } } return Mono.error(ex); } diff --git a/spring-web/src/test/java/org/springframework/web/server/handler/ResponseStatusExceptionHandlerTests.java b/spring-web/src/test/java/org/springframework/web/server/handler/ResponseStatusExceptionHandlerTests.java index 940cfa4088b..023a9a456df 100644 --- a/spring-web/src/test/java/org/springframework/web/server/handler/ResponseStatusExceptionHandlerTests.java +++ b/spring-web/src/test/java/org/springframework/web/server/handler/ResponseStatusExceptionHandlerTests.java @@ -59,10 +59,9 @@ public class ResponseStatusExceptionHandlerTests { public void responseCommitted() throws Exception { Throwable ex = new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, "Oops"); this.exchange.getResponse().setStatusCode(HttpStatus.CREATED); - this.exchange.getResponse().setComplete() - .then(this.handler.handle(this.exchange, ex)) - .block(Duration.ofSeconds(5)); - assertEquals(HttpStatus.CREATED, this.exchange.getResponse().getStatusCode()); + Mono mono = this.exchange.getResponse().setComplete() + .then(this.handler.handle(this.exchange, ex)); + StepVerifier.create(mono).consumeErrorWith(actual -> assertSame(ex, actual)).verify(); } }