arturobernalg commented on PR #474: URL: https://github.com/apache/httpcomponents-client/pull/474#issuecomment-1672110593
> # Problem > > 1. Function in question calls [`conn.isStale()`](https://github.com/apache/httpcomponents-client/blob/223669659c295a01afe9047e0910768781c8f157/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java#L339), which is implemented in httpcore5 (`BHttpConnectionBase.isStale`) by running potentially blocking IO operation inside `fillInputBuffer`. (UPD: there are other potentially blocking operations in this function, such as logging and waiting for future) > > 2. Also, this function is marked as `synchronized`, hence it holds monitor lock on `this` while it executes. > > > Unfortunately, combination of this two facts makes this function loom-unfriendly: when virtual thread acquires connection from pool, it is unable to park gracefully (since it holds a monitor lock), so it blocks together with its carrier thread. Such a behavior temporary decreases parallelism, as carrier is simply blocked instead of executing other virtual threads. > # Solution > > This PR simply replaces `synchronized` with a j.u.c ReentrantLock, and problem should be gone. > # Alternatives > > * Do nothing and just wait until Loom limitation is lifted and virtual threads holding monitor locks may park. However, it is unclear to me when this improvement will arrive. > > * I could find only one non-test usage of `LeaseRequest.get()` method - in `InternalExecRuntime.acquireEndpoint()` method, which calls it only once. Therefore it looks possible to specify that get() is never called concurrently and drop `sychronized` keyword without replacement. On the other hand it may be breaking API change. Also, this is first time I look at httpcomponents source code, so my analysis may be simply wrong. @MikailBag Could you please provide some insight into the handling of other synchronization blocks within the code? Are there any plans to review or modify them to ensure compatibility with Loom? The change should be consistent across the entire code base or not at all. I would leave 4.x be and make sure 5.x works correctly with the newest JREs. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
