On Fri, 7 Feb 2025 13:48:29 GMT, Daniel Fuchs <dfu...@openjdk.org> wrote:
> Hi,
>
> Please find here a change that fixes a potential race condition in
> SSLTube.SSLSubscriptionWrapper.
> Typically the race may get triggered if the demand increased by request() is
> not exhausted by the time
> the subscription is switched by setSubscription.
>
> Some synchronization is required to present a consistent view of the
> subscripton state, so that pending demand can be consistently transferred to
> the new the subscription.
>
> This mostly affects HTTP/1.1 over TLS since each new exchange will cause the
> subscription to be switched to the new exchange. The race condition is
> elusive and hard to reproduce. when it occurs, it mostly causes tests to fail
> in jtreg timeout as the demand from upstream may not be transferred properly.
>
> Some additional logging has been added to the DigestEchoClient.java test
> class (which is used by DigestEchoClientSSL) to help diagnosability of
> intermittent failures in these tests.
src/java.net.http/share/classes/jdk/internal/net/http/common/SSLTube.java line
644:
> 642: @Override
> 643: public void request(long n) {
> 644: final long demand = n;
Hello Daniel, is this `demand` variable necessary? From what I see, we could
just keep using `n` wherever we use `demand` in this method. I think that would
also avoid the confusion between `demanded` and this `demand` variables that
are used in this method.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23515#discussion_r1950537641
