RFR: 6968351: httpserver clashes with delayed TCP ACKs for low Content-Length [v14]
robert engels
duke at openjdk.org
Mon Apr 22 14:42:30 UTC 2024
On Mon, 22 Apr 2024 14:10:01 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:
>>> We don't want to allow it; we want to fix the broken tests, and add a release note so that the users know how to recognize that their code is broken and know how to fix it.
>>>
>>> This should be fine in a new JDK version, where the users are expected to invest some effort in upgrading. Do you plan to backport this change to older releases? If you do, it would make sense to add a system property to revert to the old (slow) behavior. This would allow the users to use the updated version without recompiling their code.
>>
>> Hi Daniel. I think there is still some disconnect - I apologize for not being clear. I don't think there will be anything to fix even if back-ported. The only thing to do would be to inform the users they no longer needs to add the 'no delay' option to achieve better network performance. Any existing client code must be closing the stream or exchange - or the code would not work - the clients would hang.
>>
>> The only problem with the existing JDk impl is that there is very slow performance when doing so. This PR fixes the performance issue - it doesn't change the semantics of the communication.
>>
>> The only reason that a few test cases are breaking with this PR is that they are written incorrectly - but it was never discovered because it was launching the test in a new jvm and only performing a single call.
>>
>> At least that is the way I see it - but there's been a lot of discussion so maybe I've missed something.
>
> I can see that a bad handler could call `sendResponse(500, 0);` instead of `sendResponse(500, -1)`, and forget to close the body output stream or the exchange. A client might just not read the body if it receives 500. Before the fix, the server will flush the headers and the client will receive them and everything would work. After the fix, the headers will not be flushed (as a chunked body is expected), and everything will hang (or eventually timeout).
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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1574876220
More information about the net-dev
mailing list