Multiple web client connections created instead of utilizing existing initial connection

[pantinor]

Questions

If we set the server to a larger number of max concurrent threads like 250 streams, and only send 100 requests on the webclient, which is using a max pool size of 8, it seems that 8 connections are created instead of using the single connection for all the streams.

Version

3.9.1

Context

See reproducer

Do you have a reproducer?

A reproducer is a simple project hosted on GitHub (or another forge supporting git clone operation) that has a build file that can be executed to reproduce the issue.

Reproducers are very helpful for contributors and will likely help them fixing your bug faster.

https://github.com/pantinor/vertx.issues/blob/main/issues/src/test/java/vertx/demo/StreamsVertxOnlyTest.java

Steps to reproduce

Run the test above, note the failure on the assertion for expecting 1 connection instead of 8.

Extra

• Anything that can be relevant such as OS version, JVM version

[vietj]

I think this is the expected result, the explanation is that when the pool is queries, then it can create up to 8 connections and will do it. It cannot really speculate on the number of max connection that the server will reply with.

I don't know if we can make this better.

[vietj]

that being said, if a single connection is opened and kept in the pool then it will try to use as much as possible this connection

[pantinor]

Hi Julien, I think we are expecting that if we configure the connection pool size to greater than 1, that they would be established dynamically when needed, not at the initial start of the web client traffic, and that it would use as much as possible the first connection if and until the streams max or client multiplex limit is reached. In the case of using the default of 1 max connection pool, then another connection cannot be created and failures would occur at max limits scenario.

But, if we have more than 1 connection created initially, and the traffic is used much as possible on the primary / initial connection in the pool, this can be fine too. I havent tested if that is the case yet, but was theorizing that the implementation simply looks for an "available" connection to send the request, so it spreads the requests over the first one found that is available non deterministically. I guess this is fine too.

[pantinor]

If we have a fix on this client connection pool issue, I guess it will be forward ported to 4.0.0 as well right ?

[vietj]

yes, but I don't think this can be fixed trivially.

[pantinor]

@vietj We should expect that when setting the idle timeout to a low value like 10-15 seconds on the webclient options, that all the connections should be closed after that idle timeout period has passed with no traffic. Do you agree? I have a test that seems to show the connections are not all getting closed in the conenction pool after like 60 seconds of waiting (i set idle timeout to 5 seconds). We are using a single web client instance for the test. Thoughts ? Thanks.

[jvican]

Just chiming in here, I think that solution makes sense. Instead of synchronizing the logic that opens connections or waiting for the results of in-flight connection handshakes, we should keep the current behavior of opening N connections, where N is the number of the max pool size, but then start closing connections until we're left with only one connection to the same server address. I guess the challenging bit is making sure we can swap the connection safely without affecting the request traffic.

[vietj]

I think that if you want a single connection, then you should have a pool of a size 1, otherwise there is nothing wrong with a pool opening 5 connections if you set max size to 5 when connections are aggressively demanded

[vietj]

@pantinor if there is a bug with idle timeout it should be fixed, is that what you observed ?

[vietj]

another concern I do have with the pool is that I would like the implementation to be kind of rewritten to be non blocking (currently it requires cooperation using a single thread event-loop) where all requests would not depend on an event loop but instead use non blocking to cooperate, hence why I would like to keep the pool as simple as possible.

[pantinor]

@vietj Question for you about using a single connection with max pool size of 1, if the server responds with a constraint on its max concurrent streams setting, will the client automatically create another connection when exceeding the streams size even tho its max pool size is 1? or will API processing receive a send / write exception indicating the failure to send?

Yes the idle timeout does not seem to be working, at least in the test I used with max pool size = 8 (using 4.0.0 release). Also, I looked at the eviction of connections (ie onEvict API of the listener) in the implementation and it is only invoked when sending or receiving the goAway indicator.

[vietj]

@pantinor no it will not

I think one thing we could try is to have a new pool setting that defines how many connection requests are concurrently achieved.

so if we set this value to 1, then only a single connection is performed at the same time. When the connection happens depending on the concurrency it will handle as much queued requests as possible.

the implementation of this seems reasonable complexity wise and would (I think) achieve this result.

[pantinor]

@vietj Does this below look correct for the use case for this dynamic connection setting?

• When max connection pool = 1 and (proposed setting) max concurrent connections = 8 connections shall scale up based on concurrency with the server (and clients multiplexing limit) to max of 8 connections. IO exception should be propagated to web client API if bytes cannot be sent over http connection(s) in the congestion scenario.
• Idle timeout mechanism shall be effective to close/evict idle connections.

When existing max connection pool setting is increased from 1, connections will still be statically created at the start of the sending of traffic on the client instance (unchanged from current behavior).

[vietj]

@pantinor this proposal would not increase the size of the connection pool, it only changes how many concurrent TCP connection can be established at the same time.

When it is set to 1, it will ensure for HTTP/2 that can multiplex connections that a single connection is established and it will allow this connection to be fully used before another physical connection is created when the initial connection is maxxed.

Idle timeout is a separate settings.

[pantinor]

@vietj The proposal looks good to me. I can be available to review the changeset when this is being added. Can you let us know a timeline when we think the team could add this setting? Thanks again.

[vietj]

currently the priority is Vert.x 4.0 release, so after this release

it has been scheduled to 3.9.5 the show the intent to be part of the 3.9 branch

[pantinor]

Can you add this to 4.0.0 release. We are using 4.0.0 as well.

On Sat, Nov 14, 2020, 3:41 AM Julien Viet notifications@github.com wrote:

currently the priority is Vert.x 4.0 release, so after this release

it has been scheduled to 3.9.5 the show the intent to be part of the 3.9 branch

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/eclipse-vertx/vert.x/issues/3590#issuecomment-727169021, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALKAHQXE77FDO5YLB6LHX3SPY7E5ANCNFSM4SBWVQEQ .

[vietj]

that would rather be 4.1 then

[vietj]

it will be for sure in 4 though, it is just that GH issues can only have a single version and currently we set 3.9.x version when we resolve an issue both for 3.9 and 4

[pantinor]

@vietj Hi, we are revisiting Vertx performance and checking back into this issue I opened 3 years ago now it seems. At that time above we were discussing a proposed setting of "new pool setting that defines how many connection requests are concurrently achieved".

I will check if the latest build has changes this regard and if this ticket can be closed out as it is stale. The same topic keeps coming back arising again and again in our group, so need to check.

[vietj]

that looks like an interesting proposal @pantinor which would lead to serialize HTTP connection creation in the pool and solve this problem