Affects: v5.0.5.RELEASE to v6.1.0-M1
Problem
When an error occurs while sending data via a ResponseBodyEmitter
, e.g. trying to send an unconvertable object, the TCP connection is left open and you can still send data using .send()
.
However calls to .complete()
and .completeWithError()
are ignored which means there is no way to close the TCP connection apart from IOExceptions.
Reproduction
- Get spring boot with the spring web dependency from https://start.spring.io/.
- Create a simple Controller with four endpoints like this:
@RestController
public class TestController {
private ResponseBodyEmitter responseBodyEmitter;
@GetMapping("/open")
public ResponseBodyEmitter handleOpen() {
return responseBodyEmitter = new ResponseBodyEmitter(Long.MAX_VALUE);
}
@GetMapping("/send")
public void handleSend() throws IOException {
responseBodyEmitter.send("Test\n");
}
@GetMapping("/break")
public void handleBreak() throws IOException {
responseBodyEmitter.send(new Object());
}
@GetMapping("/close")
public void handleClose() {
responseBodyEmitter.complete();
}
@ExceptionHandler
public void handleExceptions(Throwable ex) {
ex.printStackTrace();
}
}
- Open a connection to the
/open
endpoint. For example usingcurl localhost:8080/open
: - Test if sending works by triggering the
/send
endpoint in another window. - Test if closing works by triggering the
/close
endpoint. - Reopen the closed connection (
/open
). - Call the
/break
endpoint to break the ResponseBodyEmitter instance. This is done by trying to send an instance ofObject
which cannot be converted by the default converters. - Test sending (
/send
). It still works. - Test closing (
/close
). It doesn't work. - You can also replace
complete()
withcompleteWithError(<some exception>)
to test that completeWithError does not work as well.
Cause
Calls to complete()
and completeWithError()
are ignored if a send failed before (if sendFailed
is true).
In ResponseBodyEmitter.sendInternal()
handler.send()
is called. (handler
is an instance of ResponseBodyEmitterReturnValueHandler
) If it throws any Throwable
sendFailed
is set to true
. If handler.send()
somehow completed the request, this would be fine, but as shown above this is not necessarily the case.
sendInternal()
from ResponseBodyEmitter.java
for reference:
private void sendInternal(Object object, @Nullable MediaType mediaType) throws IOException {
if (this.handler != null) {
try {
this.handler.send(object, mediaType);
}
catch (IOException ex) {
this.sendFailed = true;
throw ex;
}
catch (Throwable ex) {
this.sendFailed = true;
throw new IllegalStateException("Failed to send " + object, ex);
}
}
else {
this.earlySendAttempts.add(new DataWithMediaType(object, mediaType));
}
}
This was introduced in https://github.com/spring-projects/spring-framework/commit/568c93457a8487b34381386f06a55e0f22432e9a (Issue #21091) I'm not 100% able to understand why this is needed but I would think just deleting this would recreate the bug from #21091.
Intended Behavior
I'm not quite sure what the intended behavior would be as I'm not that deep into spring, but here are some logical seeming suggestions:
- the
ResponseBodyEmitter
is always completed on an exception so there is no need to complete it manually - the
ResponseBodyEmitter
stays open unless anIOException
is thrown and can be completed or used further (you can handle the exception thrown bysend()
however you want) ResponseBodyEmitterReturnValueHandler.send()
throws different Exception depending on if the connection is still usable or closed
Option 1 is probably the simplest to implement but you would close an intact connection which could have still been used.
For option 2 one has to check if this would add the bug from #21091 back in
Also it only makes sense if the connection stays open for all exceptions other then IOException
.
Option 3 requires searching for all exception that could occur, maybe even exception thrown by the servlet container and check if they are fatal for the connection.
(I have not tested any of these suggestions.)
Comment From: rstoyanchev
Thanks for the report and analysis.
As this Javadoc note says, when there is an IO error, the Servlet container is aware of the exception, and calls AsyncListener#onError
. In that case we need to wait for that notification rather than complete immediately. More details on that in #20173.
The change in #21091 is intended to ensure the application can't catch the exception and call complete since it's counter-intuitive. However, depending on where the exception occurs, the Servlet container may not be aware of it.
What we can do is scale back the fix from #20173 to allow completion in case of exceptions other than IOException
. Technically, an IOException
may come from higher levels too, e.g. JacksonProcessException
but we do wrap that with HttpMessageNotWritableException
so that will remain on the side of being possible to close.