On Tue, 23 Apr 2024 15:24:52 GMT, Michael McMahon <micha...@openjdk.org> wrote:
>> I’ll update the PR to fix the tests. I understand the need for backwards >> compatibility- it’s one of the prime reasons Java is awesome - but in this >> situation the tests are validating that 1+1=3 and I don’t think anyone wants >> that. >> >> You’re assuming that changing the validation of the test cases means the >> potential for breaking code - as I’ve stated before I don’t think that’s >> possible in this case - because the test cases were never verifying code you >> would see in a production system. > > If the client was based on HttpURLConnection in Java and if the code was > doing HTTP authentication then it's possible it could happen because > HTTPURLConnection closes the connection in between each phase of the > authentication (in some cases). It is unfortunate as well that the server > side HttpExchange.sendResponseHeaders method's second parameter is quite > counter-intuitive in its meaning. It is a common mistake to provide a > parameter value of zero (which means use chunked-encoding) when what is > intended is probably -1, (meaning no response content). > > In any case, we all still accept that the change is worthwhile, and we just > need to flag it in the release notes. We also obviously have to fix the tests > so they pass. You just need to make sure the exchanges get closed in all > handlers defined in those two tests. 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18667#discussion_r1576537356
