Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #5855 - HttpClient may not send queued requests. #5856

Merged

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented Jan 5, 2021

Changed the AbstractConnectionPool.acquire() logic to call tryCreate() even
when create=false.

This is necessary when e.g. a sender thread T2 with create=true steals a
connection whose creation was triggered by another sender thread T1.
In the old code, T2 did not trigger the creation of a connection, possibly
leaving a request queued.
In the new code, T2 would call tryCreate(queuedRequests), possibly triggering
the creation of a connection.

This change re-introduces the fact that when sending e.g. 20 requests
concurrently, 20+ connections may be created.

However, it is better to err on creating more than creating less and leaving
requests queued.

Signed-off-by: Simone Bordet simone.bordet@gmail.com

Changed the AbstractConnectionPool.acquire() logic to call tryCreate() even
when create=false.

This is necessary when e.g. a sender thread T2 with create=true steals a
connection whose creation was triggered by another sender thread T1.
In the old code, T2 did not trigger the creation of a connection, possibly
leaving a request queued.
In the new code, T2 would call tryCreate(queuedRequests), possibly triggering
the creation of a connection.

This change re-introduces the fact that when sending e.g. 20 requests
concurrently, 20+ connections may be created.

However, it is better to err on creating more than creating less and leaving
requests queued.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet requested review from gregw and lorban January 5, 2021 22:13
@sbordet
Copy link
Contributor Author

sbordet commented Jan 5, 2021

@gregw @lorban the check for isMaximizeConnections() is now redundant.

Before this PR, when !create && isMaximizeConnections(), the code was calling tryCreate(maxPending).

With this PR, the call to tryCreate(maxPending) is done when create==false, so there would be no need to test for isMaximizeConnections(). However, I think it's no harm as round robin is more aggressive anyway.

If the test for isMaximizeConnections() is removed from these changes, then it's not used anywhere else and may probably be removed also from RoundRobinConnectionPool.

@lorban
Copy link
Contributor

lorban commented Jan 6, 2021

Indeed, isMaximizeConnections() is not strictly necessary anymore since the else branch may now try to create a connection anyway.

But without it, RR would only create new connections if it cannot find an idle one, which may or may not not be the expected behavior.

Personally, I think the default behavior should have been to spread the load across the smallest possible number of connections but since history has decided otherwise, this parameter should stay the way it is to default to spreading the load across the largest possible number of connections.

Copy link
Contributor

@lorban lorban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just giving this a -1 for now, as I think there is a way to do this atomically.
I will give it a try... and when it inevitably fails then I'll re-review

Issue #5855 - HttpClient may not send queued requests 

Moved field pending from Pool to AbstractConnectionPool.
As a consequence, AbstractConnectionPool.tryCreate() now performs a demand/supply calculation to decide whether to create a new connection.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
Copy link
Contributor Author

sbordet commented Jan 7, 2021

@gregw work on #5858 showed that's still not possible to precisely create an exact number of connections, so the tradeoff remains that it's better to create a few more than let requests in the queue.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet merged commit 403d5ec into jetty-9.4.x Jan 7, 2021
@sbordet sbordet deleted the jetty-9.4.x-5855_httpclient_may_not_send_queued_request branch January 7, 2021 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants