Skip to content

[HttpClient] Add portable HTTP/2 implementation based on Amp's HTTP client #35115

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.

Sign up for GitHub

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

Jump to bottom
Merged
fabpot merged 1 commit into from
Mar 2, 2020
Merged

[HttpClient] Add portable HTTP/2 implementation based on Amp's HTTP client #35115

fabpot merged 1 commit into from
Mar 2, 2020

Conversation

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 26, 2019 •
edited by stof
Loading

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

This PR provides an AmpHttpClient, which is an adapter between amphp/http-client and symfony/http-client-contracts.

This is an early experiment for now, but it works already on the happy path: I have a local h2-intensive script, and while it's slower than CurlHttpClient, this performs quite well!

This could provide a portable implementation of HTTP/2 \o/

/cc @kelunik FYI

Todo:

@nicolas-grekas nicolas-grekas added this to the next milestone Dec 26, 2019
nicolas-grekas
nicolas-grekas commented Dec 26, 2019
View reviewed changes
@nicolas-grekas nicolas-grekas changed the title [HttpClient] Add portable implementation based on Amp's HTTP client [HttpClient] Add portable HTTP/2 implementation based on Amp's HTTP client Dec 26, 2019
kelunik
kelunik reviewed Dec 27, 2019
View reviewed changes
kelunik
kelunik reviewed Dec 28, 2019
View reviewed changes
@nicolas-grekas nicolas-grekas force-pushed the hc-amphp branch 6 times, most recently from 8b4f81a to 96b3fc5 Compare December 30, 2019 17:08
@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Dec 30, 2019

Now with fixed redirection logic (not reusing the existing one from Amp because we want a different behavior that is consistent with curl+native)
I also added todo notes, which could be questions for you @kelunik :)

kelunik
kelunik reviewed Dec 30, 2019
View reviewed changes
kelunik
kelunik reviewed Dec 30, 2019
View reviewed changes
kelunik
kelunik reviewed Dec 30, 2019
View reviewed changes
kelunik
kelunik reviewed Dec 30, 2019
View reviewed changes
kelunik
kelunik reviewed Dec 30, 2019
View reviewed changes
kelunik
kelunik reviewed Dec 30, 2019
View reviewed changes
kelunik
kelunik reviewed Dec 30, 2019
View reviewed changes
@nicolas-grekas nicolas-grekas mentioned this pull request Dec 31, 2019
Merged
@nicolas-grekas nicolas-grekas mentioned this pull request Dec 31, 2019
Closed
@nicolas-grekas nicolas-grekas force-pushed the hc-amphp branch 4 times, most recently from 0713fbd to e0537ca Compare February 7, 2020 22:03
@nicolas-grekas nicolas-grekas force-pushed the hc-amphp branch 2 times, most recently from 100e79d to ab72bba Compare February 17, 2020 15:41
@nicolas-grekas nicolas-grekas force-pushed the hc-amphp branch 3 times, most recently from fd7d7db to 787b074 Compare February 29, 2020 12:02
@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Feb 29, 2020

PR is ready for final review!

Wirone
Wirone reviewed Mar 2, 2020
View reviewed changes
src/Symfony/Component/HttpClient/AmpHttpClient.php Outdated

/**
* @param array $defaultOptions Default requests' options
* @param int $maxHostConnections The maximum number of connections to open
Copy link
Contributor

@Wirone Wirone Mar 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for such minor comment, but I don't get how phpDoc works here. There are 2 missing arguments here while documented ones don't add any valuable information (arguments' names are self-descriptive, they are typed). For me only @see adds something valuable here.

Copy link
Member Author

@nicolas-grekas nicolas-grekas Mar 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback, all arguments are now documented.

@fabpot
Copy link
Member

fabpot commented Mar 2, 2020

Thank you @nicolas-grekas.

@fabpot fabpot mentioned this pull request Mar 2, 2020
Closed
@fabpot fabpot merged commit f632b76 into symfony:master Mar 2, 2020
@nicolas-grekas nicolas-grekas deleted the hc-amphp branch March 2, 2020 14:30
@nicolas-grekas nicolas-grekas mentioned this pull request Mar 2, 2020
Merged
stof
stof reviewed Mar 2, 2020
View reviewed changes
.appveyor.yml
- SET SYMFONY_PHPUNIT_SKIPPED_TESTS=phpunit.skipped
- copy /Y c:\php\php.ini-min c:\php\php.ini
- IF %APPVEYOR_REPO_BRANCH% neq master (rm -Rf src\Symfony\Bridge\PhpUnit)
- mv src\Symfony\Component\HttpClient\phpunit.xml.dist src\Symfony\Component\HttpClient\phpunit.xml
Copy link
Member

@stof stof Mar 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need this ?

Copy link
Member Author

@nicolas-grekas nicolas-grekas Mar 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To run the tests for the component separately: they block when run concurrently with other components, and we don't care as this doesn't happen in practice.

fabpot added a commit that referenced this pull request Mar 16, 2020
@fabpot
feature #35924 [HttpClient] make HttpClient::create() return an `Am…
d5c4ba9 
…pHttpClient` when `amphp/http-client` is found but curl is not or too old (nicolas-grekas)

This PR was merged into the 5.1-dev branch.

Discussion
----------

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

| 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()`.

Commits
-------

7991685 [HttpClient] made `HttpClient::create()` return an `AmpHttpClient` when `amphp/http-client` is found but curl is not or too old
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@fabpot fabpot mentioned this pull request May 5, 2020
Merged
@xabbuh xabbuh mentioned this pull request Nov 20, 2024
Closed
@nicolas-grekas nicolas-grekas mentioned this pull request Nov 20, 2024
Merged
nicolas-grekas added a commit that referenced this pull request Nov 20, 2024
@nicolas-grekas
minor #58951 add comment explaining why HttpClient tests are run sepa…
85419bc 
…rately (xabbuh)

This PR was merged into the 5.4 branch.

Discussion
----------

add comment explaining why HttpClient tests are run separately

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

see #35115 and #58916

Commits
-------

cbdb08a add comment explaining why HttpClient tests are run separately
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stof stof stof left review comments

@fabpot fabpot fabpot approved these changes

+2 more reviewers

@Wirone Wirone Wirone left review comments

@kelunik kelunik kelunik left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature HttpClient Status: Reviewed

Projects

None yet

Milestone

5.1

Development

Successfully merging this pull request may close these issues.

6 participants

@nicolas-grekas @fabpot @stof @Wirone @kelunik @carsonbot