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

Trigger E_USER_ERROR on unhandled exceptions #170

Conversation

WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented Jun 28, 2020

Largely based on @clue's work at clue-labs@0feb517 to trigger an error when a rejected promise hasn't been handled.

Closes #87

$message .= ' raised in ' . $this->reason->getFile() . ' on line ' . $this->reason->getLine();
$message .= PHP_EOL . $this->reason->getTraceAsString() . PHP_EOL;

fatalError($message);
Copy link
Member

Choose a reason for hiding this comment

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

Rejections in done() are also unhandled rejections and should probably use the same error message format.

return fatalError($exception);

https://github.com/reactphp/promise/blob/master/src/Internal/RejectedPromise.php#L45

https://github.com/reactphp/promise/blob/master/src/Internal/RejectedPromise.php#L55

Maybe we can rename fatalError() to something like unhandledRejectionError() and generate the message there.

Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

@WyriHaximus Thanks for looking into this again! I agree that any unhandled exceptions should cause the program to report a hard error and then die().

This is definitely a BC break, so this PR only targets the upcoming Promise v3 release branch. Once we can come up with decent solution, we should look into providing a better upgrade path for the Promise v2 release branch, e.g. printing a soft warning possibly with an option to turn this off etc.

Note that the error handler passed to set_error_handler() may silently discard the error and not die() internally. It's my understanding we should explicitly die() in this case:

https://3v4l.org/9J9TB

I also don't feel the global error handler is the best place to deal with unhandled rejections. I think the global exception handler might be a better place, but it looks like there's no (sane) way to explicitly call this handler. Here are some of my experiments:

https://3v4l.org/ktOfm

@WyriHaximus
Copy link
Member Author

@clue updated this PR to match your latest experiment /cc @jsor

@WyriHaximus
Copy link
Member Author

As discussed with @clue, adding a test case to spawn a child process, force it to have an unhandled rejected promise and expect a 255 exit code.

@WyriHaximus
Copy link
Member Author

@clue @jsor test added

@WyriHaximus
Copy link
Member Author

Had a long chat with @clue about this today. And we came to the conclusion that killing off the process in 3.0 is to soon. It would mean a hard migration, and while we will have our own projects ready by then, the ecosystem might be caught off guard by it.

What we want to do instead, is still be in your face annoying about this but not kill the process. We're still figuring out the best way for that as we can't use the exception handler for this, and we want to provide users with the full context plus a way to catch any of these unhandled errors. This possibly comes down to providing a singleton accepting a handler.

Comment on lines +319 to +329
// set dummy handler and return previous handler
$handler = set_exception_handler(static function (Throwable $throwable) {
throw $throwable;
});

// use dummy handler when no original handler is available
if ($handler === null) {
$handler = set_exception_handler(null);
}

set_exception_handler(null);
Copy link

@Seldaek Seldaek Jan 4, 2022

Choose a reason for hiding this comment

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

IMO you need to restore the original handler there, not wipe it.

Suggested change
// set dummy handler and return previous handler
$handler = set_exception_handler(static function (Throwable $throwable) {
throw $throwable;
});
// use dummy handler when no original handler is available
if ($handler === null) {
$handler = set_exception_handler(null);
}
set_exception_handler(null);
// set dummy handler and return previous handler
$handler = set_exception_handler(static function (Throwable $throwable) {
throw $throwable;
});
// use dummy handler when no original handler is available
if ($handler === null) {
$handler = set_exception_handler(null);
} else {
// restore original handler
set_exception_handler($handler);
}

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
4 participants