On Wed, 24 May 2023 11:20:45 GMT, Jaikiran Pai <[email protected]> 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 <[email protected]> >> - Update >> src/java.net.http/share/classes/jdk/internal/net/http/ConnectionPool.java >> >> Co-authored-by: Andrey Turbanov <[email protected]> >> - Update test/jdk/java/net/httpclient/AuthFilterCacheTest.java >> >> Co-authored-by: Andrey Turbanov <[email protected]> >> - ... 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
