Fixes #4904 - WebsocketClient creates more connections than needed. #4911
Conversation
Fixed connection pool's `acquire()` methods to correctly take into account the number of queued requests. Also fixed a collateral bug in `BufferingResponseListener` - wrong calculation of the max content length. Restored `ConnectionPoolTest` that was disabled in #2540, cleaned it up, and let it run for hours without failures. Signed-off-by: Simone Bordet <[email protected]>
More fixes to the connection pool logic. Now the connection creation is conditional, triggered by explicit send() or failures. The connection creation is not triggered _after_ a send(), where we aggressively send more queued requests - or in release(), where we send queued request after a previous one was completed. Signed-off-by: Simone Bordet <[email protected]>
More fixes to the connection pool logic. Now the connection close/removal aggressively sends more requests triggering the connection creation. Signed-off-by: Simone Bordet <[email protected]>
Improved comments. Signed-off-by: Simone Bordet <[email protected]>
jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java
Show resolved
Hide resolved
jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java
Show resolved
Hide resolved
jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java
Show resolved
Hide resolved
jetty-client/src/main/java/org/eclipse/jetty/client/HttpConnection.java
Outdated
Show resolved
Hide resolved
| if (removed) | ||
| process(); | ||
| process(true); |
There was a problem hiding this comment.
should this check that the queue is not 0? Why acquire a connection if there are no waiting exchanges?
There was a problem hiding this comment.
The check for the queue is done few lines above. Do you want to do it again here?
There was a problem hiding this comment.
Opps didn't see that.
An else if (removed) would be clearer that there is a condition above.
There was a problem hiding this comment.
I think an else if would then require the code comment to be moved to a less ideal place so I'm inclined to leave as is.
There was a problem hiding this comment.
The comment would be better in the body of the else if, as it would not need to repeat the "if removed":
if (getHttpExchanges().isEmpty())
{
tryRemoveIdleDestination();
}
else if (removed)
{
// Process queued exchanges because waiting exchanges may now be able to progress.
process(true);
}Updates after review: added javadocs. Signed-off-by: Simone Bordet <[email protected]>
jetty-client/src/main/java/org/eclipse/jetty/client/AbstractConnectionPool.java
Outdated
Show resolved
Hide resolved
| if (removed) | ||
| process(); | ||
| process(true); |
There was a problem hiding this comment.
Opps didn't see that.
An else if (removed) would be clearer that there is a condition above.
Updates after review. Signed-off-by: Simone Bordet <[email protected]>
| @@ -64,7 +64,11 @@ public Connection acquire() | |||
| Connection connection = activate(); | |||
| if (connection == null) | |||
| { | |||
| int maxPending = 1 + destination.getQueuedRequestCount() / getMaxMultiplex(); | |||
There was a problem hiding this comment.
How about this one-liner instead?
int maxPending = (destination.getQueuedRequestCount() + getMaxMultiplex() - 1) / getMaxMultiplex();
There was a problem hiding this comment.
That is obfuscated... Does it do ceiling(a/b)?
There was a problem hiding this comment.
Yes, it's the integer version of the following:
Math.ceil((double) destination.getQueuedRequestCount() / getMaxMultiplex())
| * @param create whether to schedule the opening of a connection if no idle connections are available | ||
| * @return an idle connection or {@code null} if no idle connections are available | ||
| */ | ||
| protected Connection acquire(boolean create) |
There was a problem hiding this comment.
The documented contract is not respected by the RoundRobinConnectionPool subclass. That should probably be documented too.
| return connectionPool.remove(connection); | ||
| } | ||
|
|
||
| public void close(Connection connection) |
There was a problem hiding this comment.
Since close(Connection) and remove(Connection) are now identical, shouldn't one of them be removed?
There was a problem hiding this comment.
I deprecated close(Connection) to be removed in 10.
| @@ -69,6 +69,14 @@ public void setMaxMultiplex(int maxMultiplex) | |||
| } | |||
| } | |||
|
|
|||
| @Override | |||
| protected Connection acquire(boolean create) | |||
There was a problem hiding this comment.
As mentioned earlier, this implementation breaks the javadoc's contract.
Updates after review. Signed-off-by: Simone Bordet <[email protected]>
Updates after review. Signed-off-by: Simone Bordet <[email protected]>
Updates after review. Signed-off-by: Simone Bordet <[email protected]>
sbordet
deleted the
jetty-9.4.x-4904-httpclient_creates_too_many_connections
branch
Fixed connection pool's
acquire()methods to correctly take into account the number of queued requests.Also fixed a collateral bug in
BufferingResponseListener- wrong calculation of the max content length.Restored
ConnectionPoolTestthat was disabled in #2540, cleaned it up, and let it run for hours without failures.Signed-off-by: Simone Bordet [email protected]