Skip to content

[HttpClient] make HttpClient::create() return an AmpHttpClient when amphp/http-client is found but curl is not or too old #35924

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

Merged
merged 1 commit into from
Mar 16, 2020

Conversation

nicolas-grekas
Copy link
Member

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR -

Follows #35115

Let's use amphp/http-client by default, after curl and before fopen().

stof
stof previously requested changes Mar 2, 2020
@nicolas-grekas nicolas-grekas changed the title [HttpClient] make HttpClient::create() return an AmpHttpClient when amphp/http-client is found but curl is not [HttpClient] make HttpClient::create() return an AmpHttpClient when amphp/http-client is found but curl is not or too old Mar 2, 2020
@tristanbes
Copy link
Contributor

Aweomse, since majority of PaaS outhere have a bad habit of using a really outdated curl version. 🎉

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Mar 3, 2020

@kelunik the test suite of the Messenger component is failing when using AmpHttpClient, with:

Idle timeout reached for "http://0.0.0.0:9494/messages".

The reproducer is:

docker pull feathj/fake-sqs
docker run -d -p 9494:9494 --name sqs feathj/fake-sqs
export MESSENGER_SQS_DSN=sqs://localhost:9494/messages?sslmode=disable
./phpunit src/Symfony/Component/Messenger/ -v --filter Sqs

At first, I thought the reason was the 0.0.0.0 in the URL, but actually when I force 127.0.0.1 instead, the timeout persists.

The test passes when using the curl or the native client.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Mar 3, 2020

The test passes when I use an UnlimitedConnectionPool instead of ConnectionLimitingPool.

@trowski
Copy link
Contributor

trowski commented Mar 3, 2020

@nicolas-grekas Do you still have your benchmarking script handy? I was wondering how the Amp client compares to curl, particularly for a single request and many requests to the same host, both HTTP/1.1 and HTTP/2.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Mar 3, 2020

Sure, here it is.
On h2, AMP is comparable to Curl.
On h1.1, AMP looks slower, dunno why.

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

$client = new Symfony\Component\HttpClient\AmpHttpClient(['http_version' => 1.1]);

for ($i = 0; $i < 379; ++$i) {
    $uri = "https://http2.akamai.com/demo/tile-$i.png";
    $responses[] = $client->request('GET', $uri);
}

foreach ($client->stream($responses) as $response => $chunk) {
    if ($chunk->isLast()) {
        // a $response completed
        echo '.';
    } else {
        // a $response's got network activity or timeout
    }
}

echo "\n";

@nicolas-grekas nicolas-grekas force-pushed the hc-amp-default branch 4 times, most recently from 0eb451f to 87c5be9 Compare March 12, 2020 17:22
…en `amphp/http-client` is found but curl is not or too old
@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Mar 13, 2020

Green.

Status: needs review

@fabpot
Copy link
Member

fabpot commented Mar 16, 2020

Thank you @nicolas-grekas.

@trowski
Copy link
Contributor

trowski commented Mar 16, 2020

@nicolas-grekas Thanks for the script again. I see the same results: About the same for HTTP/2, slower for HTTP/1.x. Will have to investigate what's slowing down HTTP/1.x.

I was fishing for a reason to have Amp's client be the default client, but I don't really have a compelling reason for that other than installation consistency and portability. 😝

Is there a way to suggest using Amp if the client falls back to using NativeHttpClient?

@nicolas-grekas nicolas-grekas deleted the hc-amp-default branch March 16, 2020 20:11
@nicolas-grekas
Copy link
Member Author

Is there a way to suggest using Amp if the client falls back to using NativeHttpClient?

We already have an @trigger_error() in HttpClient::create() when curl is missing. Please submit a PR to add a similar one when NativeHttpClient is used now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants