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

Cannot multiplex requests on a single H2 connection #246

Closed
nicolas-grekas opened this issue Jan 8, 2020 · 19 comments
Closed

Cannot multiplex requests on a single H2 connection #246

nicolas-grekas opened this issue Jan 8, 2020 · 19 comments

Comments

@nicolas-grekas
Copy link
Contributor

@nicolas-grekas nicolas-grekas commented Jan 8, 2020

Spotted at symfony/symfony#35115 (comment)

The following script adapted from the examples doesn't multiplex the requests.
Note that we limit to one single connection. If we remove this limit, one connection per request is made and all works concurrently - but then this is no different than using HTTP/1.1.

<?php

use Amp\Http\Client\Connection\LimitedConnectionPool;
use Amp\Http\Client\Connection\UnlimitedConnectionPool;
use Amp\Http\Client\HttpClientBuilder;
use Amp\Http\Client\HttpException;
use Amp\Http\Client\Request;
use Amp\Http\Client\Response;
use Amp\Loop;
use Amp\Sync\LocalKeyedSemaphore;

require __DIR__ . '/vendor/autoload.php';

Loop::run(static function () use ($argv) {
    $uris = [];

    for ($i = 0; $i < 379; ++$i) {
        $uris[] = "https://http2.akamai.com/demo/tile-$i.png";
    }

    // Instantiate the HTTP client
    $pool = LimitedConnectionPool::byHost(new UnlimitedConnectionPool, new LocalKeyedSemaphore(1));
    $client = (new HttpClientBuilder)
            ->usingPool($pool)
            ->build();

    $requestHandler = static function (string $uri) use ($client) {
        /** @var Response $response */
        $response = yield $client->request(new Request($uri));

        return yield $response->getBody()->buffer();
    };

    try {
        $promises = [];

        foreach ($uris as $uri) {
            $promises[$uri] = Amp\call($requestHandler, $uri);
        }

        $bodies = yield $promises;

        foreach ($bodies as $uri => $body) {
            print $uri . " - " . \strlen($body) . " bytes" . PHP_EOL;
        }
    } catch (HttpException $error) {
        // If something goes wrong Amp will throw the exception where the promise was yielded.
        // The HttpClient::request() method itself will never throw directly, but returns a promise.
        echo $error;
    }
});
@kelunik

This comment has been minimized.

Copy link
Member

@kelunik kelunik commented Jan 8, 2020

Indeed, currently it limits concurrent requests instead of concurrent connections. I guess having the same connection limit for both isn't what users generally want.

@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

@nicolas-grekas nicolas-grekas commented Jan 8, 2020

having the same connection limit for both isn't what users generally want.

sure! Note that this is not my issue here :) Is there a way to multiplex several requests on a single h2 connection? That's what I don't know how to do here. Is that a known limitation?

@kelunik

This comment has been minimized.

Copy link
Member

@kelunik kelunik commented Jan 8, 2020

Sure, that will happen by default, but if you limit to a single concurrent request, it will obviously not happen.

@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

@nicolas-grekas nicolas-grekas commented Jan 8, 2020

But it should be possible to have one single connection to a remote host and still do several h2 requests, since that's a main feature of the protocol. curl allows it.
Right now, multiplexing works by creating one socket per request, isn't it? Maybe I miss something :)

@trowski

This comment has been minimized.

Copy link
Member

@trowski trowski commented Jan 8, 2020

H2 connections can have many streams per connection. Only a single connection is actually made.

The problem is that LimitedConnectionPool is too naïve and is allowing only a single request, even if the connection being used is H2.

@kelunik I think we should change LimitedConnectionPool to store the connections returned and attempt to reuse them for subsequent requests, only acquiring a lock and waiting if Connection::getStream() returns null.

@kelunik

This comment has been minimized.

Copy link
Member

@kelunik kelunik commented Jan 8, 2020

The way it's currently written, it could have been a simple interceptor, I guess.

@trowski I don't see a way to implement that in the current implementation, because we don't ever have access to the connection object in LimitedConnectionPool. I'd have to be a separate implementation.

