From dc3d8340262527a0182ad72c7b756e8d9c555c32 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Mon, 27 Nov 2017 16:31:49 -0500 Subject: [PATCH] Refinements related to a committed response 1. setComplete checks the isCommitted flag to avoid an unnecessary debug message. This method is meant to be safe to call many times. 2. setStatusCode lowers log message to TRACE, since the return value communicates the outcome it's arguably much less critical. 3. Add comment and test case for ResponseStatusExceptionHandler. A ResponseStatusException is clearly meant to be handled by this handler so don't let it pass through even if the respones is committed. Issue: SPR-16231 --- .../server/reactive/AbstractServerHttpResponse.java | 8 ++++---- .../handler/ResponseStatusExceptionHandler.java | 7 ++++--- .../handler/ResponseStatusExceptionHandlerTests.java | 11 ++++++++++- 3 files changed, 18 insertions(+), 8 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 46100bde3e..c936aeec5b 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 @@ -89,9 +89,9 @@ public abstract class AbstractServerHttpResponse implements ServerHttpResponse { @Override public boolean setStatusCode(@Nullable HttpStatus statusCode) { if (this.state.get() == State.COMMITTED) { - if (logger.isDebugEnabled()) { - logger.debug("Can't set the status " + (statusCode != null ? statusCode.toString() : "null") + - " because the HTTP response has already been committed"); + if (logger.isTraceEnabled()) { + logger.trace("HTTP response already committed. " + + "Status not set to " + (statusCode != null ? statusCode.toString() : "null")); } return false; } @@ -183,7 +183,7 @@ public abstract class AbstractServerHttpResponse implements ServerHttpResponse { @Override public Mono setComplete() { - return doCommit(null); + return !isCommitted() ? doCommit(null) : Mono.empty(); } /** diff --git a/spring-web/src/main/java/org/springframework/web/server/handler/ResponseStatusExceptionHandler.java b/spring-web/src/main/java/org/springframework/web/server/handler/ResponseStatusExceptionHandler.java index 4ea86e65fb..81fe316e3c 100644 --- a/spring-web/src/main/java/org/springframework/web/server/handler/ResponseStatusExceptionHandler.java +++ b/spring-web/src/main/java/org/springframework/web/server/handler/ResponseStatusExceptionHandler.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-2017 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -39,11 +39,12 @@ public class ResponseStatusExceptionHandler implements WebExceptionHandler { @Override public Mono handle(ServerWebExchange exchange, Throwable ex) { if (ex instanceof ResponseStatusException) { - exchange.getResponse().setStatusCode(((ResponseStatusException) ex).getStatus()); + // Response may be committed but we'll try.. logger.debug(ex.getMessage()); + exchange.getResponse().setStatusCode(((ResponseStatusException) ex).getStatus()); return exchange.getResponse().setComplete(); } return Mono.error(ex); } -} +} \ No newline at end of file diff --git a/spring-web/src/test/java/org/springframework/web/server/handler/ResponseStatusExceptionHandlerTests.java b/spring-web/src/test/java/org/springframework/web/server/handler/ResponseStatusExceptionHandlerTests.java index 54df1e6f09..940cfa4088 100644 --- a/spring-web/src/test/java/org/springframework/web/server/handler/ResponseStatusExceptionHandlerTests.java +++ b/spring-web/src/test/java/org/springframework/web/server/handler/ResponseStatusExceptionHandlerTests.java @@ -32,7 +32,6 @@ import static org.junit.Assert.assertSame; /** * Unit tests for {@link ResponseStatusExceptionHandler}. - * * @author Rossen Stoyanchev */ public class ResponseStatusExceptionHandlerTests { @@ -56,4 +55,14 @@ public class ResponseStatusExceptionHandlerTests { StepVerifier.create(mono).consumeErrorWith(actual -> assertSame(expected, actual)).verify(); } + @Test // SPR-16231 + public void responseCommitted() throws Exception { + Throwable ex = new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, "Oops"); + this.exchange.getResponse().setStatusCode(HttpStatus.CREATED); + this.exchange.getResponse().setComplete() + .then(this.handler.handle(this.exchange, ex)) + .block(Duration.ofSeconds(5)); + assertEquals(HttpStatus.CREATED, this.exchange.getResponse().getStatusCode()); + } + }