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

worker: fix exit code for error thrown in uncaughtException handler #38012

Closed

Conversation

Linkgoron
Copy link
Member

@Linkgoron Linkgoron commented Mar 31, 2021

When an uncaughtException handler itself throws in a worker, the worker exits with an error code of 0 instead of 7 1, which happens because the worker thread global handler catches the error, and exitCode stays 0.

My fix only emits exit for "regular" unhandled exceptions (which shouldn't actually reach that code anyway, as it should set _exiting correctly in the "inner" handler) to emulate the same behaviour that happens in non-workers, where exit is not emitted when an error is thrown from a handler.

The process._exiting logic was essentially taken from here: https://github.com/nodejs/node/blob/master/lib/internal/process/execution.js#L167

Fixes: #37996

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 31, 2021
@Linkgoron Linkgoron changed the title worker: fix exit code for error thrown in unhandled handler worker: fix exit code for error thrown in uncaughtException handler Mar 31, 2021
Change worker exit code when the unhandled exception
handler throws from 0 to 7

fixes: nodejs#37996
@Linkgoron Linkgoron force-pushed the worker-throw-in-unhandled-exception branch from ddcff99 to 7cf8425 Compare March 31, 2021 23:04
@nodejs-github-bot

This comment has been minimized.

@Prinzhorn
Copy link

Thanks for fixing this so quickly. I think the docs need an update as well if we're going with 7 and not 1

If the worker was terminated, the exitCode parameter is 1.

https://nodejs.org/api/worker_threads.html#worker_threads_event_exit

@Linkgoron
Copy link
Member Author

I've changed the exit code to 1 to conform with the documentation.

@nodejs-github-bot

This comment has been minimized.

@Linkgoron Linkgoron force-pushed the worker-throw-in-unhandled-exception branch from 287f22e to 7779d71 Compare April 1, 2021 11:23
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@Linkgoron Linkgoron added the worker Issues and PRs related to Worker support. label Apr 3, 2021
@Linkgoron
Copy link
Member Author

@nodejs/workers

@nodejs-github-bot

This comment has been minimized.

@Linkgoron Linkgoron added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 5, 2021
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 6, 2021

jasnell pushed a commit that referenced this pull request Apr 6, 2021
Change worker exit code when the unhandled exception
handler throws from 0 to 7

fixes: #37996

PR-URL: #38012
Fixes: #37996
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Apr 6, 2021

Landed in 6986fa0

@jasnell jasnell closed this Apr 6, 2021
@Prinzhorn
Copy link

thanks!

Landed in 6986fa0

The 7 slipped in there again "from 0 to 7"

@Linkgoron
Copy link
Member Author

Linkgoron commented Apr 7, 2021

thanks!

Landed in 6986fa0

The 7 slipped in there again "from 0 to 7"

Yeah, it looks like I missed amending the commit message. The PR is good though, and correctly exits with 1.

targos pushed a commit that referenced this pull request May 1, 2021
Change worker exit code when the unhandled exception
handler throws from 0 to 7

fixes: #37996

PR-URL: #38012
Fixes: #37996
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Worker exits with code 0 (and not 7) when throwing inside uncaughtException
5 participants