AbstractServerHttpResponse skips commit actions on 2nd pass

Closes gh-25753
This commit is contained in:
Rossen Stoyanchev 2020-09-13 21:07:25 +01:00
parent 0db3f2b4de
commit b50ad1b9aa
2 changed files with 29 additions and 15 deletions

View File

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

View File

@ -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")));