RFR: 8309118: HttpClient: Add more tests for 100 ExpectContinue with HTTP/2 [v7]
Conor Cleary
ccleary at openjdk.org
Thu Oct 26 14:01:37 UTC 2023
On Wed, 25 Oct 2023 09:52:03 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:
>> src/java.net.http/share/classes/jdk/internal/net/http/Stream.java line 215:
>>
>>> 213: Log.logTrace("responseSubscriber.onComplete");
>>> 214: if (debug.on()) debug.log("incoming: onComplete");
>>> 215: if (inputQ.isEmpty()) sched.stop();
>>
>> I think you should remove all references to sched.stop() from this method; while adding `if (inputQ.isEmpty())` reduces the risk of races, there's still a small window where an incoming reset frame can be lost.
>
> The only thing we could do with the reset after this point in the schedule() method, if the reset had not yet been inserted into the inputQueue, is to complete the `requestBodyCF` and this would have already been taken care of at line 595 in incoming_reset. So I don't believe the scheduler would need to run again after this line (or the other like it)
I think maybe its better to let the scheduler complete without these checks. I think the effect of `stop()` is that it makes all subsequent calls to `runOrSchedule()` no-ops. I think it might be best to remove the call to stop() and allow scheduler to complete the requestBodyCf in a similar manner to how its done in `incoming_reset()`.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15664#discussion_r1373230487
More information about the net-dev
mailing list