On Mon, 22 Apr 2024 15:23:01 GMT, robert engels <d...@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
