ResponseBodyEmitter ignores calls to complete but remains open after JSON write error while sending

[hexzeug]

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

1. Get spring boot with the spring web dependency from https://start.spring.io/.
2. 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();
    }
}

3. Open a connection to the /open endpoint. For example using curl localhost:8080/open:
4. Test if sending works by triggering the /send endpoint in another window.
5. Test if closing works by triggering the /close endpoint.
6. Reopen the closed connection (/open).
7. Call the /break endpoint to break the ResponseBodyEmitter instance.
   This is done by trying to send an instance of Object which cannot be converted by the default converters.
8. Test sending (/send). It still works.
9. Test closing (/close). It doesn't work.
10. You can also replace complete() with completeWithError(<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 568c934 (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:

1. the ResponseBodyEmitter is always completed on an exception so there is no need to complete it manually
2. the ResponseBodyEmitter stays open unless an IOException is thrown and can be completed or used further (you can handle the exception thrown by send() however you want)
3. 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.)

[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.