RFR: 6968351: httpserver clashes with delayed TCP ACKs for low Content-Length [v14]

[Daniel Fuchs]

On Mon, 22 Apr 2024 15:23:01 GMT, robert engels <duke at openjdk.org> wrote:

>> Just to be clear - I agree with Daniel that the line 851 should be removed. I was just speculating about the possible backward compatibility issues of *not* flushing the stream after writing the headers. We saw some test failures, and we all agreed that these test started to fail because the handler was badly implemented. I'm just speculating here about the possible existence of such handlers in the wild. Passing 0 when intending to pass -1 to `sendResponseHeaders` is a common mistake that can go unnoticed if the body output stream or exchange are properly closed. If the exchange or body output stream is not properly close, it could go unnoticed as long as the client doesn't attempt to read the body and doesn't reuse the connection.
>
> I already removed that line - it is not necessary (but I also think it is fine - there is no race/corruption possible, and it would restore the previous behavior of always sending the headers).
> 
> To your point though, I think I must not be stating this very clearly, I'll try again.
> 
> If the code was making this "common mistake" - the code would never have worked before - the connection would hung for any future requests. The handler MUST close the stream/exchange if it states it will be returning content (length >=0) or that connection is dead.
> 
> Returning a 500 error does not force a close of the connection - the receiving client would expect to be able to send another request - at which point the server would never see it. The only time this would work is if the handler added the 'Connection: close' header and the client dropped the connection - that would allow the server to detect the dead connection and clean-up.
> 
> If it used (500,0) that is a chunked response, so if the handler never closes it, the data will not be sent - the chunked handler already buffers - and the client will hang waiting for data never to arrive.
> 
> So if there is code in the wild using sendResponse(500,0) and not closing the connection, then their server is already badly broken. It simply will not work.
> 
> You can change the TcpNoDelayNotRequired test to do this, and run it under JDK 11, and the test will fail.

I understand what you're saying - but you would be surprised of what you could encounter in the wild. In any case, it's not an issue if no backport is intended.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1574976067