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

Remove process.exit(1) from babel-node #9758

Merged
merged 1 commit into from Sep 6, 2019
Merged

Remove process.exit(1) from babel-node #9758

merged 1 commit into from Sep 6, 2019

Conversation

dword-design
Copy link
Contributor

Because it breaked graceful shutdown of the sub process. For example, if you have an express server, catch the SIGINT signal and process.exit(0) after closing the server, the command would always return exit code 1. This PR allows the subprocess to decide how to exit itself.

Because it breaked graceful shutdown of the sub process.
@babel-bot
Copy link
Collaborator

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10596/

1 similar comment
@babel-bot
Copy link
Collaborator

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10596/

@mgreenw
Copy link

mgreenw commented Apr 29, 2019

This makes sense 👍I'm still worried about the functionality here because I don't think this change fixes #8526 where the SIGINT signal gets sent to the child process twice. Should line 106 be removed entirely such that babel-node (the parent process) does not even handle SIGINT at all?

@nicolo-ribaudo nicolo-ribaudo added area: node pkg: cli PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Apr 30, 2019
@nicolo-ribaudo
Copy link
Member

Should line 106 be removed entirely such that babel-node (the parent process) does not even handle SIGINT at all?

Probably yes, but I'm not an expert on this topic.
I think that currently we are doing this:

  1. Spawn babel-node
    1.i Spawn user's program
    1.ii Ctrl+C
    1.iii SIGINT
  2. SIGINT (it bubbles to the parent process?)
    2.i Forward SIGINT to the child process
  3. process.exit(1)

@mgreenw
Copy link

mgreenw commented May 6, 2019

I'm not exactly sure what would happen if the SIGINT for some reason never got sent to the child process. Because the parent is dead, wouldn't the child get reaped at some point?

I think the initial SIGINT from the Ctrl-C should be plenty to alert the child that it should die, and the parent re-sending the signal (2.i in @nicolo-ribaudo's chart) does not make sense and should be removed.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

I think that this PR is ok as-is.

I did some experiments around processes management at https://gist.github.com/nicolo-ribaudo/e6dd1806e184aea49a824d1dea2d0001, which simluates what this PR does.
You can run node main.js and kill one of the processes by running kill -SIGINT 12345 (replace 12345 with the correct PID).

If I kill the child process (which is what running Ctrl+C in @babel/node does), I get this output:

Main process is pid 23024
Child process is pid 23031
Child process killed
Child process is exiting...
Child process exited. code: 0, signal: null
Main process is exiting...

If I kill the main process, I get this output:

Main process is pid 23183
Child process is pid 23190
Main process killed
Killing child process...
Child process killed
Child process is exiting...
Child process exited. code: 0, signal: null
Main process is exiting...

As you can see, the child process doesn't receive SIGINT twice (so this PR also fixes #8526).

You can change EXITCODE in child.js to check that the exit code of the child process is not changed by. the main process.

If you comment lines 24-25 of main.js (#9758 (comment)) and kill the main process, the child process will stay alive. On the other hand, even with those lines the child process won't receive SIGINT twice (#8526).

@mgreenw
Copy link

mgreenw commented May 13, 2019

@nicolo-ribaudo Thanks for doing that testing! It looks like this kills two birds with one stone.

Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

Thanks

@JLHwung JLHwung merged commit fc8e142 into babel:master Sep 6, 2019
dword-design added a commit to dword-design/babel that referenced this pull request Nov 15, 2019
Because it breaked graceful shutdown of the sub process.
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Dec 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: node outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: cli PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants