On Tue, 23 Apr 2024 19:08:06 GMT, robert engels <d...@openjdk.org> wrote:
>> If the handler sends headers(code,0) and doesn't close the stream, the >> connection is dead. If what you are saying is that the old behavior was to >> send the headers at this point, the client would see it and terminate the >> connection - that feels like a lot of ifs. >> >> but as I mentioned, adding the flush after the exchange handler chain call >> is safe and would catch this scenario - even if the handler returns the data >> synchronously - as the buffered output stream at the top of the stream is >> fully synchronized. > > The PR has been updated to fix the tests. By the way, as an x-Googler I am familiar with Hyrum and have seen the link. I would hope that Googlers would have written api test cases - independent of the implementation - which maybe would have caught the problem here earlier but that might be wishful thinking. Since the api allows for async handling - validating that an arbitrary handler closes the stream doesn't seem possible other than by testing that the client receives the data it expects, and that the connection does not hang when keep alive with multiple requests is enabled. So I don't think runtime checks could have caught it either. If someone wrote a handler that relied on the fact that headers were sent "early", and/or that streams were flushed even though the api specification states the handler must close the stream - they would be coding to the sun implementation - and if they lacked access to the source code they would never be able to determine that is what was happening. The sun tests in this regard are incorrectly testing/expecting behavior that the published api specifically prohibits. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1576794566
