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

Add new set_rejection_handler() function for unhandled rejections #249

Merged
merged 1 commit into from
Jul 11, 2023

Conversation

clue
Copy link
Member

@clue clue commented Jul 10, 2023

This changeset adds a new set_rejection_handler() function to set the global rejection handler for unhandled promise rejections.

The default promise rejection handler will log an error message plus its stack trace. The new set_rejection_handler() function may be used to use customize the log message or write to custom log targets.

Builds on top of #248
Refs #87

@clue clue added this to the v3.0.0 milestone Jul 10, 2023
@clue clue requested review from WyriHaximus and SimonFrings July 10, 2023 18:24
@clue clue changed the title Add new set_rejection_handler() function Add new set_rejection_handler() function for unhandled rejections Jul 10, 2023
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 👍

@WyriHaximus WyriHaximus merged commit af5814c into reactphp:3.x Jul 11, 2023
@clue clue deleted the set-rejection-handler branch July 11, 2023 10:00
public function __destruct()
{
if ($this->handled) {
return;
}

$message = 'Unhandled promise rejection with ' . \get_class($this->reason) . ': ' . $this->reason->getMessage() . ' in ' . $this->reason->getFile() . ':' . $this->reason->getLine() . PHP_EOL;
$message .= 'Stack trace:' . PHP_EOL . $this->reason->getTraceAsString();
$handler = set_rejection_handler(null);
Copy link

Choose a reason for hiding this comment

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

Hello.
Why did you make this handler one-time use? In this case, it's hard to even guess on which promise it will be used.

Copy link
Member

Choose a reason for hiding this comment

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

Hi, it's one time use because this is inside the promise that is not handled when rejected. It will only ever run once. It might run again for another promise, but not this specific one.

Copy link

Choose a reason for hiding this comment

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

But there is no connection between the handler and the promise here. I can't explicitly specify which promise this handler is for. And if the user completes another promise without handling the exception, they will not be notified about it, while the promise for which I would like to define a handler will complete without a handler.

Are you considering the possibility of adding a function to set a global persistent handler for all promises? Or add the ability to specify a chain of promises for the handler?

Copy link
Member

Choose a reason for hiding this comment

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

Hey @roxblnfk, took a closer look at this with @clue and it seems like following exceptions are currently not being handled correctly. We'll look into this, create some additional tests for multiple exceptions to define our expected behavior and implement a fix.

Thanks for taking the time and bringing this up 👍

Copy link
Member

Choose a reason for hiding this comment

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

Ok I think I misunderstood your initial comment. You are right, this shouldn't be a one use only handler but a persistent one instead. We're looking into adding additional tests and a fix for this.

Copy link

Choose a reason for hiding this comment

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

Thanks.

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.

4 participants