Skip to content

Conversation

@rbygrave
Copy link
Contributor

  • Add Context isCommitted() and reset()
  • Modify ExceptionManager to use these to reset response buffer before error handler writes error content
  • Note that there is a related fix in avaje-jsonb 1.1-RC1

- Add Context isCommitted() and reset()
- Modify ExceptionManager to use these to reset response buffer before error handler writes error content
- Note that there is a related fix in avaje-jsonb 1.1-RC1
@rbygrave rbygrave added this to the 2.4-RC1 milestone Dec 14, 2022
@rbygrave rbygrave self-assigned this Dec 14, 2022
@rbygrave rbygrave merged commit 24b891f into master Dec 14, 2022
@rbygrave rbygrave added the bug Something isn't working label Dec 14, 2022
@rbygrave rbygrave deleted the feature/error-handling-reset branch December 14, 2022 10:01
@tipsy
Copy link
Collaborator

tipsy commented Dec 14, 2022

Why is this necessary? Have the codebases somewhat diverged now?

@rbygrave
Copy link
Contributor Author

rbygrave commented Dec 14, 2022

The way that jex and javalin write json output is different yes.

With javalin the entire json is written to a string, and then that is written to the servlet outputstream/writer.
https://github.com/javalin/javalin/blob/master/javalin/src/main/java/io/javalin/http/Context.kt#L409

With jex (which supports avaje-jsonb and jackson) the json is written directly to the servlet outputStream. Jetty (and most servlet containers) have a buffer and don't write the response headers and body immediately and they want to compute content-length etc) so has a default 32kb buffer.
avaje-jsonb - https://github.com/avaje/avaje-jex/blob/master/avaje-jex/src/main/java/io/avaje/jex/core/JsonbJsonService.java#L41
jackson - https://github.com/avaje/avaje-jex/blob/master/avaje-jex/src/main/java/io/avaje/jex/core/JacksonJsonService.java#L43-L53

This fix for jex resets that buffer when there is an error DURING the writing of json response (so expected to be rare as this is plumbing executed at the very end of normal application response handling). The buffer is reset before the appropriate error handler is called, so that the error handler writes error response.
https://github.com/avaje/avaje-jex/blob/master/avaje-jex/src/main/java/io/avaje/jex/core/ExceptionManager.java#L25-L33

WIth this approach, IF the response being written exceeds the buffer size (Jetty default of 32kb) then jetty will have sent that ... and an error handler can't write an error response body (or at least any error response body appends to some already sent body).

So javalin currently doesn't have this issue because it currently writes all the json to a string before writing that to the servlet output. So if there is any error while writing that string, nothing has yet been written to servlet output.

Jex is different here because for performance it can be faster to write the json directly to outputStream in bytes.

I should link the issue that triggered this issue.

Edit: Added links to code

@rbygrave
Copy link
Contributor Author

FYI @tipsy link to originally logged issue - avaje/avaje-http#112

@tipsy
Copy link
Collaborator

tipsy commented Dec 15, 2022

With javalin the entire json is written to a string, and then that is written to the servlet outputstream/writer.

Javalin does support writing without buffering in a String, with Context#jsonStream (we extended the JsonMapper in 4x to support this).

With jex (which supports avaje-jsonb and jackson) the json is written directly to the servlet outputStream. Jetty (and most servlet containers) have a buffer and don't write the response headers and body immediately and they want to compute content-length etc) so has a default 32kb buffer.

Ah, we don't write directly to the outputStream from the handler, we set a reference to an InputStream which we write later, in the "appropriate" place in Javalin's request lifecycle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix ExceptionManager to reset buffers before error handling

4 participants