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

[robert engels]

On Mon, 22 Apr 2024 14:51:30 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> Prior to this change, If the handler does not close the body output stream or exchange, then yes, the client will see those headers but if it tries to send another request - which will reuse the same connection by default in HttpClient - the server will never see it - hanging the client.
>> 
>> The server must see a 'write finished' event in order to properly prepare the connection for the next request.
>> 
>> As I pointed out, you can change the TcpNoDelayNotRequired test and comment out the close, and run it on JDK 11 for instance - and the test will fail - it will hang leading to a timeout - even if tcp nodelay is turned on.
>> 
>> Which leads to my contention there can't be existing code like this in the wild.
>
> 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.

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

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