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.

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

Documentation suggests finally() will handle rejected promise #263

Closed
lucasnetau opened this issue Nov 5, 2024 · 1 comment
Closed

Documentation suggests finally() will handle rejected promise #263

lucasnetau opened this issue Nov 5, 2024 · 1 comment
Labels

Comments

@lucasnetau
Copy link

Unless I'm reading it wrong, the documentation appears to have an issue with unhandled rejected promises.

A rejected promise will be considered "handled" if you catch the rejection reason with either the then() method, the catch() method, or the finally() method. Note that each of these methods return a new promise that may again be rejected if you re-throw an exception.

However if you are only using a finally() method it will not be considered handled() by the current implementation and an Unhandled promise rejection will be thrown.

<?php declare(strict_types = 1);

use React\EventLoop\Loop;
use React\Promise\Deferred;

require_once __DIR__ . '/../vendor/autoload.php';

$deferred = new Deferred(function() use (&$deferred) {
    $deferred->reject(new \RuntimeException('Request cancelled'));
});

$promise = $deferred->promise();

$promise->finally(function() {
    Loop::stop();
});

Loop::get()->futureTick(function () use (&$deferred) {
    $deferred->reject(new \RuntimeException('Error'));
});

Loop::get()->run();
@lucasnetau lucasnetau added the bug label Nov 5, 2024
@clue clue added question and removed bug labels Nov 19, 2024
@clue
Copy link
Member

clue commented Nov 19, 2024

@lucasnetau Thanks for reporting, I can see where you're coming from and agree the documentation could be improved 👍

Technically, the finally() method does indeed mark the original promise "handled" – however, it will also return a new child promise that will not be marked "handled" automatically. As a result, you would still see the same "unhandled promise rejection" even if it may be coming from a different promise. I can see why this would be confusing behavior and hard to read from the current documentation. This behavior is also covered by a number of tests in #248 and #249.

I hope this helps! I believe this has been answered, so I'll close this for now. That said, I agree we may want to improve the documentation. PRs welcome I guess? :shipit:

@clue clue closed this as completed Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants