Skip to content

Commit 2ccf787

Browse files
rstoyanchevrstoyanchev
committed
Move 500 error handling to HttpWebHandlerAdapter
Issue: SPR-15506
1 parent 1881727 commit 2ccf787

File tree

4 files changed

+72
-90
lines changed

4 files changed

+72
-90
lines changed

‎spring-web/src/main/java/org/springframework/web/server/adapter/HttpWebHandlerAdapter.java

Lines changed: 47 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,15 @@
1616

1717
package org.springframework.web.server.adapter;
1818

19+
import java.util.Arrays;
20+
import java.util.HashSet;
21+
import java.util.Set;
22+
1923
import org.apache.commons.logging.Log;
2024
import org.apache.commons.logging.LogFactory;
2125
import reactor.core.publisher.Mono;
2226

27+
import org.springframework.core.NestedCheckedException;
2328
import org.springframework.http.HttpStatus;
2429
import org.springframework.http.codec.ServerCodecConfigurer;
2530
import org.springframework.http.server.reactive.HttpHandler;
@@ -28,6 +33,7 @@
2833
import org.springframework.util.Assert;
2934
import org.springframework.web.server.ServerWebExchange;
3035
import org.springframework.web.server.WebHandler;
36+
import org.springframework.web.server.handler.ExceptionHandlingWebHandler;
3137
import org.springframework.web.server.handler.WebHandlerDecorator;
3238
import org.springframework.web.server.session.DefaultWebSessionManager;
3339
import org.springframework.web.server.session.WebSessionManager;
@@ -44,8 +50,27 @@
4450
*/
4551
public class HttpWebHandlerAdapter extends WebHandlerDecorator implements HttpHandler {
4652

53+
/**
54+
* Dedicated log category for disconnected client exceptions.
55+
* <p>Servlet containers do not expose a notification when a client disconnects,
56+
* e.g. <a href="https://java.net/jira/browse/SERVLET_SPEC-44">SERVLET_SPEC-44</a>.
57+
* <p>To avoid filling logs with unnecessary stack traces, we make an
58+
* effort to identify such network failures on a per-server basis, and then
59+
* log under a separate log category a simple one-line message at DEBUG level
60+
* or a full stack trace only at TRACE level.
61+
*/
62+
private static final String DISCONNECTED_CLIENT_LOG_CATEGORY =
63+
ExceptionHandlingWebHandler.class.getName() + ".DisconnectedClient";
64+
65+
private static final Set<String> DISCONNECTED_CLIENT_EXCEPTIONS =
66+
new HashSet<>(Arrays.asList("ClientAbortException", "EOFException", "EofException"));
67+
68+
4769
private static final Log logger = LogFactory.getLog(HttpWebHandlerAdapter.class);
4870

71+
private static final Log disconnectedClientLogger = LogFactory.getLog(DISCONNECTED_CLIENT_LOG_CATEGORY);
72+
73+
4974
private WebSessionManager sessionManager = new DefaultWebSessionManager();
5075

5176
private ServerCodecConfigurer codecConfigurer;
@@ -99,10 +124,8 @@ public Mono<Void> handle(ServerHttpRequest request, ServerHttpResponse response)
99124
ServerWebExchange exchange = createExchange(request, response);
100125
return getDelegate().handle(exchange)
101126
.onErrorResume(ex -> {
102-
if (logger.isDebugEnabled()) {
103-
logger.debug("Could not complete request", ex);
104-
}
105127
response.setStatusCode(HttpStatus.INTERNAL_SERVER_ERROR);
128+
logException(ex);
106129
return Mono.empty();
107130
})
108131
.then(Mono.defer(response::setComplete));
@@ -112,4 +135,25 @@ protected ServerWebExchange createExchange(ServerHttpRequest request, ServerHttp
112135
return new DefaultServerWebExchange(request, response, this.sessionManager, getCodecConfigurer());
113136
}
114137

138+
@SuppressWarnings("serial")
139+
private void logException(Throwable ex) {
140+
NestedCheckedException nestedEx = new NestedCheckedException("", ex) {};
141+
if ("Broken pipe".equalsIgnoreCase(nestedEx.getMostSpecificCause().getMessage()) ||
142+
DISCONNECTED_CLIENT_EXCEPTIONS.contains(ex.getClass().getSimpleName())) {
143+
144+
if (disconnectedClientLogger.isTraceEnabled()) {
145+
disconnectedClientLogger.trace("Looks like the client has gone away", ex);
146+
}
147+
else if (disconnectedClientLogger.isDebugEnabled()) {
148+
disconnectedClientLogger.debug(
149+
"The client has gone away: " + nestedEx.getMessage() +
150+
" (For a full stack trace, set the log category" +
151+
"'" + DISCONNECTED_CLIENT_LOG_CATEGORY + "' to TRACE)");
152+
}
153+
}
154+
else {
155+
logger.error("Could not complete request", ex);
156+
}
157+
}
158+
115159
}

