On Mon, 9 Oct 2023 13:35:24 GMT, Daniel Fuchs <[email protected]> wrote:
>> Conor Cleary has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains ten additional
>> commits since the last revision:
>>
>> - 8309118: Fixed error in handleReset ReentrantLock
>> - 8309118: Updates to Stream cancelImpl
>> - 8309118: Refactored test, updated incoming_reset
>> - Merge branch 'master' into JDK-8309118
>> - 8309118: Cleanup identifiers and whitespace
>> - 8309118: Removed unused try-with-resources
>> - 8309118: Improve comments and annotations
>> - 8309118: Remove local test timeout value
>> - 8309118: Add more tests for 100 ExpectContinue with HTTP/2
>
> src/java.net.http/share/classes/jdk/internal/net/http/Stream.java line 584:
>
>> 582: void incoming_reset(ResetFrame frame) {
>> 583: Log.logTrace("Received RST_STREAM on stream {0}", streamid);
>> 584: Flow.Subscriber<?> subscriber = responseSubscriber == null ?
>> pendingResponseSubscriber : responseSubscriber;
>
> Suggestion:
>
> Flow.Subscriber<?> subscriber = responseSubscriber;
> if (subscriber == null) subscriber = pendingResponseSubscriber;
>
> (avoids reading volatile twice)
Ah yes, I see I think. In either case of the conditional expression either
`responseSubscriber` and `pendingResponseSubscriber` are read or
`responseSubscriber` is read and `responseSubscriber` is read. I was confused
for a bit because I thought only one operand is evaluated. But actually its the
case that `responseSubscriber` is read twice when `responseSubscriber == null`
evaluates to false.
Good suggestion!
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15664#discussion_r1364079499
