Remove unnecessary flush #161
Conversation
|
Huh? Why? This seems incorrect? BTW: Why are you merging stuff without a review? |
| try (var os = exchange.getResponseBody()) { | ||
| exchange.sendResponseHeaders(statusCode(), bytes.length == 0 ? -1 : bytes.length); | ||
| os.write(bytes); | ||
| os.flush(); |
There was a problem hiding this comment.
Why remove this one? Did you hit an issue with this or are you just seeing it as unnecessary?
There was a problem hiding this comment.
Cause of this: robaho/httpserver#13 (comment)
also the tests passed so I honestly don't know why I added it in the first place
There was a problem hiding this comment.
flush() should only be used when you have a long lived push type/events endpoint - otherwise is breaks pipelining optimizations. Although the current impl doesn’t use it, a close without flush can also make http2 hafar more efficient especially for short responses as the headers, data frame and end stream could be sent in a single tcp packet.
Ah, interesting - cool.
why I added it in the first place
You might not have written it. Some of this code has come from V2 Jex and some of that was a translation from Javalin ... and Javalin might have had those flush() calls there. So the thought I have is, did Javalin need to have those flush() calls / where they there for a reason [given that they are dealing with Jetty's OutputStream and we are dealing with the JDK Http Servers OutputStream only for this case].
Right now I've been playing a little fast and loose since we've not yet deployed 3.0 (a total rewrite), shall I cease? |
Maybe I'm old and crusty but "speed" imo really comes from moving forward with very few mistakes. That is, what is "Really Slow" is merging in changes where we don't detect as an issue/mistake ... and then only later on [potentially much later on] understanding the consequences - the "Oh Shit" moment. In that sense 2 sets of eyes is always better that 1 because it increases the chance of detecting the "unforeseen side effect" of a given change. In this case, we have subtly changed the semantics of |
No description provided.