Skip to content

Fix FiberHandler to support returning promise after awaiting - #312

Open
mdalikadar wants to merge 2 commits into
clue:mainfrom
mdalikadar:fix/fiber-handler-await-then-promise
Open

mdalikadar wants to merge 2 commits into
clue:mainfrom
mdalikadar:fix/fiber-handler-await-then-promise

Conversation

@mdalikadar

@mdalikadar mdalikadar commented Oct 2, 2026 •

Copy link
Copy Markdown

This changeset fixes FiberHandler for request handlers that suspend the
fiber with await() and then return a promise. This makes the example from
#263 work with PHP assertions enabled.

$app->get('/', function () {
    await(sleep(3));
    return resolve(Response::plaintext("Hello world!\n"));
});

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:

┌───────────┬────────────────────────────────────┬───────────────────────────┐
│   Route   │     Before (zend.assertions=1)     │ After (zend.assertions=1) │
├───────────┼────────────────────────────────────┼───────────────────────────┤
│ response  │ 200                                │ 200                       │
├───────────┼────────────────────────────────────┼───────────────────────────┤
│ promise   │ no response, AssertionError logged │ 200                       │
├───────────┼────────────────────────────────────┼───────────────────────────┤
│ generator │ no response, AssertionError logged │ 200                       │
└───────────┴────────────────────────────────────┴───────────────────────────┘

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

  • Opening line and the code block: the example uses sleep() from react/promise-timer, as in the issue, though my own test script used a timer.
  • The generator row: I included it because it shows the fix covers the second thing a reader might wonder about. It's accurate: I measured it before and after.
  • "Resolves Using await and returning response as promise does not work #263": that closes the issue on merge. It's the project's convention for fixing an issue, and the maintainer already agreed the behavior is a bug.
  • Coverage: CI enforces 100% coverage and I couldn't check it locally. If CI complains, the fix is likely one more test.
  • Order of steps: comment on Using await and returning response as promise does not work #263 first, rebase onto 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Using await and returning response as promise does not work

1 participant