RFR: 8349662: SSLTube SSLSubscriptionWrapper has potential races when switching subscriptions [v2]

[Jaikiran Pai]

On Tue, 11 Feb 2025 10:00:26 GMT, Daniel Fuchs <dfuchs at 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.
>
> Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Review feedback

test/jdk/java/net/httpclient/DigestEchoClient.java line 411:

> 409:                 BodyPublisher reqBody = BodyPublishers.ofString(body);
> 410:                 URI baseReq = URI.create(uri + "?iteration=" + i + ",async=" + async
> 411:                         + ",addHeaders=" + addHeaders + ",preemptive=" + preemptive

Should this (and the other places where we are updating the request URI) use `&` instead of `,`, for request query parameter delimiting or is this just changed for better logging (on the server side)?

test/jdk/java/net/httpclient/DigestEchoClient.java line 531:

> 529:             if (error != null) {
> 530:                 if (failed != null) error.addSuppressed(failed);
> 531:                 throw error;

Do you think an additional `error != failed` check would be needed here, before calling `addSuppressed()` or is that not a concern here?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23515#discussion_r1950547920
PR Review Comment: https://git.openjdk.org/jdk/pull/23515#discussion_r1950550813