On Mon, 22 Apr 2024 14:40:02 GMT, robert engels <d...@openjdk.org> wrote:
>> 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. 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1574895460
