Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
hexzeug opened this issue Jun 18, 2023 · 1 comment
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Milestone

Comments

@hexzeug
Copy link

hexzeug commented Jun 18, 2023

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();
    }
}
  1. Open a connection to the /open endpoint. For example using curl localhost:8080/open:
  2. Test if sending works by triggering the /send endpoint in another window.
  3. Test if closing works by triggering the /close endpoint.
  4. Reopen the closed connection (/open).
  5. 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.
  6. Test sending (/send). It still works.
  7. Test closing (/close). It doesn't work.
  8. 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.)

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jun 18, 2023
@poutsma poutsma added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Jun 19, 2023
@rstoyanchev
Copy link
Contributor

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.

@rstoyanchev rstoyanchev added type: enhancement A general enhancement type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on type: enhancement A general enhancement labels Oct 2, 2023
@rstoyanchev rstoyanchev added this to the 6.1.0-RC1 milestone Oct 2, 2023
@rstoyanchev rstoyanchev self-assigned this Oct 2, 2023
@rstoyanchev rstoyanchev changed the title ResponseBodyEmitter ignores calls to complete but remains open after IllegalStateException while sending ResponseBodyEmitter ignores calls to complete but remains open after JSON write error while sending Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants