RFR: 8343855: HTTP/2 ConnectionWindowUpdateSender may miss some unprocessed DataFrames from closed streams [v3]

[Jaikiran Pai]

On Tue, 12 Nov 2024 19:43:45 GMT, Daniel Fuchs <dfuchs at 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 <turbanoff at gmail.com>

test/jdk/java/net/httpclient/http2/StreamFlowControlTest.java line 355:

> 353:                 if (t instanceof FCHttp2TestExchange fct) {
> 354:                     fct.responseSent(query);
> 355:                 } else fail("Exchange is not %s but %s"

Nit, while we are here, I think it might be better to wrap the contents of the `else` block in a `{}` for consistency.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21991#discussion_r1839934207