Fix FiberHandler to support returning promise after awaiting - #312
Open
mdalikadar wants to merge 2 commits into
Open
mdalikadar wants to merge 2 commits into
mdalikadar wants to merge 2 commits into
Conversation
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.
This changeset fixes
FiberHandlerfor request handlers that suspend thefiber with
await()and then return a promise. This makes the example from#263 work with PHP assertions enabled.
Once the fiber had been suspended and resumed, FiberHandler asserted that the
final result is a ResponseInterface. However, the next handler is allowed to
return a promise (as documented in the method's own docblock and as already
asserted a few lines above), so this assertion was too strict.
With assertions enabled (zend.assertions=1, which is also PHP's built-in
default if no php.ini is loaded), the AssertionError was thrown inside the
fiber and ended up as an unhandled promise rejection. The deferred promise
returned to the HTTP server was never settled, so the client never received a
response and the request hung until the client gave up. With assertions
disabled the code happened to work, because Deferred::resolve() follows a
promise passed to it at runtime.
This changeset accepts a promise here and explicitly makes the deferred follow
it, which also keeps this correct for PHPStan (level max) instead of relying
on that runtime behavior.
I checked this against a real server (X_LISTEN + curl) with a handler that
awaits for 0.2s and then returns (a) a response, (b) a resolved promise and
(c) a generator-based coroutine:
The generator case did not need any special handling here: App already turns
a generator into a promise before the request reaches FiberHandler, so only a
response or a promise can arrive after resuming. The generator route is fixed
by the promise change alone.
Added two tests that fail without this change: returning an already resolved
promise and returning a still pending promise after awaiting a pending promise.
The full test suite and PHPStan pass.
Resolves #263
Notes before you open it
sleep()fromreact/promise-timer, as in the issue, though my own test script used a timer.upstream/main, run the two commits I gave you, then push your branch and open the PR from your fork. I haven't done any of that, and I won't unless you ask.