[HttpClient] Add support for amphp/http-client v5 #43
Conversation
35b6b12 to
193a288
Compare
There was a problem hiding this comment.
Here is a WIP aimed at adding support for AMPv3, aka fibers.
I didn't grasp my head around a few things, see review comments.
But the most critical thing I don't get for now is how to change the code in AmpResponse::select() and AmpResponse::stopLoop().
/cc @kelunik would you be available to have a look and give me some hints maybe? 🙏
916d9a5 to
bf2947d
Compare
|
@nicolas-grekas Could you please do separate commits instead of force pushing for now? Makes it much easier to see what changed. |
|
Sure, thanks for reviewing! I pushed something with a static suspension before seeing your comment and it breaks of course :) |
ebfd618 to
8483639
Compare
|
I pushed something that might be closer to the working solution, but I'm not there yet. |
61d82d2 to
97c9be2
Compare
| @@ -152,7 +133,7 @@ public function __wakeup() | |||
| public function __destruct() | |||
| { | |||
| try { | |||
| $this->doDestruct(); | |||
| //$this->doDestruct(); | |||
There was a problem hiding this comment.
I figured out the blocker: we cannot switch fibers in destructors.
Most tests pass now. The ones that rely on destructors fail of course.
I tried this instead:
if ($this->initializer && null === $this->info['error']) {
$clone = clone $this;
$clone->shouldBuffer = true;
EventLoop::queue($clone->getHeaders(...));
}It doesn't work (yet?) but I fear it cannot work because it relies on the event loop having a chance to be resumed after the destructor is called, which might not be always the case.
I might miss something of course.
There was a problem hiding this comment.
I can make many tests pass by patching php and Amp this way.
I'm going to need your help @kelunik and @trowski to figure out if there is something to improve in either PHP, Amp and/or Symfony.
- allow switching fibers in destructors:
--- a/Zend/zend_fibers.c
+++ b/Zend/zend_fibers.c
@@ -391,7 +391,7 @@ ZEND_API void zend_fiber_switch_unblock(void)
ZEND_API bool zend_fiber_switch_blocked(void)
{
- return zend_fiber_switch_blocking;
+ return false;//zend_fiber_switch_blocking;
}- ignore
Cannot suspend in a force-closed fiber:
--- a/src/EventLoop/Internal/AbstractDriver.php
+++ b/src/EventLoop/Internal/AbstractDriver.php
@@ -538,6 +538,7 @@ abstract class AbstractDriver implements Driver
private function createCallbackFiber(): void
{
$this->callbackFiber = new \Fiber(function (): void {
+ try {
do {
$this->invokeMicrotasks();
@@ -599,6 +600,8 @@ abstract class AbstractDriver implements Driver
/** @noinspection PhpUnhandledExceptionInspection */
\Fiber::suspend($this->internalSuspensionMarker);
} while (true);
+ } catch (\FiberError) {
+ }
});
}There was a problem hiding this comment.
Allowing fiber switches in destructors requires the garbage collector to be safe for re-entry from multiple fibers.
It's also generally unadvisable to switch fibers in a finally block, which is likely what is causing the force-closed fiber issue. This is similar to force-closing a generator, you cannot yield in the finally block.
Perhaps the solution then is to move any I/O out of a finally block?
You can switch fibers in a shutdown function, in case that is of any help.
I took a short look at the code this morning and… wow there's a lot going on here through multiple layers. Wrapping $this->doDestruct() into EventLoop::queue($this->doDestruct(...)); had some promising results, but throwing the exceptions from the destructor to the event loop handler. Perhaps wrap that call with a catch that adds the exceptions to a list and re-throws them at a later time? Can initialize() be run immediately upon receiving a response rather than waiting until the destructor to run if it wasn't otherwise run?
There was a problem hiding this comment.
Calling initialize() soon after creating an AmpResponse instance seems more promising, but I'm getting exceptions thrown in the wrong places in tests I think. I'm going to bow out here for now, I have a rather busy weekend ahead of me. Hopefully @nicolas-grekas you can run with these ideas and make some progress.
There was a problem hiding this comment.
Thanks for having a look! Calling initialize means waiting for the headers of the response to come back. This means we cannot call it immediately since this would make everything synchronous.
A common pattern with the concurrency model of HttpClient is to stack requests in an array and let them complete when the array is released:
$pool = [];
foreach ($urls as $url) {
$pool[] = $client->request('GET', $url);
}
unset($pool); // either explicit or more commonly implicit on exit of a methodThe only way to make this API work is by doing I/O in destructors I'm afraid.
Wrapping $this->doDestruct() into EventLoop::queue($this->doDestruct(...)); had some promising results, but throwing the exceptions from the destructor to the event loop handler
That's the closest we can have now indeed. But this requires running inside the event loop, which is not the most common for HttpClient. Alternatively, this could work with a new method on the client to wait for requests in such a pool, but this would break the current API, and the current API has one major desired feature: if ppl forget about checking their responses, they will get an exception if a 4xx/5xx is returned. This also requires doing I/O in destructors, and cannot be solved by any extra method of course.
When not running in an event loop, I thought about pooling destructed responses in a queue to check on every other method call on the client/responses. This would have the quite unpleasant effect or running those destructed requests out of context (think call-stack). This also wouldn't work for the very last set of responses. Yes, shutdown function (or $client->reset() as called by eg Symfony DI) could provide this missing tick, but the DX looks terrible...
An alternative idea could be to run in some sort of fiber-less mode inside destructors - that'd be a sync mode I suppose. Is that something that AMP could be of any help for?
Allowing fiber switches in destructors requires the garbage collector to be safe for re-entry from multiple fibers
Is it correct or not that calling destructors during collect cycles is the step that requires this reentry, and that calling them on refcount=0 doesn't? If correct, would it be possible to allow switching fibers in refcount=0 situations? This would partially fix the problem for HttpClient since response objects are rarely caught in ref cycles.
Of course, the next question is: would it be possible to make the GC safe for re-entry from multiple fibers? The current situation makes destructors kinda reintroduce the “What color is your function?” problem, which fibers were designed to avoid.
There was a problem hiding this comment.
$pool = []; foreach ($urls as $url) { $pool[] = $client->request('GET', $url); } unset($pool); // either explicit or more commonly implicit on exit of a method
That's quite a strange design choice and pattern IMO, which is indeed not possible to implement right now with fibers. In general, destructors shouldn't really do any work, similar to constructors.
The current situation makes destructors kinda reintroduce the “What color is your function?” problem, which fibers were designed to avoid.
Indeed, but it's avoidable if destructors don't do any work, which worked quite well for us so far.
There was a problem hiding this comment.
I had a quick look at amphp/http-client v5 and you have a few destructors that do just that: i/o to e.g. close sockets (and deferring to EventLoop::queue() to do so). The fact that Symfony's client does more than just closing sockets is not a reason to dismiss its style of API. This should be fully supported by the engine.
I get that this might be significant work of course.
Do you want me to open an issue on php-src? Who could help on the implementation? @trowski would you? Who else?
There was a problem hiding this comment.
Please open an issue in php-src. I'd like to see support for suspensions in destructors as well. It's been on my (long) todo list, but now might be a good time to move it toward the top. Unfortunately I know little about the GC, so will probably need some help.
We've avoided suspending in destructors by using a defer in the event loop, but this probably only really works in a long running app (and runs the risk of never running). Closing a socket doesn't require suspending because there's no result to await, but it is I/O, so that's a fair criticism.
I glanced through the GC code today, and it looks to already guard against re-entry. The concern of course would be that if a destructor which suspended never again resumed it would block the GC from ever being run again. I'm not sure if this is acceptable or not.
…he monorepo (fabpot, dunglas, KorvinSzanto, xabbuh, aimeos, ahundiak, Danielss89, rougin, csunolgomez, Jérôme Parmentier, mtibben, Nyholm, ajgarlag, uphlewis, samnela, grachevko, nicolas-grekas, tinyroy, danizord, Daniel Degasperi, rbaarsma, Ekman, 4rthem, derrabus, mleczakm, iluuu1994, Tobion, chalasr, lemon-juice, franmomu, cidosx, erikn69, AurelienPillevesse) This PR was merged into the 6.4 branch. Discussion ---------- [PsrHttpMessageBridge] Import the bridge into the monorepo | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | N/A | License | MIT | Doc PR | TODO⚠️ Don't squash! I propose to import the `symfony/psr-http-message-bridge` package into the Symfony monorepo for further maintenance. Commits ------- e40dd66 [PsrHttpMessageBridge] Patch return types and fix CS 266c09f [PsrHttpMessageBridge] Import the bridge into the monorepo 0c0323a Add 'src/Symfony/Bridge/PsrHttpMessage/' from commit '581ca6067eb62640de5ff08ee1ba6850a0ee472e' 581ca60 Prepare release 2.3.1 45d0349 Fix CS 6410dda bug symfony#122 Don't rely on Request::getPayload() to populate the parsed body (nicolas-grekas) ef03b6d Don't rely on Request::getPayload() to populate the parsed body 3c62b81 minor symfony#120 Prepare release 2.3.0 (derrabus) 96acbfd Prepare release 2.3.0 7eedd34 feature symfony#119 Implement ValueResolverInterface (derrabus) 0b54b85 Implement ValueResolverInterface 6b2f5df feature symfony#117 Leverage `Request::getPayload()` to populate the parsed body of PSR-7 requests (AurelienPillevesse) 3a8caad Leverage `Request::getPayload()` to populate the parsed body of PSR-7 requests 18c9e82 minor symfony#118 Add native types where possible (derrabus) 4fd4323 Add native types where possible 28a732c minor symfony#115 Prepare the 2.2.0 release (derrabus) 7944831 cs fix 99ddcaa Prepare the 2.2.0 release 8a5748d feature symfony#113 Bump psr/http-message version (erikn69) ec83c1c Bump psr/http-message version 694016e feature symfony#114 Drop support for Symfony 4 (derrabus) b360b35 Drop support for Symfony 4 998d8d2 minor symfony#111 Adjustments for PHP CS Fixer 3 (derrabus) 5fa5f62 Adjustments for PHP CS Fixer 3 a125b93 minor symfony#110 Add PHP 8.2 to CI (derrabus) 4592df2 Add PHP 8.2 to CI 4617ac3 bug symfony#109 perf: ensure timely flush stream buffers (cidosx) 8c8a75b perf: ensure timely flush stream buffers d444f85 Update changelog 155a7ae bug symfony#107 Ignore invalid HTTP headers when creating PSR7 objects (nicolas-grekas) 9a78a16 Ignore invalid HTTP headers when creating PSR7 objects bdb2871 minor symfony#104 Add missing .gitattributes (franmomu) 808561a Add missing .gitattributes 316f5cb bug symfony#103 Fix for wrong type passed to moveTo() (lemon-juice) 7f3b5c1 Fix for wrong type passed to moveTo() 22b37c8 minor symfony#101 Release v2.1.2 (chalasr) c382d76 Release v2.1.2 c81476c feature symfony#100 Allow Symfony 6 (chalasr) c7a0be3 Allow Symfony 6 df83a38 minor symfony#98 Add PHP 8.1 to CI (derrabus) b2bd334 Add PHP 8.1 to CI 824711c minor symfony#99 Add return types to fixtures (derrabus) f8f70fa Add return types to fixtures d558dcd minor symfony#97 Inline $tmpDir (derrabus) d152649 Inline $tmpDir f12a9e6 minor symfony#96 Run PHPUnit on GitHub Actions (derrabus) ab64c69 Run PHPUnit on GitHub Actions c901299 bug symfony#95 Allow `psr/log` 2 and 3 (derrabus) 8e13ae4 Allow psr/log 2 and 3 26068fa Minor cleanups 87fabb9 Fix copyright year 3d9241f minor symfony#92 remove link to sensio extra bundle which removed psr7 support (Tobion) 7078739 remove link to sensio extra bundle which removed psr7 support 81db2d4 feature symfony#89 PSR HTTP message converters for controllers (derrabus) aa26e61 PSR HTTP message converters for controllers e62b239 minor symfony#91 Fix CS (derrabus) 2bead22 Fix CS 488df9b minor symfony#90 Fix CI failures with Xdebug 3 and test on PHP 7.4/8.0 as well (derrabus) a6697fd Fix CI failures with Xdebug 3 and test on PHP 7.4/8.0 as well c62f7d0 Update branch-alias 51a21cb Update changelog a20fff9 bug symfony#87 Fix populating server params from URI in HttpFoundationFactory (nicolas-grekas) 4933e04 bug symfony#86 Create cookies as raw in HttpFoundationFactory (nicolas-grekas) 66095a5 Fix populating server params from URI in HttpFoundationFactory 42cca49 Create cookies as raw in HttpFoundationFactory cffb3a8 bug symfony#85 Fix BinaryFileResponse with range to psr response conversion (iluuu1994) 5d5932d Fix BinaryFileResponse with range to psr response conversion e44f249 bug symfony#81 Don't normalize query string in PsrHttpFactory (nicolas-grekas) bc25829 Don't normalize query string in PsrHttpFactory df735ec bug symfony#78 Fix populating default port and headers in HttpFoundationFactory (mleczakm) 4f30401 Fix populating default port and headers in HttpFoundationFactory 1309b64 bug symfony#77 fix conversion for https requests (4rthem) e86de3f minor symfony#79 Allow installation on php 8 (derrabus) 9243f93 Allow installation on php 8. d336c73 fix conversion for https requests 126903c Fix format of CHANGELOG.md ce709cd feature symfony#75 Remove deprecated code (fabpot) dfc5238 Remove deprecated code 9d3e80d bug symfony#72 Use adapter for UploadedFile objects (nicolas-grekas) a4f9f6d Use adapter for UploadedFile objects ec7892b Fix CHANGELOG, bump branch-alias 7ab4fe4 minor symfony#70 Updated CHANGELOG (rbaarsma) 9ad4bcc Updated CHANGELOG c4c904a minor symfony#71 Cleanup after bump to Symfony v4.4 (nicolas-grekas) e9a9557 Cleanup after bump to Symfony v4.4 3d10a6c feature symfony#66 Add support for streamed Symfony request (Ekman) df26630 Add support for streamed Symfony request 5aa8ca9 bug symfony#69 Allow Symfony 5.0 (rbaarsma) 1158149 Allow Symfony 5.0 81ae86d Merge branch '1.1' a33352a bug symfony#64 Fixed createResponse (ddegasperi) 7a4b449 minor symfony#65 Fix tests (ajgarlag) 19905b0 Fix tests 580de38 Fixed createResponse 9ab9d71 minor symfony#63 Added links to documentation (Nyholm) 59b9406 Added links to documentation c1cb51c feature symfony#50 Add support for streamed response (danizord) 4133c7a bug symfony#48 Convert Request/Response multiple times (Nyholm) 8564bf7 Convert Request/Response multiple times 7cc1605 Add support for streamed response aebc14b feature symfony#62 bump to PHP 7.1 (nicolas-grekas) 8e10923 bump to PHP 7.1 5e5e0c3 Revert "Undeprecate DiactorosFactory for 1.1" 921f866 Undeprecate DiactorosFactory for 1.1 8592ca3 bug symfony#61 removed 'Set-Cookie' from header when it is already converted to a Symfony header cookie (tinyroy) dd1111e removed 'Set-Cookie' from header when it is already converted to a Symfony header cookie ba672d8 bump branch-alias 5f9a032 typo f2c48c5 fix tests 3a52e44 bug symfony#59 Fix SameSite attribute conversion from PSR7 to HttpFoundation (ajgarlag) 5ee1f8f Fix SameSite attribute conversion from PSR7 to HttpFoundation f6d7d3a bug symfony#58 [Bugfix] Typo header set-sookie (grachevko) 16eb6e1 minor symfony#57 Excluded tests from classmap (samnela) 36a8065 Deprecate DiactorosFactory, use nyholm/psr7 for tests 5076934 bug symfony#54 Fix symfony#51 (compatability issue with zendframework/zend-diactoros ^2.0) (uphlewis) 757ea81 [Bugfix] Typo header set-sookie 25f9c3a Excluded tests from classmap 8ff61e5 Fix compatability issue with "zendframework/zend-diactoros": "^2.0." (symfony#51) 53c15a6 updated CHANGELOG c821241 bumped version to 1.1 f26d01f minor symfony#47 Updated changelog (Nyholm) c2282e3 Updated changelog eddd6c8 feature #43 Create PSR-7 messages using PSR-17 factories (ajgarlag) dd81b4b Create PSR-7 messages using PSR-17 factories f11f173 feature #45 Fixed broken build (Nyholm) 8780dd3 Fixed broken build c2b7579 bug #30 Fix the request target in PSR7 Request (mtibben) 94fcfa5 Fix the request target in PSR7 Request 64640ee minor #38 Run PHP 5.3 tests on Precise (Lctrs) 64c0cb0 Run PHP 5.3 tests on Precise b209840 minor #32 Allow Symfony 4 (dunglas) 97635f1 Allow Symfony 4 147a238 minor #31 test suite compatibility with PHPUnit 6 (xabbuh) f5c46f0 test suite compatibility with PHPUnit 6 66085f2 preparing 1.0 release 533d3e4 added a CHANGELOG for 1.0 14269f9 bug #28 Fix REQUEST_METHOD on symfony request (csunol) 98ab85a Fix REQUEST_METHOD on symfony request 29be4f8 updated LICENCE year d2db47c removed obsolete CHANGELOG file 1c30b17 bug #22 Fixes #16 Symfony Request created without any URI (rougin) a59c572 Fixes #16 Symfony Request created without any URI 7a5aa92 bug #23 Fixes #9 Bridge error when no file is selected (ahundiak, Danielss89) a1a631a Update assert error message e5d62e6 Fixes based on code-review 101b608 Handles null file in createrequest bridge. d16c63c bug #18 Allow multiple calls to Request::getContent() (aimeos) 9624b8b Allow multiple calls to Request::getContent() 9c747c4 Merge pull request #19 from xabbuh/travis-config a388c43 update Travis CI configuration ac5cd86 minor #14 Remove use of deprecated 'deep' parameter in tests (KorvinSzanto) 305c0fe Remove use of deprecated 'deep' parameter 3664dc0 minor #7 Test Diactoros Factory with PHP 5.4 (dunglas) bab1530 Test Diactoros Factory with PHP 5.4 d7660b8 Suggest psr/http-message-implementation dc7e308 removed the branch alias for now as we are pre 1.0 3f8977e feature #1 Initial support (dunglas) ca41146 Initial support 01b110b added the initial set of files
requires ^3.50 of Fixer
…ator` (MatTheCat) This PR was merged into the 7.0 branch. Discussion ---------- [Validator] Simplify `NoSuspiciousCharactersValidator` | Q | A | ------------- | --- | Branch? | 7.0 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | N/A | License | MIT php/php-src#10647 has been fixed in PHP 8.1.17. Now that Symfony requires PHP ≥ 8.2, we can avoid calling `Spoofchecker::isSuspicious` for every check by leveraging its `$errorCode` parameter. Commits ------- c24e3e7 [Validator] Simplify `NoSuspiciousCharactersValidator`
…r when PCRE error (smnandre) This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [AssetMapper] Throw exception in Javascript compiler when PCRE error | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #... | License | MIT `preg_match_callback` can return null when a PCRE error occured, leading there to a TypeError. Let's throw an exception and expose the error message. (follows symfony#54078 / complementary to symfony#54079 ) Commits ------- 2333b58 [AssetMapper] Throw exception in Javascript compiler when PCRE error
* 6.4: [AssetMapper] Throw exception in Javascript compiler when PCRE error
* 7.0: [AssetMapper] Throw exception in Javascript compiler when PCRE error Remove undefined variable
This PR was submitted for the 6.4 branch but it was merged into the 7.1 branch instead. Discussion ---------- [Mailer] Simplify MailerTestCommand | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | | License | MIT Should I target SF 7.1? Commits ------- fdb0340 [Mailer] Simplify MailerTestCommand
released v5.4.37
released v6.4.5
released v7.0.5
* 5.4: Bump Symfony version to 5.4.38 Update VERSION for 5.4.37 Update CONTRIBUTORS for 5.4.37 Update CHANGELOG for 5.4.37
…bbarth)
This PR was merged into the 6.4 branch.
Discussion
----------
[HttpClient] Preserve float in JsonMockResponse
| Q | A
| ------------- | ---
| Branch? | 6.4
| Bug fix? | yes
| New feature? | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Issues | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead -->
| License | MIT
Hello,
I am mocking an API Response that can send float in values :
```
{
"values": 1.0
}
```
Converting this to a php array and giving it to JsonMockResponse return me an int after, because the missing json_encode flag to keep zero fraction, and leds to issue when I want to deserialize it into a float property.
In my mind it's a bugfix, but if it's considered as a new feature, I can target 7.1.
Commits
-------
4d120b8 [HttpClient] Preserve float in JsonMockResponse
* 6.4: Bump Symfony version to 6.4.6 Update VERSION for 6.4.5 Update CHANGELOG for 6.4.5 Bump Symfony version to 5.4.38 Update VERSION for 5.4.37 Update CONTRIBUTORS for 5.4.37 Update CHANGELOG for 5.4.37 [HttpClient] Preserve float in JsonMockResponse
* 7.0: Bump Symfony version to 7.0.6 Update VERSION for 7.0.5 Update CHANGELOG for 7.0.5 Bump Symfony version to 6.4.6 Update VERSION for 6.4.5 Update CHANGELOG for 6.4.5 Bump Symfony version to 5.4.38 Update VERSION for 5.4.37 Update CONTRIBUTORS for 5.4.37 Update CHANGELOG for 5.4.37 [HttpClient] Preserve float in JsonMockResponse
|
PR submitted at symfony#54179 |
…olas-grekas) This PR was merged into the 7.2 branch. Discussion ---------- [HttpClient] Add support for amphp/http-client v5 | Q | A | ------------- | --- | Branch? | 7.2 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | #52008 | License | MIT This PR adds support for amphp/http-client version 5 as a transport for our HttpClient component. It started as a draft at nicolas-grekas#43, which helped spot that PHP is missing the capability to suspend fibers in destructors. This was reported as php/php-src#11389 and is being fixed at php/php-src#13460. Since the fix for php-src is going to land on PHP 8.4, using amphp/http-client version 5 will require php >= 8.4. The implementation duplicates the one we have for v4 with the needed changes to use the v5 API. The difference are not big in size of code, but they're very low level (generators vs fibers). That would be quite useless to factor IMHO. Commits ------- a5fb61c [HttpClient] Add support for amphp/http-client v5
No description provided.