Repository navigation
[Client] Send HTTP requests from outside the request fiber - #599
Open
aton-of-data wants to merge 1 commit into
Open
aton-of-data wants to merge 1 commit into
aton-of-data wants to merge 1 commit into
Conversation
An amphp-backed PSR-18 client (Symfony's AmpHttpClient, or amphp's own
adapter) suspends the fiber it is called from and waits for the Revolt
event loop to resume it. HttpTransport::send() ran inside the request
fiber, so nothing drove that loop: the request never left the process,
and processFiber() eventually resumed Amp's suspension with the timeout
Error, which surfaced as "FutureIterator::consume(): Return value must
be of type ?array, Mcp\Schema\JsonRpc\Error returned".
Inside the request fiber, send() now suspends with the message and the
transport's tick sends it from {main}, where Revolt runs its loop
itself. Response body reads happen there too, since they suspend the
same way. A failed send is thrown back into the fiber, so it still
surfaces from send(), and the cancellation notification sent after a
timeout or a cancellation goes out the same way.
Fixes modelcontextprotocol#504
aton-of-data
requested review from
CodeWithKyrian,
Nyholm,
chr-hertel and
soyuka
as code owners
October 14, 2026 00:11
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Interruption handling can enter a blocking SSE read before sending the yielded cancellation notification.
1 open finding
What changed in this PR
Moves HTTP I/O outside SDK-managed request fibers to support Amp-based PSR-18 clients.
Changes:
- Defers fiber-originated sends to the transport loop.
- Preserves suspension state during initialization and cancellation.
- Adds Amp-like fiber suspension coverage and changelog documentation.
| File | Description |
|---|---|
src/Client/Transport/HttpTransport.php |
Processes HTTP sends outside request fibers. |
tests/Unit/Client/Transport/HttpTransportTest.php |
Tests fiber-suspending HTTP clients and response bodies. |
CHANGELOG.md |
Documents the compatibility fix. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+613
to
+614
| // The fiber may step out again to send the cancellation notification. | ||
| $this->activeSuspend = $this->activeFiber->throw($error); |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #504
Reproduction and mechanism: #504 (comment). Lines are on main at d1e8091.
Why it fails
connect()runs the handshake in a fiber (HttpTransport.php:141-148), andsend()callssendRequest()inside it (line 211). An amphp-based client does a bareFiber::suspend()there and waits for the Revolt loop, which only runs from{main}. So the request never leaves the process.processFiber()then resumes it with the timeoutError(line 1027), and Amp throws theFutureIterator::consume()TypeError. Body reads (line 261) suspend the same way.Change
send()suspends with the message.tick()first callsprocessSends(), which sends it from{main}and resumes the fiber. A send failure is thrown back into the fiber.processFiber()skips a fiber that waits for a send.connect()keeps whatstart()returns, asrunRequest()does.checkInterruption()keeps whatthrow()returns: thenotifications/cancelledsent since [Client] ThrowTimeoutExceptionon request timeout and cancel the request #567 now steps out too.The errors from #577 are stored during the send, so the fiber still finds them as an immediate response. Every client takes this path, so the existing transport tests cover it too.
Verification
PHP 8.3.6,
composer update --prefer-stable.sendRequest()and body reads. On main it fails withHTTP request failed: Resumed by something other than the event loop, with Mcp\Schema\JsonRpc\Error.With the change it passes.php-cs-fixer fix --dry-run: clean.DualEraElicitationTesterrors on main and with the change. That example prints its shebang underphp -Shere.Interop\Server\Httperrors on main and with the change. All 72Interop\Clienttests pass (1 skipped).Not verified
Psr18Client(new AmpHttpClient())on this base (needs PHP 8.4).listen: truewith Amp.ci-lowest, PHP other than 8.3.