root
/
spring-framework
mirror of https://github.com/spring-projects/spring-framework.git
1
0
Fork 0
Code Issues Actions Packages Projects Releases Wiki Activity

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
This commit is contained in:
Rossen Stoyanchev 2017-11-29 12:39:06 -05:00
parent 873cb4e58b
commit dd0d270ba2
3 changed files with 19 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
spring-web/src/main/java/org/springframework/web/server/adapter/HttpWebHandlerAdapter.java
View File

@ -71,7 +71,7 @@ public class HttpWebHandlerAdapter extends WebHandlerDecorator implements HttpHa
* <p>TODO: * <p>TODO:
* This definition is currently duplicated between HttpWebHandlerAdapter * This definition is currently duplicated between HttpWebHandlerAdapter
* and AbstractSockJsSession. It is a candidate for a common utility class. * and AbstractSockJsSession. It is a candidate for a common utility class.
* @see #indicatesDisconnectedClient(Throwable) * @see #isDisconnectedClientError(Throwable)
*/ */
private static final Set<String> DISCONNECTED_CLIENT_EXCEPTIONS = private static final Set<String> DISCONNECTED_CLIENT_EXCEPTIONS =
new HashSet<>(Arrays.asList("ClientAbortException", "EOFException", "EofException")); new HashSet<>(Arrays.asList("ClientAbortException", "EOFException", "EofException"));
@ -158,8 +158,7 @@ public class HttpWebHandlerAdapter extends WebHandlerDecorator implements HttpHa
ServerWebExchange exchange = createExchange(request, response); ServerWebExchange exchange = createExchange(request, response);
return getDelegate().handle(exchange) return getDelegate().handle(exchange)
.onErrorResume(ex -> { .onErrorResume(ex -> {
response.setStatusCode(HttpStatus.INTERNAL_SERVER_ERROR); handleFailure(response, ex);
logHandleFailure(ex);
return Mono.empty(); return Mono.empty();
}) })
.then(Mono.defer(response::setComplete)); .then(Mono.defer(response::setComplete));
@ -170,8 +169,9 @@ public class HttpWebHandlerAdapter extends WebHandlerDecorator implements HttpHa
getCodecConfigurer(), getLocaleContextResolver()); getCodecConfigurer(), getLocaleContextResolver());
} }
private void logHandleFailure(Throwable ex) { private void handleFailure(ServerHttpResponse response, Throwable ex) {
if (indicatesDisconnectedClient(ex)) { boolean statusCodeChanged = response.setStatusCode(HttpStatus.INTERNAL_SERVER_ERROR);
if (isDisconnectedClientError(ex)) {
if (disconnectedClientLogger.isTraceEnabled()) { if (disconnectedClientLogger.isTraceEnabled()) {
disconnectedClientLogger.trace("Looks like the client has gone away", ex); disconnectedClientLogger.trace("Looks like the client has gone away", ex);
} }
@ -181,12 +181,16 @@ public class HttpWebHandlerAdapter extends WebHandlerDecorator implements HttpHa
"' to TRACE level.)"); "' to TRACE level.)");
} }
} }
else if (!statusCodeChanged) {
logger.error("Unhandled failure: " + ex.getMessage() + ", " +
"response already committed with status=" + response.getStatusCode());
}
else { else {
logger.error("Failed to handle request", ex); logger.error("Failed to handle request", ex);
} }
} }
private boolean indicatesDisconnectedClient(Throwable ex) { private boolean isDisconnectedClientError(Throwable ex) {
String message = NestedExceptionUtils.getMostSpecificCause(ex).getMessage(); String message = NestedExceptionUtils.getMostSpecificCause(ex).getMessage();
message = (message != null ? message.toLowerCase() : ""); message = (message != null ? message.toLowerCase() : "");
String className = ex.getClass().getSimpleName(); String className = ex.getClass().getSimpleName();

8
spring-web/src/main/java/org/springframework/web/server/handler/ResponseStatusExceptionHandler.java
View File

@ -20,6 +20,7 @@ import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory; import org.apache.commons.logging.LogFactory;
import reactor.core.publisher.Mono; import reactor.core.publisher.Mono;
import org.springframework.http.HttpStatus;
import org.springframework.web.server.ResponseStatusException; import org.springframework.web.server.ResponseStatusException;
import org.springframework.web.server.ServerWebExchange; import org.springframework.web.server.ServerWebExchange;
import org.springframework.web.server.WebExceptionHandler; import org.springframework.web.server.WebExceptionHandler;
@ -39,11 +40,12 @@ public class ResponseStatusExceptionHandler implements WebExceptionHandler {
@Override @Override
public Mono<Void> handle(ServerWebExchange exchange, Throwable ex) { public Mono<Void> handle(ServerWebExchange exchange, Throwable ex) {
if (ex instanceof ResponseStatusException) { if (ex instanceof ResponseStatusException) {
// Response may be committed but we'll try.. HttpStatus status = ((ResponseStatusException) ex).getStatus();
logger.debug(ex.getMessage()); if (exchange.getResponse().setStatusCode(status)) {
exchange.getResponse().setStatusCode(((ResponseStatusException) ex).getStatus()); logger.trace(ex.getMessage());
return exchange.getResponse().setComplete(); return exchange.getResponse().setComplete();
} }
}
return Mono.error(ex); return Mono.error(ex);
} }

7
spring-web/src/test/java/org/springframework/web/server/handler/ResponseStatusExceptionHandlerTests.java
View File

@ -59,10 +59,9 @@ public class ResponseStatusExceptionHandlerTests {
public void responseCommitted() throws Exception { public void responseCommitted() throws Exception {
Throwable ex = new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, "Oops"); Throwable ex = new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, "Oops");
this.exchange.getResponse().setStatusCode(HttpStatus.CREATED); this.exchange.getResponse().setStatusCode(HttpStatus.CREATED);
this.exchange.getResponse().setComplete() Mono<Void> mono = this.exchange.getResponse().setComplete()
.then(this.handler.handle(this.exchange, ex)) .then(this.handler.handle(this.exchange, ex));
.block(Duration.ofSeconds(5)); StepVerifier.create(mono).consumeErrorWith(actual -> assertSame(ex, actual)).verify();
assertEquals(HttpStatus.CREATED, this.exchange.getResponse().getStatusCode());
} }
} }