Add connection-limiting pool

[trowski]

Not sure on the name, suggestions welcome.

Should we deprecate LimitedConnectionPool or rename it (with an alias of the old name) to SingleStreamConnectionPool or leave it as-is?

Closes #252.

[nicolas-grekas]

thanks for the clarification

[kelunik]

Why SingleStreamConnectionPool? It isn't a single stream, but the concurrency you specify in the semaphore.

[trowski]

@kelunik Yes, was thinking of a mutex not a semaphore. I renamed it to StreamLimitingConnectionPool instead. I can revert that commit if we just want to keep the original name.

[kelunik]

@nicolas-grekas Could you clarify which limit you actually intend to use? I guess you want something like 6 connections for HTTP/1 and one connection for HTTP/2?

[nicolas-grekas]

Yes correct. Using up to 6 connections on h2 would work too, but I don't know which algo should select opening a new connection vs a new stream. Dunno also how curl does it.

[kelunik]

But isn't the limit also there to limit concurrent requests, which is then basically no longer the case for HTTP/2 connections?

[nicolas-grekas]

Yes that's true. Yet browsers still open several connections to work around TCP throttling, which is the drawback of h2, the reason why h3 exists.

[trowski]

Pushed a commit that separates the HTTP/2 and HTTP/1.x connection limits in the pool. Default is 6 HTTP/1.x and 1 HTTP/2. Does this make sense for most use-cases?

[nicolas-grekas]

Thanks! 6 & 1 work for me but I'm wondering: let's say there is only one single maximum for both h1 and h2. Is there a way to race for "new connection is ready" vs "existing connection becomes available" and have this logic decide which stream is used for the next request? It would work for both h1 and h2 and might be the most performant, WDYT?

[kelunik]

Is there a way to race for "new connection is ready" vs "existing connection becomes available" and have this logic decide which stream is used for the next request?

Yes, that should be possible.

[nicolas-grekas]

I hope my last comment didn't block progress on this one :)

[trowski]

@nicolas-grekas Maybe a little. 😏

I have another branch, fallback-to-existing-connection that extends this PR, what do you think? Can you give it a try? If that approach works well, I'll merge it into this PR.

[nicolas-grekas]

I just checked out this PR and I confirm it fixes the perf issue, which means connections are now much better reused.
Since Symfony has a single setting for the number of maximum connections to open, I'm creating the pool using:
$pool = CountLimitingConnectionPool::byAuthority($maxHostConnections, $maxHostConnections, $factory);

I tried using 1 for the number of HTTP/2 connections but that changes nothing as far as perf is concerned on my use case (actually it's a bit slower).

I think there should be only one single maxHttpConnections argument here too.

[nicolas-grekas]

PR updated on Symfony's side, now using a single $maxHostConnections setting, all green. Merge + tag pending, then we'll be able to merge on Symfony's side \o/

[kelunik]

@nicolas-grekas We're done. \o/