On Tue, 11 Feb 2025 10:00:26 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.
>
> 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
