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

Report any unhandled promise rejections #248

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

clue
Copy link
Member

@clue clue commented Jul 6, 2023

This changeset adds new functionality to make sure we report any unhandled promise rejections. If you remove the last reference to a rejected promise that has not been handled, it will now report an unhandled promise rejection:

function incorrect(): int
{
     $promise = React\Promise\reject(new RuntimeException('Request failed'));

     // Commented out: No rejection handler registered here.
     // $promise->then(null, function (\Throwable $e): void { /* ignore */ });

     // Returning from a function will remove all local variable references, hence why
     // this will report an unhandled promise rejection here.
     return 42;
}

// Calling this function will log an error message plus its stack trace:
// Unhandled promise rejection with RuntimeException: Request failed in example.php:10
incorrect();

The old behavior was to simply hide any unhandled rejections which has been a constant source of frustration for newcomers and for larger applications alike (see #87 and dozens of referenced tickets). The updated implementation provides a better default behavior that is more in line with how exceptions in PHP are also reported by default and significantly improves error reporting.

Any unhandled promise rejections will currently always be written to PHP's default error log location (STDERR for cli, Apache error log, etc.). Once this PR is merged, I've prepared a follow-up PR to provide a custom log handler function to write to custom log locations similar to how set_exception_handler() and set_error_handler() work.

I've also tested this branch against all ReactPHP components and dozens of downstream dependencies. Most projects do indeed execute just fine these changes. However, it is quite common for unhandled promise rejections to occur in their test suites. I've prepared a number of PRs for downstream components that I would file in the next couple of days to address this.

This PR comes with a sophisticated test suite and does not otherwise affect the existing behavior as verified by the existing test suite, so this should be safe to apply. Combined, you're looking a several days worth of work, enjoy! 🎉

Resolves / closes #87
Supersedes / closes #170
Supersedes / closes #222
Builds on top of #246, #240 and others

Copy link
Member

@SimonFrings SimonFrings left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one 👍

Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! 👍

@WyriHaximus WyriHaximus merged commit d87b562 into reactphp:3.x Jul 7, 2023
@clue clue deleted the report-unhandled branch July 7, 2023 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detecting unhandled rejections
3 participants