On Wed, 25 Oct 2023 09:52:03 GMT, Daniel Fuchs <[email protected]> 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
