On Tue, 12 Nov 2024 19:43:45 GMT, Daniel Fuchs <dfu...@openjdk.org> wrote:
>> [JDK-8342075](https://bugs.openjdk.org/browse/JDK-8342075) has introduced >> more flow controls checks, but also introduced a race condition where >> DataFrames for closed streams may fail to be discounted from the connection >> window. >> >> The consequence is that WINDOW_UPDATE frames for the connection window may >> not be sent when they should, preventing the server from making progress and >> stalling the connection. >> >> This can be shown by modifying the StreamFlowControlTest to send less but >> bigger frames (e.g. chunks of 1600 bytes instead of chunks of 12 bytes). >> With such a modification the test can be seen failing intermittently, when >> sameClient=true. >> >> The race happens when frames that have been added to Stream::inputQ fail to >> be drained after the stream is closed (or continue to be added to the inputQ >> after the stream is closed). >> >> The fix ensures that Stream::drainInputQueue() is called when the stream is >> closed, and that no further data farme will be added to the inputQ after the >> stream is marked closed. >> >> The modified StreamFlowControlTest could be observed failing relatively >> frequently on linux-aarch64 without the fix. >> With the fix the test no longer fails. > > Daniel Fuchs has updated the pull request incrementally with one additional > commit since the last revision: > > Update test/jdk/java/net/httpclient/http2/StreamFlowControlTest.java > > Co-authored-by: Andrey Turbanov <turban...@gmail.com> test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/common/HttpServerAdapters.java line 434: > 432: * The response is always returned with fixed length. > 433: */ > 434: public static class HttpHeadHandler implements HttpTestHandler { The class name is slightly misleading since this also handles `GET`. Perhaps `HeadOrGetHandler` would be appropriate? test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/common/HttpServerAdapters.java line 467: > 465: } > 466: } > 467: t.getResponseBody().close(); It's a `HttpTestExchange`, so we don't have clear defined semantics for what happens when `HttpTestExchange.getResponseBody()` is invoked after the response body is already `close()`d previously. I think it might be better to move this `t.getResponseBody().close()` into the individual `case` blocks to avoid calling `t.getResponseBody()` after the `case GET` already closes the response body in its try-with-resources. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21991#discussion_r1839947396 PR Review Comment: https://git.openjdk.org/jdk/pull/21991#discussion_r1839945465
