From 2ccf78743af231b9f1eb9443fca52c3b18c647b5 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Tue, 2 May 2017 16:44:37 -0400 Subject: [PATCH] Move 500 error handling to HttpWebHandlerAdapter Issue: SPR-15506 --- .../server/adapter/HttpWebHandlerAdapter.java | 50 ++++++++++++- .../handler/ExceptionHandlingWebHandler.java | 70 +------------------ ... => ExceptionHandlingWebHandlerTests.java} | 40 ++++++----- .../src/test/resources/log4j2-test.xml | 2 +- 4 files changed, 72 insertions(+), 90 deletions(-) rename spring-web/src/test/java/org/springframework/web/server/handler/{ExceptionHandlingHttpHandlerTests.java => ExceptionHandlingWebHandlerTests.java} (72%) 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 95814850ba6..e4cd24d9fd5 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 @@ -16,10 +16,15 @@ package org.springframework.web.server.adapter; +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; + import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import reactor.core.publisher.Mono; +import org.springframework.core.NestedCheckedException; import org.springframework.http.HttpStatus; import org.springframework.http.codec.ServerCodecConfigurer; import org.springframework.http.server.reactive.HttpHandler; @@ -28,6 +33,7 @@ import org.springframework.http.server.reactive.ServerHttpResponse; import org.springframework.util.Assert; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.server.WebHandler; +import org.springframework.web.server.handler.ExceptionHandlingWebHandler; import org.springframework.web.server.handler.WebHandlerDecorator; import org.springframework.web.server.session.DefaultWebSessionManager; import org.springframework.web.server.session.WebSessionManager; @@ -44,8 +50,27 @@ import org.springframework.web.server.session.WebSessionManager; */ public class HttpWebHandlerAdapter extends WebHandlerDecorator implements HttpHandler { + /** + * Dedicated log category for disconnected client exceptions. + *

Servlet containers do not expose a notification when a client disconnects, + * e.g. SERVLET_SPEC-44. + *

To avoid filling logs with unnecessary stack traces, we make an + * effort to identify such network failures on a per-server basis, and then + * log under a separate log category a simple one-line message at DEBUG level + * or a full stack trace only at TRACE level. + */ + private static final String DISCONNECTED_CLIENT_LOG_CATEGORY = + ExceptionHandlingWebHandler.class.getName() + ".DisconnectedClient"; + + private static final Set DISCONNECTED_CLIENT_EXCEPTIONS = + new HashSet<>(Arrays.asList("ClientAbortException", "EOFException", "EofException")); + + private static final Log logger = LogFactory.getLog(HttpWebHandlerAdapter.class); + private static final Log disconnectedClientLogger = LogFactory.getLog(DISCONNECTED_CLIENT_LOG_CATEGORY); + + private WebSessionManager sessionManager = new DefaultWebSessionManager(); private ServerCodecConfigurer codecConfigurer; @@ -99,10 +124,8 @@ public class HttpWebHandlerAdapter extends WebHandlerDecorator implements HttpHa ServerWebExchange exchange = createExchange(request, response); return getDelegate().handle(exchange) .onErrorResume(ex -> { - if (logger.isDebugEnabled()) { - logger.debug("Could not complete request", ex); - } response.setStatusCode(HttpStatus.INTERNAL_SERVER_ERROR); + logException(ex); return Mono.empty(); }) .then(Mono.defer(response::setComplete)); @@ -112,4 +135,25 @@ public class HttpWebHandlerAdapter extends WebHandlerDecorator implements HttpHa return new DefaultServerWebExchange(request, response, this.sessionManager, getCodecConfigurer()); } + @SuppressWarnings("serial") + private void logException(Throwable ex) { + NestedCheckedException nestedEx = new NestedCheckedException("", ex) {}; + if ("Broken pipe".equalsIgnoreCase(nestedEx.getMostSpecificCause().getMessage()) || + DISCONNECTED_CLIENT_EXCEPTIONS.contains(ex.getClass().getSimpleName())) { + + if (disconnectedClientLogger.isTraceEnabled()) { + disconnectedClientLogger.trace("Looks like the client has gone away", ex); + } + else if (disconnectedClientLogger.isDebugEnabled()) { + disconnectedClientLogger.debug( + "The client has gone away: " + nestedEx.getMessage() + + " (For a full stack trace, set the log category" + + "'" + DISCONNECTED_CLIENT_LOG_CATEGORY + "' to TRACE)"); + } + } + else { + logger.error("Could not complete request", ex); + } + } + } diff --git a/spring-web/src/main/java/org/springframework/web/server/handler/ExceptionHandlingWebHandler.java b/spring-web/src/main/java/org/springframework/web/server/handler/ExceptionHandlingWebHandler.java index d6ef736f304..4d0e976a366 100644 --- a/spring-web/src/main/java/org/springframework/web/server/handler/ExceptionHandlingWebHandler.java +++ b/spring-web/src/main/java/org/springframework/web/server/handler/ExceptionHandlingWebHandler.java @@ -16,19 +16,11 @@ package org.springframework.web.server.handler; -import java.util.ArrayList; -import java.util.Arrays; import java.util.Collections; -import java.util.HashSet; import java.util.List; -import java.util.Set; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; import reactor.core.publisher.Mono; -import org.springframework.core.NestedCheckedException; -import org.springframework.http.HttpStatus; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.server.WebExceptionHandler; import org.springframework.web.server.WebHandler; @@ -42,40 +34,13 @@ import org.springframework.web.server.WebHandler; */ public class ExceptionHandlingWebHandler extends WebHandlerDecorator { - /** - * Dedicated log category for disconnected client exceptions. - *

Servlet containers do not expose a notification when a client disconnects, - * e.g. SERVLET_SPEC-44. - *

To avoid filling logs with unnecessary stack traces, we make an - * effort to identify such network failures on a per-server basis, and then - * log under a separate log category a simple one-line message at DEBUG level - * or a full stack trace only at TRACE level. - */ - private static final String DISCONNECTED_CLIENT_LOG_CATEGORY = - ExceptionHandlingWebHandler.class.getName() + ".DisconnectedClient"; - - private static final Set DISCONNECTED_CLIENT_EXCEPTIONS = - new HashSet<>(Arrays.asList("ClientAbortException", "EOFException", "EofException")); - - - - private static final Log logger = LogFactory.getLog(ExceptionHandlingWebHandler.class); - - private static final Log disconnectedClientLogger = LogFactory.getLog(DISCONNECTED_CLIENT_LOG_CATEGORY); - private final List exceptionHandlers; public ExceptionHandlingWebHandler(WebHandler delegate, List handlers) { super(delegate); - this.exceptionHandlers = initHandlers(handlers); - } - - private List initHandlers(List handlers) { - List result = new ArrayList<>(handlers); - result.add(new UnresolvedExceptionHandler()); - return Collections.unmodifiableList(result); + this.exceptionHandlers = Collections.unmodifiableList(handlers); } @@ -105,37 +70,4 @@ public class ExceptionHandlingWebHandler extends WebHandlerDecorator { return completion; } - - private static class UnresolvedExceptionHandler implements WebExceptionHandler { - - - @Override - public Mono handle(ServerWebExchange exchange, Throwable ex) { - logException(ex); - exchange.getResponse().setStatusCode(HttpStatus.INTERNAL_SERVER_ERROR); - return exchange.getResponse().setComplete(); - } - - @SuppressWarnings("serial") - private void logException(Throwable ex) { - NestedCheckedException nestedEx = new NestedCheckedException("", ex) {}; - if ("Broken pipe".equalsIgnoreCase(nestedEx.getMostSpecificCause().getMessage()) || - DISCONNECTED_CLIENT_EXCEPTIONS.contains(ex.getClass().getSimpleName())) { - - if (disconnectedClientLogger.isTraceEnabled()) { - disconnectedClientLogger.trace("Looks like the client has gone away", ex); - } - else if (disconnectedClientLogger.isDebugEnabled()) { - disconnectedClientLogger.debug( - "The client has gone away: " + nestedEx.getMessage() + - " (For a full stack trace, set the log category" + - "'" + DISCONNECTED_CLIENT_LOG_CATEGORY + "' to TRACE)"); - } - } - else { - logger.error("Could not complete request", ex); - } - } - } - } diff --git a/spring-web/src/test/java/org/springframework/web/server/handler/ExceptionHandlingHttpHandlerTests.java b/spring-web/src/test/java/org/springframework/web/server/handler/ExceptionHandlingWebHandlerTests.java similarity index 72% rename from spring-web/src/test/java/org/springframework/web/server/handler/ExceptionHandlingHttpHandlerTests.java rename to spring-web/src/test/java/org/springframework/web/server/handler/ExceptionHandlingWebHandlerTests.java index a241345cfb3..46b84744588 100644 --- a/spring-web/src/test/java/org/springframework/web/server/handler/ExceptionHandlingHttpHandlerTests.java +++ b/spring-web/src/test/java/org/springframework/web/server/handler/ExceptionHandlingWebHandlerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-2017 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -20,61 +20,67 @@ import java.util.Arrays; import org.junit.Test; import reactor.core.publisher.Mono; +import reactor.test.StepVerifier; import org.springframework.http.HttpStatus; import org.springframework.mock.http.server.reactive.test.MockServerHttpRequest; -import org.springframework.mock.http.server.reactive.test.MockServerWebExchange; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.server.WebExceptionHandler; import org.springframework.web.server.WebHandler; +import org.springframework.web.server.adapter.HttpWebHandlerAdapter; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; /** + * Unit tests for {@link ExceptionHandlingWebHandler}. * @author Rossen Stoyanchev - * @since 5.0 */ -public class ExceptionHandlingHttpHandlerTests { +public class ExceptionHandlingWebHandlerTests { - private final MockServerWebExchange exchange = MockServerHttpRequest.get("http://localhost:8080").toExchange(); + private final ServerWebExchange exchange = MockServerHttpRequest.get("http://localhost:8080").toExchange(); private final WebHandler targetHandler = new StubWebHandler(new IllegalStateException("boo")); @Test public void handleErrorSignal() throws Exception { - WebExceptionHandler exceptionHandler = new BadRequestExceptionHandler(); - createWebHandler(exceptionHandler).handle(this.exchange).block(); - + createWebHandler(new BadRequestExceptionHandler()).handle(this.exchange).block(); assertEquals(HttpStatus.BAD_REQUEST, this.exchange.getResponse().getStatusCode()); } @Test public void handleErrorSignalWithMultipleHttpErrorHandlers() throws Exception { - WebExceptionHandler[] exceptionHandlers = new WebExceptionHandler[] { + createWebHandler( new UnresolvedExceptionHandler(), new UnresolvedExceptionHandler(), new BadRequestExceptionHandler(), - new UnresolvedExceptionHandler() - }; - createWebHandler(exceptionHandlers).handle(this.exchange).block(); + new UnresolvedExceptionHandler()).handle(this.exchange).block(); assertEquals(HttpStatus.BAD_REQUEST, this.exchange.getResponse().getStatusCode()); } @Test public void unresolvedException() throws Exception { - WebExceptionHandler exceptionHandler = new UnresolvedExceptionHandler(); - createWebHandler(exceptionHandler).handle(this.exchange).block(); + Mono mono = createWebHandler(new UnresolvedExceptionHandler()).handle(this.exchange); + StepVerifier.create(mono).expectErrorMessage("boo").verify(); + assertNull(this.exchange.getResponse().getStatusCode()); + } + + @Test + public void unresolvedExceptionWithWebHttpHandlerAdapter() throws Exception { + + // HttpWebHandlerAdapter handles unresolved errors + + new HttpWebHandlerAdapter(createWebHandler(new UnresolvedExceptionHandler())) + .handle(this.exchange.getRequest(), this.exchange.getResponse()).block(); assertEquals(HttpStatus.INTERNAL_SERVER_ERROR, this.exchange.getResponse().getStatusCode()); } @Test public void thrownExceptionBecomesErrorSignal() throws Exception { - WebExceptionHandler exceptionHandler = new BadRequestExceptionHandler(); - createWebHandler(exceptionHandler).handle(this.exchange).block(); - + createWebHandler(new BadRequestExceptionHandler()).handle(this.exchange).block(); assertEquals(HttpStatus.BAD_REQUEST, this.exchange.getResponse().getStatusCode()); } diff --git a/spring-webflux/src/test/resources/log4j2-test.xml b/spring-webflux/src/test/resources/log4j2-test.xml index 7cacb266ab4..4c57ce96510 100644 --- a/spring-webflux/src/test/resources/log4j2-test.xml +++ b/spring-webflux/src/test/resources/log4j2-test.xml @@ -7,7 +7,7 @@ - +