@trowski

This comment has been minimized.

Copy link
Member

@trowski trowski commented Jan 8, 2020

Oh yes, right… it's only returning Stream objects. Yep, then it will have to be a whole new, separate implementation.

@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

@nicolas-grekas nicolas-grekas commented Jan 9, 2020

This is the callgraph of the above script without the LimitedConnectionPool.

https://blackfire.io/profiles/20a80a98-340c-4327-b966-4c9db0c4f679/graph

As you can see, it opens several connections to the host.
stream_socket_enable_crypto is called 845 times, which makes it the most exclusive-time consuming function, and ResourceSocket::__construct is called 280 times.

I'll have a closer look, but if you have some hints meanwhile, I'd be happy to read them.

@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

@nicolas-grekas nicolas-grekas commented Jan 9, 2020

OK, here is what happens: the script requests more than the maximum number of allowed concurrent streams.

This means $stream = yield $connection->getStream($request); returns null in UnlimitedConnectionPool, so that a second connection is opened once the limit is reached.
Then, all next requests open a separate connection, because $isHttps && \count($connections) === 1 is false as of now.

Here is the Blackfire profile that shows only one connection is opened:
https://blackfire.io/profiles/4b7fe9f4-c427-45fb-9b4d-ce0d67881d4f/graph

I suppose the pool logic could be improved :)

@trowski

This comment has been minimized.

Copy link
Member

@trowski trowski commented Jan 9, 2020

Thanks for figuring that out… I was scratching my head for the last half hour trying to figure out why the H2 connection wasn't being reused. Can you try removing && \count($connections) === 1. That should improve things quite a bit.

@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

@nicolas-grekas nicolas-grekas commented Jan 9, 2020

Can you try removing && \count($connections) === 1.

No difference, it still opens new connections after the threshold.
Please run the script if you want to verify :)

@trowski

This comment has been minimized.

Copy link
Member

@trowski trowski commented Jan 9, 2020

Tried the script – it only used a single connection, but it also seemed to only be making one request at a time. Is there something I need to do so all requests are sent at once?

EDIT: I was trying the script you have on the symfony PR, not the one above. Let me investigate further.

@trowski

This comment has been minimized.

Copy link
Member

@trowski trowski commented Jan 9, 2020

@nicolas-grekas Just pushed 1181944, which should fix your problem. Please pull the latest master and give it a try. 😃

@kelunik

This comment has been minimized.

Copy link
Member

@kelunik kelunik commented Jan 9, 2020

@trowski We can't leave 1181944 as is, because that will make the performance for HTTP/1.1 much worse, because every request needs to await any currently in progress connections. I will have to think about the optimal algorithm there.

@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

@nicolas-grekas nicolas-grekas commented Jan 9, 2020

Just pushed 1181944, which should fix your problem

Confirmed! That's the right direction apparently :)

@trowski

This comment has been minimized.

Copy link
Member

@trowski trowski commented Jan 9, 2020

@kelunik Yes, I realized that, but I wasn't sure about what a good solution there would be and I wanted to confirm that at least solved @nicolas-grekas's problem.

First idea I had is examine the first connection returned, and if HTTP/1.x, assume that the next one will also be HTTP/1.x and immediately make a new connection.

trowski added a commit that referenced this issue Jan 9, 2020
If a prior HTTPS connection returned a non-H2 connection, the pool assumes the next connection will also not be H2.

Related to #246.
trowski added a commit that referenced this issue Jan 9, 2020
If a prior HTTPS connection returned a non-H2 connection, the pool assumes the next connection will also not be H2.

Related to #246.
@kelunik

This comment has been minimized.

Copy link
Member

@kelunik kelunik commented Jan 13, 2020

Closing, as this has been fixed on master.

@kelunik kelunik closed this Jan 13, 2020
@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

@nicolas-grekas nicolas-grekas commented Jan 14, 2020

Thank you!
Should I open a separate issue about LimitedConnectionPool?

@kelunik

This comment has been minimized.

Copy link
Member

@kelunik kelunik commented Jan 14, 2020

Yup, please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.