AbstractServerHttpResponse skips commit actions on 2nd pass

Closes gh-25753

Author: rstoyanchev

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 void beforeCommit(Supplier<? extends Mono<Void>> action) {
 
 	@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 @@ protected Mono<Void> doCommit() {
 	 * @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(() -> {

spring-web/src/test/java/org/springframework/http/server/reactive/ServerHttpResponseTests.java

@@ -33,6 +33,7 @@
 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 @@ void beforeCommitActionWithSetComplete() {
 		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 @@ void beforeCommitErrorShouldLeaveResponseNotCommitted() {
 			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")));