diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpResponse.java b/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpResponse.java index 7074a3e252..0ca6b5128a 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpResponse.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpResponse.java @@ -18,6 +18,7 @@ package org.springframework.http.server.reactive; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Supplier; import java.util.stream.Collectors; @@ -46,6 +47,15 @@ import org.springframework.util.MultiValueMap; */ public abstract class AbstractServerHttpResponse implements ServerHttpResponse { + /** + * COMMITTING -> COMMITTED is the period after doCommit is called but before + * the response status and headers have been applied to the underlying + * response during which time pre-commit actions can still make changes to + * the response status and headers. + */ + private enum State {NEW, COMMITTING, COMMITTED}; + + private final Log logger = LogFactory.getLog(getClass()); private final DataBufferFactory dataBufferFactory; @@ -56,7 +66,7 @@ public abstract class AbstractServerHttpResponse implements ServerHttpResponse { private final MultiValueMap cookies; - private volatile boolean committed; + private final AtomicReference state = new AtomicReference<>(State.NEW); private final List>> commitActions = new ArrayList<>(4); @@ -77,7 +87,7 @@ public abstract class AbstractServerHttpResponse implements ServerHttpResponse { @Override public boolean setStatusCode(HttpStatus statusCode) { Assert.notNull(statusCode); - if (this.committed) { + if (this.state.get() == State.COMMITTED) { if (logger.isDebugEnabled()) { logger.debug("Can't set the status " + statusCode.toString() + " because the HTTP response has already been committed"); @@ -97,12 +107,14 @@ public abstract class AbstractServerHttpResponse implements ServerHttpResponse { @Override public HttpHeaders getHeaders() { - return (this.committed ? HttpHeaders.readOnlyHttpHeaders(this.headers) : this.headers); + return (this.state.get() == State.COMMITTED ? + HttpHeaders.readOnlyHttpHeaders(this.headers) : this.headers); } @Override public MultiValueMap getCookies() { - return (this.committed ? CollectionUtils.unmodifiableMultiValueMap(this.cookies) : this.cookies); + return (this.state.get() == State.COMMITTED ? + CollectionUtils.unmodifiableMultiValueMap(this.cookies) : this.cookies); } @Override @@ -144,7 +156,7 @@ public abstract class AbstractServerHttpResponse implements ServerHttpResponse { * @return a completion publisher */ protected Mono doCommit(Supplier> writeAction) { - if (this.committed) { + if (!this.state.compareAndSet(State.NEW, State.COMMITTING)) { if (logger.isDebugEnabled()) { logger.debug("Can't set the status " + statusCode.toString() + " because the HTTP response has already been committed"); @@ -152,12 +164,11 @@ public abstract class AbstractServerHttpResponse implements ServerHttpResponse { return Mono.empty(); } - this.committed = true; - this.commitActions.add(() -> { applyStatusCode(); applyHeaders(); applyCookies(); + this.state.set(State.COMMITTED); return Mono.empty(); }); diff --git a/spring-web/src/test/java/org/springframework/http/server/reactive/ServerHttpResponseTests.java b/spring-web/src/test/java/org/springframework/http/server/reactive/ServerHttpResponseTests.java index 34b8b03b23..92654c5cd4 100644 --- a/spring-web/src/test/java/org/springframework/http/server/reactive/ServerHttpResponseTests.java +++ b/spring-web/src/test/java/org/springframework/http/server/reactive/ServerHttpResponseTests.java @@ -33,7 +33,6 @@ import org.springframework.http.ResponseCookie; import static junit.framework.TestCase.assertTrue; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; /** @@ -101,24 +100,6 @@ public class ServerHttpResponseTests { assertEquals("c", new String(response.body.get(2).asByteBuffer().array(), StandardCharsets.UTF_8)); } - @Test - public void beforeCommitActionWithError() throws Exception { - TestServerHttpResponse response = new TestServerHttpResponse(); - IllegalStateException error = new IllegalStateException("boo"); - 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")); - - assertEquals(3, response.body.size()); - assertEquals("a", new String(response.body.get(0).asByteBuffer().array(), StandardCharsets.UTF_8)); - assertEquals("b", new String(response.body.get(1).asByteBuffer().array(), StandardCharsets.UTF_8)); - assertEquals("c", new String(response.body.get(2).asByteBuffer().array(), StandardCharsets.UTF_8)); - } - @Test public void beforeCommitActionWithSetComplete() throws Exception { ResponseCookie cookie = ResponseCookie.from("ID", "123").build();