From c1518c3fdef8822e26ca4c82ad43a46dbb5dede1 Mon Sep 17 00:00:00 2001 From: Sebastien Deleuze Date: Mon, 4 Jul 2016 12:03:04 +0200 Subject: [PATCH] Avoid exception while setting the status code of a committed response Before this commit, it was not possible to set the status code of an HTTP response without throwing an exception if it was already committed. The consequence was a lot of errors in the logs for long lived HTTP exchanges like Server-Sent Events for example. After this commit, ServerHttpResponse#setStatusCode() returns true if the operation succeeded and false if the status code has not been set because the response has already been committed. In term of implementation, that makes status code managed consistently with headers and cookies: AbstractServerHttpResponse#setStatusCode() stores the status code that will be effectively set later in the response lifecycle by the HTTP server via AbstractServerHttpResponse#writeStatusCode() when the response will be committed. --- .../reactive/AbstractServerHttpResponse.java | 33 +++++++++++++++++++ .../reactive/ReactorServerHttpResponse.java | 13 +++++--- .../reactive/RxNettyServerHttpResponse.java | 13 +++++--- .../server/reactive/ServerHttpResponse.java | 5 ++- .../reactive/ServletServerHttpResponse.java | 11 ++++--- .../reactive/UndertowServerHttpResponse.java | 14 ++++---- .../reactive/MockServerHttpResponse.java | 3 +- .../reactive/ServerHttpResponseTests.java | 14 ++++++-- 8 files changed, 82 insertions(+), 24 deletions(-) diff --git a/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpResponse.java b/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpResponse.java index 2e478a1db4..a1132ef6a9 100644 --- a/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpResponse.java +++ b/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpResponse.java @@ -21,12 +21,15 @@ import java.util.List; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Supplier; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.reactivestreams.Publisher; import reactor.core.publisher.Mono; import org.springframework.core.io.buffer.DataBuffer; import org.springframework.core.io.buffer.DataBufferFactory; import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpStatus; import org.springframework.http.ResponseCookie; import org.springframework.util.Assert; import org.springframework.util.CollectionUtils; @@ -38,15 +41,20 @@ import org.springframework.util.MultiValueMap; * Base class for {@link ServerHttpResponse} implementations. * * @author Rossen Stoyanchev + * @author Sebastien Deleuze */ public abstract class AbstractServerHttpResponse implements ServerHttpResponse { + private static Log logger = LogFactory.getLog(AbstractServerHttpResponse.class); + private static final int STATE_NEW = 1; private static final int STATE_COMMITTING = 2; private static final int STATE_COMMITTED = 3; + private HttpStatus statusCode; + private final HttpHeaders headers; private final MultiValueMap cookies; @@ -70,6 +78,10 @@ public abstract class AbstractServerHttpResponse implements ServerHttpResponse { return this.dataBufferFactory; } + protected HttpStatus getStatusCode() { + return statusCode; + } + @Override public HttpHeaders getHeaders() { if (STATE_COMMITTED == this.state.get()) { @@ -106,6 +118,7 @@ public abstract class AbstractServerHttpResponse implements ServerHttpResponse { }); mono = mono.then(() -> { this.state.set(STATE_COMMITTED); + writeStatusCode(); writeHeaders(); writeCookies(); return Mono.empty(); @@ -114,6 +127,12 @@ public abstract class AbstractServerHttpResponse implements ServerHttpResponse { return mono; } + /** + * Implement this method to write the status code to the underlying response. + * This method is called once only. + */ + protected abstract void writeStatusCode(); + /** * Implement this method to apply header changes from {@link #getHeaders()} * to the underlying response. This method is called once only. @@ -143,4 +162,18 @@ public abstract class AbstractServerHttpResponse implements ServerHttpResponse { return applyBeforeCommit(); } + @Override + public boolean setStatusCode(HttpStatus statusCode) { + Assert.notNull(statusCode); + if (STATE_NEW == this.state.get()) { + this.statusCode = statusCode; + return true; + } + else if (logger.isDebugEnabled()) { + logger.debug("Can't set the status " + statusCode.toString() + + " because the HTTP response has already been committed"); + } + return false; + } + } diff --git a/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpResponse.java b/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpResponse.java index fd2467ec09..5ca6ca3bc8 100644 --- a/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpResponse.java +++ b/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpResponse.java @@ -60,11 +60,6 @@ public class ReactorServerHttpResponse extends AbstractServerHttpResponse return this.channel; } - @Override - public void setStatusCode(HttpStatus status) { - getReactorChannel().status(HttpResponseStatus.valueOf(status.value())); - } - @Override protected Mono writeWithInternal(Publisher publisher) { return Flux.from(publisher) @@ -75,6 +70,14 @@ public class ReactorServerHttpResponse extends AbstractServerHttpResponse .then(); } + @Override + protected void writeStatusCode() { + HttpStatus statusCode = this.getStatusCode(); + if (statusCode != null) { + getReactorChannel().status(HttpResponseStatus.valueOf(statusCode.value())); + } + } + @Override protected void writeHeaders() { for (String name : getHeaders().keySet()) { diff --git a/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/RxNettyServerHttpResponse.java b/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/RxNettyServerHttpResponse.java index 61aac8cc6f..80aadad457 100644 --- a/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/RxNettyServerHttpResponse.java +++ b/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/RxNettyServerHttpResponse.java @@ -59,11 +59,6 @@ public class RxNettyServerHttpResponse extends AbstractServerHttpResponse { return this.response; } - @Override - public void setStatusCode(HttpStatus status) { - this.response.setStatus(HttpResponseStatus.valueOf(status.value())); - } - @Override protected Mono writeWithInternal(Publisher body) { Observable content = RxJava1ObservableConverter.from(body).map(this::toByteBuf); @@ -75,6 +70,14 @@ public class RxNettyServerHttpResponse extends AbstractServerHttpResponse { return (buffer instanceof FlushingDataBuffer ? new FlushingByteBuf(byteBuf) : byteBuf); } + @Override + protected void writeStatusCode() { + HttpStatus statusCode = this.getStatusCode(); + if (statusCode != null) { + this.response.setStatus(HttpResponseStatus.valueOf(statusCode.value())); + } + } + @Override protected void writeHeaders() { for (String name : getHeaders().keySet()) { diff --git a/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/ServerHttpResponse.java b/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/ServerHttpResponse.java index 4e641499f5..c458cfcc7e 100644 --- a/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/ServerHttpResponse.java +++ b/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/ServerHttpResponse.java @@ -27,14 +27,17 @@ import org.springframework.util.MultiValueMap; * Represents a reactive server-side HTTP response. * * @author Arjen Poutsma + * @author Sebastien Deleuze */ public interface ServerHttpResponse extends ReactiveHttpOutputMessage { /** * Set the HTTP status code of the response. * @param status the HTTP status as an {@link HttpStatus} enum value + * @return {@code false} if the status code has not been set because the HTTP response + * is already committed, {@code true} if it has been set correctly. */ - void setStatusCode(HttpStatus status); + boolean setStatusCode(HttpStatus status); /** * Return a mutable map with the cookies to send to the server. diff --git a/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/ServletServerHttpResponse.java b/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/ServletServerHttpResponse.java index 926315c204..60cca26f23 100644 --- a/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/ServletServerHttpResponse.java +++ b/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/ServletServerHttpResponse.java @@ -63,13 +63,16 @@ public class ServletServerHttpResponse extends AbstractServerHttpResponse { } @Override - public void setStatusCode(HttpStatus status) { - getServletResponse().setStatus(status.value()); + protected Mono writeWithInternal(Publisher publisher) { + return this.responseBodyWriter.apply(publisher); } @Override - protected Mono writeWithInternal(Publisher publisher) { - return this.responseBodyWriter.apply(publisher); + protected void writeStatusCode() { + HttpStatus statusCode = this.getStatusCode(); + if (statusCode != null) { + getServletResponse().setStatus(statusCode.value()); + } } @Override diff --git a/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpResponse.java b/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpResponse.java index 0dfb54f26b..73fba1d28a 100644 --- a/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpResponse.java +++ b/spring-web-reactive/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpResponse.java @@ -66,12 +66,6 @@ public class UndertowServerHttpResponse extends AbstractServerHttpResponse return this.exchange; } - @Override - public void setStatusCode(HttpStatus status) { - Assert.notNull(status); - getUndertowExchange().setStatusCode(status.value()); - } - @Override protected Mono writeWithInternal(Publisher publisher) { return Mono.from(s -> { @@ -107,6 +101,14 @@ public class UndertowServerHttpResponse extends AbstractServerHttpResponse } } + @Override + protected void writeStatusCode() { + HttpStatus statusCode = this.getStatusCode(); + if (statusCode != null) { + getUndertowExchange().setStatusCode(statusCode.value()); + } + } + @Override protected void writeHeaders() { for (Map.Entry> entry : getHeaders().entrySet()) { diff --git a/spring-web-reactive/src/test/java/org/springframework/http/server/reactive/MockServerHttpResponse.java b/spring-web-reactive/src/test/java/org/springframework/http/server/reactive/MockServerHttpResponse.java index 30dbda1681..c12ba69f94 100644 --- a/spring-web-reactive/src/test/java/org/springframework/http/server/reactive/MockServerHttpResponse.java +++ b/spring-web-reactive/src/test/java/org/springframework/http/server/reactive/MockServerHttpResponse.java @@ -49,8 +49,9 @@ public class MockServerHttpResponse implements ServerHttpResponse { @Override - public void setStatusCode(HttpStatus status) { + public boolean setStatusCode(HttpStatus status) { this.status = status; + return true; } public HttpStatus getStatus() { diff --git a/spring-web-reactive/src/test/java/org/springframework/http/server/reactive/ServerHttpResponseTests.java b/spring-web-reactive/src/test/java/org/springframework/http/server/reactive/ServerHttpResponseTests.java index 6eff9434bf..a5e5637605 100644 --- a/spring-web-reactive/src/test/java/org/springframework/http/server/reactive/ServerHttpResponseTests.java +++ b/spring-web-reactive/src/test/java/org/springframework/http/server/reactive/ServerHttpResponseTests.java @@ -28,7 +28,6 @@ import reactor.core.publisher.Mono; import org.springframework.core.io.buffer.DataBuffer; import org.springframework.core.io.buffer.DefaultDataBufferFactory; -import org.springframework.http.HttpStatus; import org.springframework.http.ResponseCookie; import static junit.framework.TestCase.assertTrue; @@ -36,6 +35,7 @@ import static org.junit.Assert.*; /** * @author Rossen Stoyanchev + * @author Sebastien Deleuze */ public class ServerHttpResponseTests { @@ -47,6 +47,7 @@ public class ServerHttpResponseTests { TestServerHttpResponse response = new TestServerHttpResponse(); response.writeWith(Flux.just(wrap("a"), wrap("b"), wrap("c"))).block(); + assertTrue(response.statusCodeWritten); assertTrue(response.headersWritten); assertTrue(response.cookiesWritten); @@ -62,6 +63,7 @@ public class ServerHttpResponseTests { IllegalStateException error = new IllegalStateException("boo"); response.writeWith(Flux.error(error)).otherwise(ex -> Mono.empty()).block(); + assertFalse(response.statusCodeWritten); assertFalse(response.headersWritten); assertFalse(response.cookiesWritten); assertTrue(response.body.isEmpty()); @@ -72,6 +74,7 @@ public class ServerHttpResponseTests { TestServerHttpResponse response = new TestServerHttpResponse(); response.setComplete().block(); + assertTrue(response.statusCodeWritten); assertTrue(response.headersWritten); assertTrue(response.cookiesWritten); assertTrue(response.body.isEmpty()); @@ -87,6 +90,7 @@ public class ServerHttpResponseTests { }); response.writeWith(Flux.just(wrap("a"), wrap("b"), wrap("c"))).block(); + assertTrue(response.statusCodeWritten); assertTrue(response.headersWritten); assertTrue(response.cookiesWritten); assertSame(cookie, response.getCookies().getFirst("ID")); @@ -104,6 +108,7 @@ public class ServerHttpResponseTests { response.beforeCommit(() -> Mono.error(error)); response.writeWith(Flux.just(wrap("a"), wrap("b"), wrap("c"))).block(); + assertTrue("beforeCommit action errors should be ignored", response.statusCodeWritten); assertTrue("beforeCommit action errors should be ignored", response.headersWritten); assertTrue("beforeCommit action errors should be ignored", response.cookiesWritten); assertNull(response.getCookies().get("ID")); @@ -124,6 +129,7 @@ public class ServerHttpResponseTests { }); response.setComplete().block(); + assertTrue(response.statusCodeWritten); assertTrue(response.headersWritten); assertTrue(response.cookiesWritten); assertTrue(response.body.isEmpty()); @@ -139,6 +145,8 @@ public class ServerHttpResponseTests { private static class TestServerHttpResponse extends AbstractServerHttpResponse { + private boolean statusCodeWritten; + private boolean headersWritten; private boolean cookiesWritten; @@ -150,7 +158,9 @@ public class ServerHttpResponseTests { } @Override - public void setStatusCode(HttpStatus status) { + public void writeStatusCode() { + assertFalse(this.statusCodeWritten); + this.statusCodeWritten = true; } @Override