From 4edc7196fb172cabe454dfc0377d322678b7ea7f Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Tue, 24 Sep 2019 11:09:42 +0100 Subject: [PATCH 1/4] Refine disconnected client handling in WebFlux If an error looks like a "disconnected client" but the response is not yet committed then it can't be an I/O error from writing to the server response. It is most likely as a result of a remote call as part of request handling. Not setting the response to 500 in this case results in a 200 response status despite the error. Even if it was an I/O error from the server response, setting the status won't impact a failed response. Closes gh-23319 --- .../web/server/adapter/HttpWebHandlerAdapter.java | 5 +++++ 1 file changed, 5 insertions(+) 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 56ae42aa23b..8f4788adafb 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 @@ -268,6 +268,11 @@ public class HttpWebHandlerAdapter extends WebHandlerDecorator implements HttpHa String logPrefix = exchange.getLogPrefix(); if (isDisconnectedClientError(ex)) { + // Request handling error (e.g. remote call), if we manage to set the status.. + if (response.setStatusCode(HttpStatus.INTERNAL_SERVER_ERROR)) { + logger.error(logPrefix + "500 Server Error for " + formatRequest(request), ex); + return Mono.empty(); + } if (lostClientLogger.isTraceEnabled()) { lostClientLogger.trace(logPrefix + "Client went away", ex); } From 27942644804732378806a6a560be6b321f74c0ce Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Tue, 24 Sep 2019 11:34:46 +0100 Subject: [PATCH 2/4] Fix JettyRequestUpgradeStrategy initialization bug Closes gh-23313 --- .../server/upgrade/JettyRequestUpgradeStrategy.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/socket/server/upgrade/JettyRequestUpgradeStrategy.java b/spring-webflux/src/main/java/org/springframework/web/reactive/socket/server/upgrade/JettyRequestUpgradeStrategy.java index f688756b22a..f5fe5cd5ece 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/socket/server/upgrade/JettyRequestUpgradeStrategy.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/socket/server/upgrade/JettyRequestUpgradeStrategy.java @@ -95,7 +95,6 @@ public class JettyRequestUpgradeStrategy implements RequestUpgradeStrategy, Life synchronized (this.lifecycleMonitor) { ServletContext servletContext = this.servletContext; if (!isRunning() && servletContext != null) { - this.running = true; try { this.factory = (this.webSocketPolicy != null ? new WebSocketServerFactory(servletContext, this.webSocketPolicy) : @@ -109,6 +108,7 @@ public class JettyRequestUpgradeStrategy implements RequestUpgradeStrategy, Life return container.getAdapter(); }); this.factory.start(); + this.running = true; } catch (Throwable ex) { throw new IllegalStateException("Unable to start WebSocketServerFactory", ex); @@ -121,10 +121,10 @@ public class JettyRequestUpgradeStrategy implements RequestUpgradeStrategy, Life public void stop() { synchronized (this.lifecycleMonitor) { if (isRunning()) { - this.running = false; if (this.factory != null) { try { this.factory.stop(); + this.running = false; } catch (Throwable ex) { throw new IllegalStateException("Failed to stop WebSocketServerFactory", ex); @@ -203,11 +203,11 @@ public class JettyRequestUpgradeStrategy implements RequestUpgradeStrategy, Life } private void startLazily(HttpServletRequest request) { - if (this.servletContext != null) { + if (isRunning()) { return; } synchronized (this.lifecycleMonitor) { - if (this.servletContext == null) { + if (!isRunning()) { this.servletContext = request.getServletContext(); start(); } From 99d9dacc4f6a95220ebe90455dac6410f2ffa649 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Tue, 24 Sep 2019 12:09:41 +0100 Subject: [PATCH 3/4] Log sendBufferSizeLimit exceeded at warn Closes gh-23534 --- .../web/socket/messaging/SubProtocolWebSocketHandler.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/spring-websocket/src/main/java/org/springframework/web/socket/messaging/SubProtocolWebSocketHandler.java b/spring-websocket/src/main/java/org/springframework/web/socket/messaging/SubProtocolWebSocketHandler.java index 0ff49f1549f..c4a90f69195 100644 --- a/spring-websocket/src/main/java/org/springframework/web/socket/messaging/SubProtocolWebSocketHandler.java +++ b/spring-websocket/src/main/java/org/springframework/web/socket/messaging/SubProtocolWebSocketHandler.java @@ -359,6 +359,9 @@ public class SubProtocolWebSocketHandler if (logger.isDebugEnabled()) { logger.debug("Terminating '" + session + "'", ex); } + else if (logger.isWarnEnabled()) { + logger.debug("Terminating '" + session + "': " + ex.getMessage()); + } this.stats.incrementLimitExceededCount(); clearSession(session, ex.getStatus()); // clear first, session may be unresponsive session.close(ex.getStatus()); From 955000699aa7675a698933057d607b4abd603d41 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Tue, 24 Sep 2019 12:58:52 +0100 Subject: [PATCH 4/4] Add getRawStatusCode() to ExchangeResult Closes gh-23630 --- .../http/client/reactive/MockClientHttpResponse.java | 2 +- .../test/web/reactive/server/ExchangeResult.java | 11 ++++++++++- .../test/web/reactive/server/StatusAssertions.java | 4 ++-- .../web/reactive/server/StatusAssertionTests.java | 11 ++++++++++- 4 files changed, 23 insertions(+), 5 deletions(-) diff --git a/spring-test/src/main/java/org/springframework/mock/http/client/reactive/MockClientHttpResponse.java b/spring-test/src/main/java/org/springframework/mock/http/client/reactive/MockClientHttpResponse.java index 528dd2b2b6f..9385899bff0 100644 --- a/spring-test/src/main/java/org/springframework/mock/http/client/reactive/MockClientHttpResponse.java +++ b/spring-test/src/main/java/org/springframework/mock/http/client/reactive/MockClientHttpResponse.java @@ -64,7 +64,7 @@ public class MockClientHttpResponse implements ClientHttpResponse { } public MockClientHttpResponse(int status) { - Assert.isTrue(status >= 100 && status < 600, "Status must be between 1xx and 5xx"); + Assert.isTrue(status > 99 && status < 1000, "Status must be between 100 and 999"); this.status = status; } diff --git a/spring-test/src/main/java/org/springframework/test/web/reactive/server/ExchangeResult.java b/spring-test/src/main/java/org/springframework/test/web/reactive/server/ExchangeResult.java index ba8f58bb12b..f94b49965ca 100644 --- a/spring-test/src/main/java/org/springframework/test/web/reactive/server/ExchangeResult.java +++ b/spring-test/src/main/java/org/springframework/test/web/reactive/server/ExchangeResult.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2017 the original author or authors. + * Copyright 2002-2019 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. @@ -161,6 +161,15 @@ public class ExchangeResult { return this.response.getStatusCode(); } + /** + * Return the HTTP status code (potentially non-standard and not resolvable + * through the {@link HttpStatus} enum) as an integer. + * @since 5.1.10 + */ + public int getRawStatusCode() { + return this.response.getRawStatusCode(); + } + /** * Return the response headers received from the server. */ diff --git a/spring-test/src/main/java/org/springframework/test/web/reactive/server/StatusAssertions.java b/spring-test/src/main/java/org/springframework/test/web/reactive/server/StatusAssertions.java index f6a91eecc22..cce9cb8f9a3 100644 --- a/spring-test/src/main/java/org/springframework/test/web/reactive/server/StatusAssertions.java +++ b/spring-test/src/main/java/org/springframework/test/web/reactive/server/StatusAssertions.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 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. @@ -55,7 +55,7 @@ public class StatusAssertions { * Assert the response status as an integer. */ public WebTestClient.ResponseSpec isEqualTo(int status) { - int actual = this.exchangeResult.getStatus().value(); + int actual = this.exchangeResult.getRawStatusCode(); this.exchangeResult.assertWithDiagnostics(() -> AssertionErrors.assertEquals("Status", status, actual)); return this.responseSpec; } diff --git a/spring-test/src/test/java/org/springframework/test/web/reactive/server/StatusAssertionTests.java b/spring-test/src/test/java/org/springframework/test/web/reactive/server/StatusAssertionTests.java index 7e77ee17576..5b6f1c459a3 100644 --- a/spring-test/src/test/java/org/springframework/test/web/reactive/server/StatusAssertionTests.java +++ b/spring-test/src/test/java/org/springframework/test/web/reactive/server/StatusAssertionTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 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. @@ -62,6 +62,11 @@ public class StatusAssertionTests { } } + @Test // gh-23630 + public void isEqualToWithCustomStatus() { + statusAssertions(600).isEqualTo(600); + } + @Test public void reasonEquals() { StatusAssertions assertions = statusAssertions(HttpStatus.CONFLICT); @@ -177,6 +182,10 @@ public class StatusAssertionTests { private StatusAssertions statusAssertions(HttpStatus status) { + return statusAssertions(status.value()); + } + + private StatusAssertions statusAssertions(int status) { MockClientHttpRequest request = new MockClientHttpRequest(HttpMethod.GET, URI.create("/")); MockClientHttpResponse response = new MockClientHttpResponse(status);