Skip to content

[CS2] CLI: Propagate SIGINT and SIGTERM signals when node is forked #4625

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

Merged
merged 2 commits into from
Aug 3, 2017

Conversation

Gobie
Copy link
Contributor

@Gobie Gobie commented Jul 28, 2017

When running commands like coffee --nodejs --max_old_space_size=128 index.coffee the coffee process doesn't forward signals to node process, which was already discussed in #4185.

Problem is that tools that use flow SIGTERM, delay, SIGKILL leave zombie processes.

This fixes #4185.

@GeoffreyBooth
Copy link
Collaborator

Please retarget this against 2.

@@ -444,6 +444,9 @@ forkNode = ->
cwd: process.cwd()
env: process.env
stdio: [0, 1, 2]
['SIGINT', 'SIGTERM'].forEach (signal) ->
Copy link
Collaborator

@GeoffreyBooth GeoffreyBooth Jul 28, 2017

Choose a reason for hiding this comment

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

Our preferred style is to use for…in:

for signal in ['SIGINT', 'SIGTERM']

This compiles into a traditional for loop, without a new function or scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought so looking at the surrounding code but in this case the new scope is necessary for the code to work properly.

Anyway I retargeted against 2 and used for...in with do for the scope.

@Gobie Gobie force-pushed the patch/propagate-signals branch from 76b1132 to 967c860 Compare July 28, 2017 23:14
@Gobie Gobie changed the base branch from master to 2 July 28, 2017 23:15
@GeoffreyBooth
Copy link
Collaborator

Thanks. @lydell or @connec?

@GeoffreyBooth GeoffreyBooth changed the title Propagate SIGINT and SIGTERM signals when node is forked [CS2] CLI: Propagate SIGINT and SIGTERM signals when node is forked Jul 28, 2017
@GeoffreyBooth GeoffreyBooth merged commit 3db60c0 into jashkenas:2 Aug 3, 2017
GeoffreyBooth pushed a commit to GeoffreyBooth/coffeescript that referenced this pull request Aug 3, 2017
…ashkenas#4625)

* Propagate SIGINT and SIGTERM signals when node is forked

* Use for loop for consistency
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.

2 participants