On Tue, 23 Apr 2024 19:31:03 GMT, robert engels <d...@openjdk.org> wrote:
>> 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. > > 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. > 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. Yes, the connection is dead, but what I'm saying is sometimes that doesn't matter, because the client (if it is HttpURLConnection) is going to close it anyway. I agree this is not a common scenario we need to worry about, and a simple release note flag should be enough to cover it. So, I don't think we are disagreeing on anything substantive here. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1577503119
