You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by "MikailBag (via GitHub)" <gi...@apache.org> on 2023/08/08 20:36:55 UTC
[GitHub] [httpcomponents-client] MikailBag opened a new pull request, #474: Avoid loom-unfriendly monitors in PoolingHttpClientConnectionManager
MikailBag opened a new pull request, #474:
URL: https://github.com/apache/httpcomponents-client/pull/474
# 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`.
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.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on pull request #474: Avoid loom-unfriendly monitors in PoolingHttpClientConnectionManager
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #474:
URL: https://github.com/apache/httpcomponents-client/pull/474#issuecomment-1681289239
@MikailBag There is nothing wrong with the proposed change-set. It looks perfectly reasonable but we do not want to be fixing synchronized methods piecemeal. They all should be changed consistently across the entire code base in a single change-set similar to the one committed to HttpCore 5.3.x: https://github.com/apache/httpcomponents-core/commit/4d6d9f55631226756736807d4a19d793c75d9500
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] MikailBag closed pull request #474: Avoid loom-unfriendly monitors in PoolingHttpClientConnectionManager
Posted by "MikailBag (via GitHub)" <gi...@apache.org>.
MikailBag closed pull request #474: Avoid loom-unfriendly monitors in PoolingHttpClientConnectionManager
URL: https://github.com/apache/httpcomponents-client/pull/474
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on pull request #474: Avoid loom-unfriendly monitors in PoolingHttpClientConnectionManager
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #474:
URL: https://github.com/apache/httpcomponents-client/pull/474#issuecomment-1672655744
@MikailBag I agree with @arturobernalg It makes no sense to apply such changes piecemeal. It should be done across the entire code base like with apache/httpcomponents-core#412
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] MikailBag commented on pull request #474: Avoid loom-unfriendly monitors in PoolingHttpClientConnectionManager
Posted by "MikailBag (via GitHub)" <gi...@apache.org>.
MikailBag commented on PR #474:
URL: https://github.com/apache/httpcomponents-client/pull/474#issuecomment-1680740702
> I propose we close this PR and focus on https://issues.apache.org/jira/projects/HTTPCLIENT/issues/HTTPCLIENT-2290
To be honest, I don't see contradictions between this PR and the ticket description. Do you mean that this synchronized usage should be resolved in some other way, i.e. completely removing any synchronization? Or that this PR is too small in scope?
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] arturobernalg commented on pull request #474: Avoid loom-unfriendly monitors in PoolingHttpClientConnectionManager
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] MikailBag commented on pull request #474: Avoid loom-unfriendly monitors in PoolingHttpClientConnectionManager
Posted by "MikailBag (via GitHub)" <gi...@apache.org>.
MikailBag commented on PR #474:
URL: https://github.com/apache/httpcomponents-client/pull/474#issuecomment-1673328398
> Could you please provide some insight into the handling of other synchronization blocks within the code?
This particular `synchronized` block pinning was actually caught in practice, so I changed it. I initially chose not to touch other blocks since I'm not sure if blocking inside them is possible or not.
Other than that, I have no objections to replacing all other blocks.
> I would leave 4.x be and make sure 5.x works correctly with the newest JREs.
If I don't mix things up, this PR is opened against 5.x code.
> My vote is to do nothing until virtual threads, which are only out of beta in Java 21, are fully debugged and accepted.
In JDK early access releases they should be already usable without any `--enable-preview` flags.
Anyway, I guess I understand your position. Maybe this pull request should be closed/postponed until JDK 21 is released.
Thank you in advance for providing fast feedback!
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on pull request #474: Avoid loom-unfriendly monitors in PoolingHttpClientConnectionManager
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #474:
URL: https://github.com/apache/httpcomponents-client/pull/474#issuecomment-1678725664
@MikailBag I propose we close this PR and focus on https://issues.apache.org/jira/projects/HTTPCLIENT/issues/HTTPCLIENT-2290
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] garydgregory commented on pull request #474: Avoid loom-unfriendly monitors in PoolingHttpClientConnectionManager
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #474:
URL: https://github.com/apache/httpcomponents-client/pull/474#issuecomment-1672890856
We should tread very carefully here IMO. My vote is to do nothing until virtual threads, which are only out of beta in Java 21, are fully debugged and accepted. I don't see it as a positive to increasebour code size and complexity for "friendliness".
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org