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.

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

Add Request::onInformationalResponseHandler() to process e.g. 103 Early Hints #239

Merged
merged 1 commit into from
Jan 16, 2020
Merged

Add Request::onInformationalResponseHandler() to process e.g. 103 Early Hints #239

merged 1 commit into from
Jan 16, 2020

Conversation

nicolas-grekas
Copy link
Contributor

@nicolas-grekas nicolas-grekas commented Dec 31, 2019 •
edited
Loading

Will be required to complete symfony/symfony#35115
Draft state for now until I confirm this allows passing the testInformationalResponseStream() test case.
Still, WDYT?

See https://evertpot.com/http/103-early-hints

@nicolas-grekas nicolas-grekas mentioned this pull request Dec 31, 2019
Merged
21 tasks
@nicolas-grekas nicolas-grekas changed the title Add EventListener::handleInformationalResponse() to be able to process 103 Early Hint Add EventListener::handleInformationalResponse() to be able to process 103 Early Hints Dec 31, 2019
@nicolas-grekas nicolas-grekas force-pushed the infoevent branch 2 times, most recently from b93ced2 to 4173970 Compare December 31, 2019 15:25
kelunik
kelunik reviewed Dec 31, 2019
View reviewed changes
src/EventListener.php Outdated Show resolved Hide resolved
@nicolas-grekas nicolas-grekas changed the title Add EventListener::handleInformationalResponse() to be able to process 103 Early Hints Add Request::onInformationalResponseHandler() to process e.g. 103 Early Hints Jan 1, 2020
@nicolas-grekas nicolas-grekas marked this pull request as ready for review January 1, 2020 12:23
@nicolas-grekas
Copy link
Contributor Author

My suggestion is to add it as Request::setInformationalResponseHandler

updated, I confirm this works for Symfony HttpClient

kelunik
kelunik reviewed Jan 1, 2020
View reviewed changes
src/Connection/Internal/Http2ConnectionProcessor.php
$this->handleStreamException(new Http2StreamException('Informational response handler threw an exception', $streamId, self::CANCEL));
}
});
}
Copy link
Member

Choose a reason for hiding this comment

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

If we do it async here, we should probably do the same for HTTP/1?

Copy link
Contributor Author

@nicolas-grekas nicolas-grekas Jan 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I've no opinion on that: I just borrowed from the way event listeners are called. What would you advise?

@kelunik kelunik requested a review from trowski January 12, 2020 18:01
@trowski trowski merged commit 4bea16a into amphp:master Jan 16, 2020
@nicolas-grekas nicolas-grekas deleted the infoevent branch January 27, 2020 20:35
fabpot added a commit to symfony/symfony that referenced this pull request Mar 2, 2020
@fabpot
feature #35115 [HttpClient] Add portable HTTP/2 implementation based …
f632b76 
…on Amp's HTTP client (nicolas-grekas)

This PR was merged into the 5.1-dev branch.

Discussion
----------

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

| 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`](https://github.com/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:
- [x] async request/response
- [x] streaming and multiplexing
- [x] handle all ssl options
- [x] timers info
- [x] upload/download progress info
- [x] upload/download progress callback
- [x] HTTP proxy support
- [x] streamed upload
- [x] public-key pinning
- [x] peer certificate capturing
- [x] stream casting with `$response->toStream()`
- [x] ~amphp/http-client#241
- [x] extensive debug info
- [x] HTTP/2 PUSH support
- [x] amphp/http-client#243
- [x] amphp/http-client#242
- [x] amphp/http-client#250
- [x] amphp/http-client#239
- [x] ~kelunik/certificate#2
- [x] amphp/socket#71
- [x] amphp/http-client#252

Commits
-------

ef113fe [HttpClient] Add portable HTTP/2 implementation based on Amp's HTTP client
symfony-splitter pushed a commit to symfony/http-client that referenced this pull request Mar 2, 2020
@fabpot
feature #35115 [HttpClient] Add portable HTTP/2 implementation based …
3846a7f 
…on Amp's HTTP client (nicolas-grekas)

This PR was merged into the 5.1-dev branch.

Discussion
----------

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

| 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`](https://github.com/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:
- [x] async request/response
- [x] streaming and multiplexing
- [x] handle all ssl options
- [x] timers info
- [x] upload/download progress info
- [x] upload/download progress callback
- [x] HTTP proxy support
- [x] streamed upload
- [x] public-key pinning
- [x] peer certificate capturing
- [x] stream casting with `$response->toStream()`
- [x] ~amphp/http-client#241
- [x] extensive debug info
- [x] HTTP/2 PUSH support
- [x] amphp/http-client#243
- [x] amphp/http-client#242
- [x] amphp/http-client#250
- [x] amphp/http-client#239
- [x] ~kelunik/certificate#2
- [x] amphp/socket#71
- [x] amphp/http-client#252

Commits
-------

ef113feeb3 [HttpClient] Add portable HTTP/2 implementation based on Amp's HTTP client
sadafrangian3 pushed a commit to sadafrangian3/Dependency-Injection-http-client that referenced this pull request Nov 2, 2022
feature #35115 [HttpClient] Add portable HTTP/2 implementation based …
5d33bdc 
…on Amp's HTTP client (nicolas-grekas)

This PR was merged into the 5.1-dev branch.

Discussion
----------

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

| 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`](https://github.com/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:
- [x] async request/response
- [x] streaming and multiplexing
- [x] handle all ssl options
- [x] timers info
- [x] upload/download progress info
- [x] upload/download progress callback
- [x] HTTP proxy support
- [x] streamed upload
- [x] public-key pinning
- [x] peer certificate capturing
- [x] stream casting with `$response->toStream()`
- [x] ~amphp/http-client#241
- [x] extensive debug info
- [x] HTTP/2 PUSH support
- [x] amphp/http-client#243
- [x] amphp/http-client#242
- [x] amphp/http-client#250
- [x] amphp/http-client#239
- [x] ~kelunik/certificate#2
- [x] amphp/socket#71
- [x] amphp/http-client#252

Commits
-------

ef113feeb3 [HttpClient] Add portable HTTP/2 implementation based on Amp's HTTP client
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kelunik kelunik kelunik approved these changes

@trowski trowski Awaiting requested review from trowski

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nicolas-grekas @kelunik @trowski