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

[robert engels]

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

>> 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.

If I write a test case "test multiple requests without closing" - and add that test case to earlier JDK versions, it will fail every time - with no setting that would allow it to work.

So trying to protect earlier bad code that couldn't possibly have worked doesn't seem right to me. I am fairly certain that the only existing code that will break are the incorrect tests cases that exist in the JDK. There can be no existing handlers in the wild that will break with these changes.

I feel like I am taking up too much of your time with these discussions.  I was only trying to fix the jdk version for others (using the knowledge I gained in the robaho impl).

A back-ported change could guard the changes - they are fairly trivial - but I'll let others undertake that.

The api [docs](https://docs.oracle.com/en/java/javase/11/docs/api/jdk.httpserver/com/sun/net/httpserver/HttpExchange.html) already state:

> When the response body has been written, the stream must be closed to terminate the exchange.

so I don't think back-porting as is an issue. I have never encountered an impl of an api that tries to work around improper use of the api.

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

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