[
https://issues.apache.org/jira/browse/HTTPCORE-750?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17728836#comment-17728836
]
Gaojie Liu commented on HTTPCORE-750:
-------------------------------------
Hi [~olegk]
Thanks for looking at the PR and this problem.
It will take some time to reproduce this issue since it is a race condition and
it is not easy to be triggered in a simple unit test and under high load, we
did see this conn storm behavior.
Regarding calling `future.get()` in class: `AbstractIOSessionPool`, I
understand it is not good in general, but the code in PR did the `isDone` check
before calling `future.get()`:
{code:java}
if (poolEntry.sessionFuture != null && poolEntry.sessionFuture.isDone()) {
// Check whether we should recreate a new conn or not
try {
poolEntry.session = poolEntry.sessionFuture.get();
while (true) {
final FutureCallback<IOSession> pendingCallback =
poolEntry.requestQueue.poll();
if (pendingCallback != null) {
pendingCallback.completed(poolEntry.session);
} else {
break;
}
}
} catch (final Exception e) {
poolEntry.sessionFuture = null;
} {code}
So in practice, the `future.get()` won't block, and if you still have a
concern, I can modify the code to use a timed get:
{code:java}
poolEntry.session = poolEntry.sessionFuture.get(1, TimeUnit.MILLISECONDS);
{code}
Actually, this is not my original approach.
Originally, I was thinking to check whether the `sessionFuture` is failed or
cancelled or not before creating a new conn, but `Future` doesn't provide an
API to check whether the Future is failed or not and it only has an API:
`isCancelled`, which is not good enough. `Future.get()` will cover both
scenarios:
[https://github.com/apache/httpcomponents-core/blob/master/httpcore5/src/main/java/org/apache/hc/core5/concurrent/BasicFuture.java#L70]
With this in mind, another way can be:
1. Use `BasicFuture` instead of `Future` for `poolEntry.sessionFuture`.
2. Add a new method: `isFailed` to `BasicFuture`.
3. `AbstractIOSessionPool` will use the following logic:
{code:java}
if (poolEntry.sessionFuture != null && (poolEntry.sessionFuture.isCancelled()
|| poolEntry.sessionFuture.isFailed())) {
// Check whether we should recreate a new conn or not
poolEntry.sessionFuture = null;
}{code}
Which approach do you prefer?
> H2ConnPool creates too many conns under high load at initialization time
> ------------------------------------------------------------------------
>
> Key: HTTPCORE-750
> URL: https://issues.apache.org/jira/browse/HTTPCORE-750
> Project: HttpComponents HttpCore
> Issue Type: Bug
> Components: HttpCore
> Affects Versions: 5.2.1
> Environment: Linux / Ubuntu / openjdk version "1.8.0_292"
> Reporter: Gaojie Liu
> Priority: Major
> Time Spent: 50m
> Remaining Estimate: 0h
>
> We are using httpcore5 5.2.1 and we observed that at application startup
> time, httpclient5 could create more than 1 conn per endpoint and in some
> cases, too many connections caused OOM issue since httpclient5 creates a pair
> of input/output buffer per conn.
> The regression is introduced by HTTPCORE-750, which made this change:
> [https://github.com/apache/httpcomponents-core/commit/14caf43eae407c544161c7f92329e8beb42a3534]
>
> {code:java}
> if (poolEntry.session != null) {
> callback.completed(poolEntry.session);
> } else {
> poolEntry.requestQueue.add(callback);
> if (poolEntry.sessionFuture != null &&
> poolEntry.sessionFuture.isDone()) {
> poolEntry.sessionFuture = null;
> } {code}
> When poolEntry.sessionFuture.isDone() is true, the connection is already
> ready, but the existing logic will abandon it and create a new one, and under
> high load, this logic could create a lot of conns per endpoint, which
> consumes a lot of memory.
>
> The proposed fix:
> {code:java}
> if (poolEntry.session != null) {
> callback.completed(poolEntry.session);
> } else {
> poolEntry.requestQueue.add(callback);
> if (poolEntry.sessionFuture != null &&
> poolEntry.sessionFuture.isDone()) {
> // Check whether we should recreate a new conn or not
> try {
> poolEntry.session = poolEntry.sessionFuture.get();
> while (true) {
> final FutureCallback<IOSession> pendingCallback =
> poolEntry.requestQueue.poll();
> if (pendingCallback != null) {
> pendingCallback.completed(poolEntry.session);
> } else {
> break;
> }
> }
> } catch (final Exception e) {
> poolEntry.sessionFuture = null;
> }
> } {code}
> I am planning to cut a PR for this.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@hc.apache.org
For additional commands, e-mail: dev-h...@hc.apache.org
