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.

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<String, ResponseCookie> 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;
+	}
+
 }

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<Void> writeWithInternal(Publisher<DataBuffer> 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()) {

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<Void> writeWithInternal(Publisher<DataBuffer> body) {
 		Observable<ByteBuf> 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()) {

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.

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<Void> writeWithInternal(Publisher<DataBuffer> publisher) {
+		return this.responseBodyWriter.apply(publisher);
 	}
 
 	@Override
-	protected Mono<Void> writeWithInternal(Publisher<DataBuffer> publisher) {
-		return this.responseBodyWriter.apply(publisher);
+	protected void writeStatusCode() {
+		HttpStatus statusCode = this.getStatusCode();
+		if (statusCode != null) {
+			getServletResponse().setStatus(statusCode.value());
+		}
 	}
 
 	@Override

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<Void> writeWithInternal(Publisher<DataBuffer> 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<String, List<String>> entry : getHeaders().entrySet()) {

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() {

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