RFR: 8308310: HttpClient: Avoid logging or locking from within synchronized blocks [v8]

Daniel Fuchs dfuchs at openjdk.org
Wed May 24 11:34:02 UTC 2023


On Wed, 24 May 2023 11:20:45 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Daniel Fuchs has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 11 commits:
>> 
>>  - Merge branch 'master' into HttpClient-Logging-8308310
>>  - Merge branch 'master' into HttpClient-Logging-8308310
>>  - Fix whitespace
>>  - make stateLock final
>>  - Add debug traces to ExpectContinueTest.java
>>  - failedRef should be final
>>  - Align parameters
>>  - Update src/java.net.http/share/classes/jdk/internal/net/http/Http1Response.java
>>    
>>    Co-authored-by: Andrey Turbanov <turbanoff at gmail.com>
>>  - Update src/java.net.http/share/classes/jdk/internal/net/http/ConnectionPool.java
>>    
>>    Co-authored-by: Andrey Turbanov <turbanoff at gmail.com>
>>  - Update test/jdk/java/net/httpclient/AuthFilterCacheTest.java
>>    
>>    Co-authored-by: Andrey Turbanov <turbanoff at gmail.com>
>>  - ... and 1 more: https://git.openjdk.org/jdk/compare/c0c4d771...c5d2f1f2
>
> src/java.net.http/share/classes/jdk/internal/net/http/MultiExchange.java line 218:
> 
>> 216:         synchronized (this) {
>> 217:             previousExchange = this.exchange;
>> 218:             this.exchange = exchange;
> 
> There's a behavioural change here as compared to before this change. Previously, we used to first call `released()` on the old exchange and `cancel()` the new exchange, before setting the `this.exchange` to the new exchange. Now, with this change, we first switch the `this.exchange` to the new exchange and then call `released()` and `cancel()`. Do you think that could have any (odd) issues?

Good observation. When we reach here the previous exchange is practically terminated. Looking at the implementation of released(), and at the place where `setExchange` is called, I don't think it matters.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14038#discussion_r1203945286


More information about the net-dev mailing list