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

Shell: improve handling of unhandled promise rejection #1758

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

andreabergia
Copy link
Contributor

We have changed the logging to log all the exceptions, rather than just the first one. Also, we quit the process with non-zero status if we are evaluating a file (i.e. not in REPL mode) and there are any unhandled promise rejections.

Example: create a file named promise.js:

Promise.reject('some error');
Promise.reject('another error');

If you launch this in both node and rhino, you will see that node exits with status 1, while rhino (before this PR) with status 0. With this change, rhino too exits with a non-zero status.

We have changed the logging to log _all_ the exceptions, rather than
just the first one. Also, we quit the process with non-zero status
if we are evaluating a file (i.e. not in REPL mode) and there are any
unhandled promise rejections.

Co-authored-by: Satish Srinivasan <[email protected]>
Copy link
Collaborator

@gbrail gbrail left a comment

Choose a reason for hiding this comment

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

This seems fine with me. Thanks!

@gbrail
Copy link
Collaborator

gbrail commented Dec 17, 2024

My only question is whether we think that someone will be broken by this change in a serious enough way to require us to add a command-line switch to not terminate on unhandled promise rejections.

@andreabergia
Copy link
Contributor Author

Honestly, rhino doesn't provide any API that returns a promise, such as fetch or similar. So I would guess the probability is on the low side. My preference would be to enable this behavior by default and, if someone complains, consider adding a switch to have the old behavior.

@gbrail
Copy link
Collaborator

gbrail commented Dec 17, 2024

OK -- I'm ok with this then. Thanks!

@gbrail gbrail merged commit 63b6206 into mozilla:master Dec 17, 2024
3 checks passed
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.

2 participants