From b50ad1b9aab82f0220982b497b1867cfe1ea7a46 Mon Sep 17 00:00:00 2001
From: Rossen Stoyanchev <rstoyanchev@vmware.com>
Date: Sun, 13 Sep 2020 21:07:25 +0100
Subject: [PATCH] AbstractServerHttpResponse skips commit actions on 2nd pass

Closes gh-25753
---
 .../reactive/AbstractServerHttpResponse.java  | 32 +++++++++++--------
 .../reactive/ServerHttpResponseTests.java     | 12 ++++++-
 2 files changed, 29 insertions(+), 15 deletions(-)

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 517a2436ba..23476445b6 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
@@ -57,7 +57,7 @@ public abstract class AbstractServerHttpResponse implements ServerHttpResponse {
 	 * response during which time pre-commit actions can still make changes to
 	 * the response status and headers.
 	 */
-	private enum State {NEW, COMMITTING, COMMITTED}
+	private enum State {NEW, COMMITTING, COMMIT_ACTION_FAILED, COMMITTED}
 
 	protected final Log logger = HttpLogging.forLogName(getClass());
 
@@ -204,7 +204,8 @@ public abstract class AbstractServerHttpResponse implements ServerHttpResponse {
 
 	@Override
 	public boolean isCommitted() {
-		return this.state.get() != State.NEW;
+		State state = this.state.get();
+		return (state != State.NEW && state != State.COMMIT_ACTION_FAILED);
 	}
 
 	@Override
@@ -251,19 +252,22 @@ public abstract class AbstractServerHttpResponse implements ServerHttpResponse {
 	 * @return a completion publisher
 	 */
 	protected Mono<Void> doCommit(@Nullable Supplier<? extends Mono<Void>> writeAction) {
-		if (!this.state.compareAndSet(State.NEW, State.COMMITTING)) {
-			return Mono.empty();
-		}
-
 		Flux<Void> allActions = Flux.empty();
-
-		if (!this.commitActions.isEmpty()) {
-			allActions = Flux.concat(Flux.fromIterable(this.commitActions).map(Supplier::get))
-					.doOnError(ex -> {
-						if (this.state.compareAndSet(State.COMMITTING, State.NEW)) {
-							getHeaders().clearContentHeaders();
-						}
-					});
+		if (this.state.compareAndSet(State.NEW, State.COMMITTING)) {
+			if (!this.commitActions.isEmpty()) {
+				allActions = Flux.concat(Flux.fromIterable(this.commitActions).map(Supplier::get))
+						.doOnError(ex -> {
+							if (this.state.compareAndSet(State.COMMITTING, State.COMMIT_ACTION_FAILED)) {
+								getHeaders().clearContentHeaders();
+							}
+						});
+			}
+		}
+		else if (this.state.compareAndSet(State.COMMIT_ACTION_FAILED, State.COMMITTING)) {
+			// Skip commit actions
+		}
+		else {
+			return Mono.empty();
 		}
 
 		allActions = allActions.concatWith(Mono.fromRunnable(() -> {
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 431ec473ae..16c96fc643 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,6 +33,7 @@ import org.springframework.core.io.buffer.DataBuffer;
 import org.springframework.core.io.buffer.DefaultDataBuffer;
 import org.springframework.core.io.buffer.DefaultDataBufferFactory;
 import org.springframework.http.HttpHeaders;
+import org.springframework.http.HttpStatus;
 import org.springframework.http.MediaType;
 import org.springframework.http.ResponseCookie;
 
@@ -150,7 +151,7 @@ public class ServerHttpResponseTests {
 		assertThat(response.getCookies().getFirst("ID")).isSameAs(cookie);
 	}
 
-	@Test // gh-24186
+	@Test // gh-24186, gh-25753
 	void beforeCommitErrorShouldLeaveResponseNotCommitted() {
 
 		Consumer<Supplier<Mono<Void>>> tester = preCommitAction -> {
@@ -168,6 +169,15 @@ public class ServerHttpResponseTests {
 			assertThat(response.cookiesWritten).isFalse();
 			assertThat(response.isCommitted()).isFalse();
 			assertThat(response.getHeaders()).isEmpty();
+
+			// Handle the error
+			response.setStatusCode(HttpStatus.SERVICE_UNAVAILABLE);
+			StepVerifier.create(response.setComplete()).verifyComplete();
+
+			assertThat(response.statusCodeWritten).isTrue();
+			assertThat(response.headersWritten).isTrue();
+			assertThat(response.cookiesWritten).isTrue();
+			assertThat(response.isCommitted()).isTrue();
 		};
 
 		tester.accept(() -> Mono.error(new IllegalStateException("Max sessions")));