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

AbstractJackson2HttpMessageConverter writes partial data when exception occurs during write #26246

Closed
jdelong747 opened this issue Dec 8, 2020 · 2 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: regression A bug that is also a regression
Milestone

Comments

@jdelong747
Copy link

Affects: 5.2.10.RELEASE and later


The AbstractJackson2HttpMessageConverter.writeInteral method was modified to use a try with resources block to optimize Jackson resource management gh-25910. This now calls close on the JsonGenerator in the event of an error such as a JsonProcessingException, which flushes what was written before the error occurred to the response body. When attempting to return a customized error response from a ControllerAdvice, the response body will contain the data written before the exception was thrown concatenated with the data written by the ControllerAdvice. This leads to an invalid response being returned to the client.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 8, 2020
@bclozel bclozel self-assigned this Dec 8, 2020
@jhoeller jhoeller added in: web Issues in web modules (web, webmvc, webflux, websocket) type: regression A bug that is also a regression and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Dec 8, 2020
@jhoeller jhoeller added this to the 5.2.12 milestone Dec 8, 2020
bclozel added a commit that referenced this issue Dec 8, 2020
Prior to this commit, a change introduced in gh-25910 would close the
`JsonGenerator` after it's been used for JSON serialization. This would
not only close it and recycle resources, but also flush the underlyning
buffer to the output.
In a case where the JSON serialization process would throw an exception,
the buffer would be still flushed to the response output. Before the
change introduced in gh-25910, the response body could be still empty at
that point and error handling could write an error body instead.

This commits only closes the `JsonGenerator` when serialization has been
successful.

Note that we're changing this in the spirit of backwards compatibility
in the 5.2.x line, but change this won't be merged forward on the 5.3.x
line, for several reasons:

* this behavior is not consistent. If the JSON output exceeds a
certain size, or if Jackson has been configured to flush after each
write, the response output might still contain an incomplete JSON
payload (just like before this change)

* this behavior is not consistent with the WebFlux and Messaging codecs,
which are flushing or closing the generator

* not closing the generator for error cases prevents resources from
being recycled as expected by Jackson

Fixes gh-26246
@spring-projects-issues
Copy link
Collaborator

Fixed via a8091b9

@bclozel
Copy link
Member

bclozel commented Dec 8, 2020

Thanks for this report @jdelong747 !

We've reverted this behavior change with a8091b9.

There are still a few things to consider here. The fact that the JSON payload might not be written to the response output in case of serialization errors is not a feature: this is really about the fact that the underlying buffer is not full yet or that Jackson didn't decide to flush it at that point. This is really accidental.
The best way to realize that is that you could configure Jackson to flush after each write, or just writing a large body could flush to the output before the error happens. In your case, the response body might be small enough that you're relying on this behavior.

In general, you should not rely on an @ExceptionHandler to handle an exception that happens while writing to the response. At that point, there's no guarantee that the HTTP response has not been committed and bits sent over the network.

Note that we won't be merging forward this commit, as:

  • this behavior is not an actual feature and is not 100% controlled
  • this behavior is not consistent with the WebFlux and Messaging codecs, which are flushing or closing the generator
  • not closing the generator for error cases prevents resources from being recycled as expected by Jackson

I'm afraid I don't see a way to reproduce this former behavior on your side in Framework 5.3, besides getting full control and serializing to some ByteArrayOutputStream and flushing that to the response later. This comes with performance and memory downsides, as you would expect.

We've added a note in the migration wiki in the MVC section of the 5.3 migration guide.

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: regression A bug that is also a regression
Projects
None yet
Development

No branches or pull requests

4 participants