Move 500 error handling to HttpWebHandlerAdapter

Issue: SPR-15506

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.
+	 * <p>Servlet containers do not expose a notification when a client disconnects,
+	 * e.g. <a href="https://java.net/jira/browse/SERVLET_SPEC-44">SERVLET_SPEC-44</a>.
+	 * <p>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<String> 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);
+		}
+	}
+
 }

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.
-	 * <p>Servlet containers do not expose a notification when a client disconnects,
-	 * e.g. <a href="https://java.net/jira/browse/SERVLET_SPEC-44">SERVLET_SPEC-44</a>.
-	 * <p>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<String> 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<WebExceptionHandler> exceptionHandlers;
 
 
 	public ExceptionHandlingWebHandler(WebHandler delegate, List<WebExceptionHandler> handlers) {
 		super(delegate);
-		this.exceptionHandlers = initHandlers(handlers);
-	}
-
-	private List<WebExceptionHandler> initHandlers(List<WebExceptionHandler> handlers) {
-		List<WebExceptionHandler> 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<Void> 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);
-			}
-		}
-	}
-
 }

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<Void> 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());
 	}
 

spring-webflux/src/test/resources/log4j2-test.xml

@@ -7,7 +7,7 @@
 	</Appenders>
 	<Loggers>
 		<Logger name="org.springframework.http" level="debug" />
-		<Logger name="org.springframework.web" level="debug" />
+		<!--<Logger name="org.springframework.web" level="debug" />-->
 		<Logger name="reactor" level="info" />
 		<Logger name="io.reactivex" level="info" />
 		<Root level="error">