‎spring-web/src/main/java/org/springframework/web/server/handler/ExceptionHandlingWebHandler.java

Lines changed: 1 addition & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -16,19 +16,11 @@
1616

1717
package org.springframework.web.server.handler;
1818

19-
import java.util.ArrayList;
20-
import java.util.Arrays;
2119
import java.util.Collections;
22-
import java.util.HashSet;
2320
import java.util.List;
24-
import java.util.Set;
2521

26-
import org.apache.commons.logging.Log;
27-
import org.apache.commons.logging.LogFactory;
2822
import reactor.core.publisher.Mono;
2923

30-
import org.springframework.core.NestedCheckedException;
31-
import org.springframework.http.HttpStatus;
3224
import org.springframework.web.server.ServerWebExchange;
3325
import org.springframework.web.server.WebExceptionHandler;
3426
import org.springframework.web.server.WebHandler;
@@ -42,40 +34,13 @@
4234
*/
4335
public class ExceptionHandlingWebHandler extends WebHandlerDecorator {
4436

45-
/**
46-
* Dedicated log category for disconnected client exceptions.
47-
* <p>Servlet containers do not expose a notification when a client disconnects,
48-
* e.g. <a href="https://java.net/jira/browse/SERVLET_SPEC-44">SERVLET_SPEC-44</a>.
49-
* <p>To avoid filling logs with unnecessary stack traces, we make an
50-
* effort to identify such network failures on a per-server basis, and then
51-
* log under a separate log category a simple one-line message at DEBUG level
52-
* or a full stack trace only at TRACE level.
53-
*/
54-
private static final String DISCONNECTED_CLIENT_LOG_CATEGORY =
55-
ExceptionHandlingWebHandler.class.getName() + ".DisconnectedClient";
56-
57-
private static final Set<String> DISCONNECTED_CLIENT_EXCEPTIONS =
58-
new HashSet<>(Arrays.asList("ClientAbortException", "EOFException", "EofException"));
59-
60-
61-
62-
private static final Log logger = LogFactory.getLog(ExceptionHandlingWebHandler.class);
63-
64-
private static final Log disconnectedClientLogger = LogFactory.getLog(DISCONNECTED_CLIENT_LOG_CATEGORY);
65-
6637

6738
private final List<WebExceptionHandler> exceptionHandlers;
6839

6940

7041
public ExceptionHandlingWebHandler(WebHandler delegate, List<WebExceptionHandler> handlers) {
7142
super(delegate);
72-
this.exceptionHandlers = initHandlers(handlers);
73-
}
74-
75-
private List<WebExceptionHandler> initHandlers(List<WebExceptionHandler> handlers) {
76-
List<WebExceptionHandler> result = new ArrayList<>(handlers);
77-
result.add(new UnresolvedExceptionHandler());
78-
return Collections.unmodifiableList(result);
43+
this.exceptionHandlers = Collections.unmodifiableList(handlers);
7944
}
8045

8146

@@ -105,37 +70,4 @@ public Mono<Void> handle(ServerWebExchange exchange) {
10570
return completion;
10671
}
10772

108-
109-
private static class UnresolvedExceptionHandler implements WebExceptionHandler {
110-
111-
112-
@Override
113-
public Mono<Void> handle(ServerWebExchange exchange, Throwable ex) {
114-
logException(ex);
115-
exchange.getResponse().setStatusCode(HttpStatus.INTERNAL_SERVER_ERROR);
116-
return exchange.getResponse().setComplete();
117-
}
118-
119-
@SuppressWarnings("serial")
120-
private void logException(Throwable ex) {
121-
NestedCheckedException nestedEx = new NestedCheckedException("", ex) {};
122-
if ("Broken pipe".equalsIgnoreCase(nestedEx.getMostSpecificCause().getMessage()) ||
123-
DISCONNECTED_CLIENT_EXCEPTIONS.contains(ex.getClass().getSimpleName())) {
124-
125-
if (disconnectedClientLogger.isTraceEnabled()) {
126-
disconnectedClientLogger.trace("Looks like the client has gone away", ex);
127-
}
128-
else if (disconnectedClientLogger.isDebugEnabled()) {
129-
disconnectedClientLogger.debug(
130-
"The client has gone away: " + nestedEx.getMessage() +
131-
" (For a full stack trace, set the log category" +
132-
"'" + DISCONNECTED_CLIENT_LOG_CATEGORY + "' to TRACE)");
133-
}
134-
}
135-
else {
136-
logger.error("Could not complete request", ex);
137-
}
138-
}
139-
}
140-
14173
}

