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

fix: handle unexpected errors #2368

Merged
3 commits merged into from
Nov 29, 2021
Merged

fix: handle unexpected errors #2368

3 commits merged into from
Nov 29, 2021

Conversation

ghost
Copy link

@ghost ghost commented Nov 16, 2021

Ensure unexpected errors are handled with Exit Code 2 rather than the NodeJS default 1. We use 1 for vulns found.

Node 16 now uses Exit Code 1 for unhandledRejections (rather than only logging), so this also makes us consistent across different Node versions.

https://nodejs.org/en/blog/release/v15.0.0/#throw-on-unhandled-rejections-33021

Note for Reviewers

When reviewing, it's best to review through each commit.

I've moved a file and placed a new file in its old place. To avoid losing history, these two steps are in separate commits.

If I didn't do this, git will squash the two changes to look like I deleted a bunch of code and created a new file with a bunch of new code. Obviously, that's not correct. This is what GitHub's PR interface also shows, but it's not how it looks in history with separate commits.

Notes

  • Add a test for it. Probably can't test via CLI acceptance tests or unit tests as it requires "unexpected" errors and the process needs to be exiting. Probably can write some sort of separate acceptance test which uses the same helper function.

@ghost ghost marked this pull request as ready for review November 16, 2021 13:49
@ghost ghost requested review from a team as code owners November 16, 2021 13:49
@ghost ghost requested review from almog27, JCheung2004 and Jdunsby November 16, 2021 13:49
Copy link
Contributor

@maxjeffos maxjeffos left a comment

Choose a reason for hiding this comment

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

Great change

@ghost ghost mentioned this pull request Nov 16, 2021
@ghost ghost force-pushed the feat/handle-unexpected-errors branch 6 times, most recently from 713d244 to 738efe5 Compare November 19, 2021 20:40
@ghost ghost mentioned this pull request Nov 25, 2021
4 tasks
Copy link
Author

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Self-review based on in-person discussions.

* This function can only be used once in the same process. If you have multiple
* callables needing this, compose them into a single callable.
*/
export async function callHandlingUnexpectedErrors(
Copy link
Author

@ghost ghost Nov 25, 2021

Choose a reason for hiding this comment

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

@maxjeffos how about "safelyCall". Seems like a more minimal solution (vs. inventing an understandable DSL). Might be too generic...

Jahed Ahmed added 3 commits November 25, 2021 20:54
Ensure unexpected errors are handled with Exit Code 2 rather than the NodeJS default 1. We use 1 for vulns found.

Node 16 now uses Exit Code 1 for unhandledRejections (rather than only logging), so this also makes us consistent across different Node versions.
Need to wrap index logic in an error
handler but doing so will lose file history. This commit will be followed by a new index.js
@ghost ghost force-pushed the feat/handle-unexpected-errors branch from 738efe5 to a8835ab Compare November 25, 2021 20:55
@ghost
Copy link

ghost commented Nov 25, 2021

This PR modifies files linked to issues tracked in Stepsize.

You might want to review their status, priority, and scope.

 Mention [stepsize] in a comment if you'd like to report some technical debt. See examples here.

@ghost ghost merged commit 9dafe39 into master Nov 29, 2021
@ghost ghost deleted the feat/handle-unexpected-errors branch November 29, 2021 11:42
This pull request was closed.
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.

1 participant