‎spring-web/src/test/java/org/springframework/web/server/handler/ExceptionHandlingHttpHandlerTests.java renamed to ‎spring-web/src/test/java/org/springframework/web/server/handler/ExceptionHandlingWebHandlerTests.java

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2016 the original author or authors.
2+
* Copyright 2002-2017 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -20,61 +20,67 @@
2020

2121
import org.junit.Test;
2222
import reactor.core.publisher.Mono;
23+
import reactor.test.StepVerifier;
2324

2425
import org.springframework.http.HttpStatus;
2526
import org.springframework.mock.http.server.reactive.test.MockServerHttpRequest;
26-
import org.springframework.mock.http.server.reactive.test.MockServerWebExchange;
2727
import org.springframework.web.server.ServerWebExchange;
2828
import org.springframework.web.server.WebExceptionHandler;
2929
import org.springframework.web.server.WebHandler;
30+
import org.springframework.web.server.adapter.HttpWebHandlerAdapter;
3031

3132
import static org.junit.Assert.assertEquals;
33+
import static org.junit.Assert.assertNull;
3234

3335
/**
36+
* Unit tests for {@link ExceptionHandlingWebHandler}.
3437
* @author Rossen Stoyanchev
35-
* @since 5.0
3638
*/
37-
public class ExceptionHandlingHttpHandlerTests {
39+
public class ExceptionHandlingWebHandlerTests {
3840

39-
private final MockServerWebExchange exchange = MockServerHttpRequest.get("http://localhost:8080").toExchange();
41+
private final ServerWebExchange exchange = MockServerHttpRequest.get("http://localhost:8080").toExchange();
4042

4143
private final WebHandler targetHandler = new StubWebHandler(new IllegalStateException("boo"));
4244

4345

4446
@Test
4547
public void handleErrorSignal() throws Exception {
46-
WebExceptionHandler exceptionHandler = new BadRequestExceptionHandler();
47-
createWebHandler(exceptionHandler).handle(this.exchange).block();
48-
48+
createWebHandler(new BadRequestExceptionHandler()).handle(this.exchange).block();
4949
assertEquals(HttpStatus.BAD_REQUEST, this.exchange.getResponse().getStatusCode());
5050
}
5151

5252
@Test
5353
public void handleErrorSignalWithMultipleHttpErrorHandlers() throws Exception {
54-
WebExceptionHandler[] exceptionHandlers = new WebExceptionHandler[] {
54+
createWebHandler(
5555
new UnresolvedExceptionHandler(),
5656
new UnresolvedExceptionHandler(),
5757
new BadRequestExceptionHandler(),
58-
new UnresolvedExceptionHandler()
59-
};
60-
createWebHandler(exceptionHandlers).handle(this.exchange).block();
58+
new UnresolvedExceptionHandler()).handle(this.exchange).block();
6159

6260
assertEquals(HttpStatus.BAD_REQUEST, this.exchange.getResponse().getStatusCode());
6361
}
6462

6563
@Test
6664
public void unresolvedException() throws Exception {
67-
WebExceptionHandler exceptionHandler = new UnresolvedExceptionHandler();
68-
createWebHandler(exceptionHandler).handle(this.exchange).block();
65+
Mono<Void> mono = createWebHandler(new UnresolvedExceptionHandler()).handle(this.exchange);
66+
StepVerifier.create(mono).expectErrorMessage("boo").verify();
67+
assertNull(this.exchange.getResponse().getStatusCode());
68+
}
69+
70+
@Test
71+
public void unresolvedExceptionWithWebHttpHandlerAdapter() throws Exception {
72+
73+
// HttpWebHandlerAdapter handles unresolved errors
74+
75+
new HttpWebHandlerAdapter(createWebHandler(new UnresolvedExceptionHandler()))
76+
.handle(this.exchange.getRequest(), this.exchange.getResponse()).block();
6977

7078
assertEquals(HttpStatus.INTERNAL_SERVER_ERROR, this.exchange.getResponse().getStatusCode());
7179
}
7280

7381
@Test
7482
public void thrownExceptionBecomesErrorSignal() throws Exception {
75-
WebExceptionHandler exceptionHandler = new BadRequestExceptionHandler();
76-
createWebHandler(exceptionHandler).handle(this.exchange).block();
77-
83+
createWebHandler(new BadRequestExceptionHandler()).handle(this.exchange).block();
7884
assertEquals(HttpStatus.BAD_REQUEST, this.exchange.getResponse().getStatusCode());
7985
}
8086

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
</Appenders>
88
<Loggers>
99
<Logger name="org.springframework.http" level="debug" />
10-
<Logger name="org.springframework.web" level="debug" />
10+
<!--<Logger name="org.springframework.web" level="debug" />-->
1111
<Logger name="reactor" level="info" />
1212
<Logger name="io.reactivex" level="info" />
1313
<Root level="error">

0 commit comments

Comments
 (0)