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

forward signals correctly (was: signals: ignore them) #419

Merged
merged 3 commits into from
Dec 10, 2017

Conversation

stelcheck
Copy link
Contributor

signals are passed to the spawned child process; this means
that the main process should not react to them, and instead
wait for the spawned process to exit as a result of receiving
the forwarded signal.

@coveralls
Copy link

coveralls commented Sep 7, 2017

Coverage Status

Coverage remained the same at 78.1% when pulling 79142b4 on stelcheck:fixes/signals into 2ee9575 on TypeStrong:master.

@blakeembrey
Copy link
Member

Can you share the situation that this solves? I've never come across this being an issue myself, nor have I seen it handled in other libraries that spawn child processes (https://github.com/babel/babel/blob/777a9ae6e42db1c9d57bbbdca8b233c2dc9b89ac/packages/babel-node/src/babel-node.js).

@stelcheck
Copy link
Contributor Author

stelcheck commented Sep 8, 2017

Given the following code:

process.on('SIGINT', () => {
  console.log('received sigint')
  setTimeout(() => {
    console.log('exit')
    process.exit(0)
  }, 2000)
})

console.log('running')
require('http').createServer(() => true).listen(8080)

Running directly in node:

$ node index.ts   
running
^Creceived sigint
exit
$

While in ts-node

ts-node index.ts
running
^Creceived sigint

$ exit

As you can see, with ts-node, the exit log is shown after the process has been terminated. This is because the ts-node process need only to forward the signals, not react to them.

npm also suffered from the same problem until 4.5.0, for what it is worth: npm/npm@878aceb

@blakeembrey
Copy link
Member

Awesome, thanks. Do you think we get add a test case so this doesn't occur in the future?

@stelcheck
Copy link
Contributor Author

I wrote a quick sample test, but for some reason, the child process does not get killed at all. Will look into it a bit further.

@coveralls
Copy link

coveralls commented Sep 8, 2017

Coverage Status

Coverage decreased (-32.2%) to 45.876% when pulling d3e5eb2 on stelcheck:fixes/signals into 2ee9575 on TypeStrong:master.

@coveralls
Copy link

coveralls commented Sep 8, 2017

Coverage Status

Coverage decreased (-32.5%) to 45.596% when pulling 98ae8b8 on stelcheck:fixes/signals into 2ee9575 on TypeStrong:master.

@coveralls
Copy link

coveralls commented Sep 8, 2017

Coverage Status

Coverage decreased (-32.6%) to 45.455% when pulling b91ccb8 on stelcheck:fixes/signals into 2ee9575 on TypeStrong:master.

return done()
})

setTimeout(() => proc.kill('SIGTERM'), 100)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reasons, the kill signal is not properly sent.

Copy link
Member

Choose a reason for hiding this comment

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

Would it work if you forwarded the signals to the child? It's possible that the current code change is just ignoring the signal.

@stelcheck
Copy link
Contributor Author

The point of this test is to ensure that the signal is properly caught by the binary, not by the child.

I tried to use 'SIGKILL', and that didn't work either, so maybe Mocha is getting in the way...

@coveralls
Copy link

coveralls commented Sep 9, 2017

Coverage Status

Coverage decreased (-32.5%) to 45.596% when pulling 7a6b94a on stelcheck:fixes/signals into 2ee9575 on TypeStrong:master.

@stelcheck stelcheck changed the title signals: ignore them forward signals correctly (was: signals: ignore them) Sep 9, 2017
@stelcheck
Copy link
Contributor Author

Found the issue. In a nutshell, signals triggered by keyboard interrupts are sent to the process and all subprocesses at once, except if the suprocess was spawned in detached mode.

The solution is to detach the subprocess and manually forward relevant signals. I have found three so far:

  • SIGINT
  • SIGTERM
  • SIGWINCH (for cases where the terminal size is changed)

Will update the PR shortly, but have already updated its title. I am trying to add a test to verify that keyboard interrupts have the same behavior as sending a signal to the top process first to make sure behavior always remain consistent.

@stelcheck
Copy link
Contributor Author

I gave up on testing specifically for keyboard interrupts. If you know of a way to test for that and would like me to do so, please let me know how I could set up the test.

@coveralls
Copy link

coveralls commented Sep 9, 2017

Coverage Status

Coverage decreased (-32.4%) to 45.736% when pulling d84900b on stelcheck:fixes/signals into 2ee9575 on TypeStrong:master.

@stelcheck stelcheck force-pushed the fixes/signals branch 2 times, most recently from 21bb4bc to 77b44d8 Compare September 9, 2017 13:25
@coveralls
Copy link

coveralls commented Sep 9, 2017

Coverage Status

Coverage decreased (-32.4%) to 45.736% when pulling 21bb4bc on stelcheck:fixes/signals into 2ee9575 on TypeStrong:master.

@coveralls
Copy link

coveralls commented Sep 9, 2017

Coverage Status

Coverage decreased (-32.4%) to 45.736% when pulling 77b44d8 on stelcheck:fixes/signals into 2ee9575 on TypeStrong:master.

@stelcheck
Copy link
Contributor Author

This passes just fine on my desktop (Linux), so I am not sure as to why it would fail on CI. Apologies for the spam.

@coveralls
Copy link

coveralls commented Sep 9, 2017

Coverage Status

Coverage increased (+0.6%) to 78.718% when pulling 88bac37 on stelcheck:fixes/signals into 2ee9575 on TypeStrong:master.

@stelcheck
Copy link
Contributor Author

@stelcheck
Copy link
Contributor Author

All tests are now passing. Please let me know if further changes are warranted before this can be merged.

src/bin.ts Outdated
proc.on('close', (code: number, signal: string) => {
if (signal) {
process.kill(process.pid, signal)
} else if (code) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to check the else if here? If it closes, shouldn't it have one or the other? In a hypothetical case it has neither, wouldn't we still want to exit?

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 think we want the process to exit naturally (no process.exit) upon a signal, but to exit with a code when there is one.

src/bin.ts Outdated
})
// Ignore signals, and instead forward them to the
// child process
const forward = (signal: string) => process.on(signal, () => proc.kill(signal))
Copy link
Member

Choose a reason for hiding this comment

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

Probably easier just to make this an array loop. The comments are super verbose and mostly unnecessary too. E.g.

['SIGINT', 'SIGTERM', 'SIGWINCH'].forEach(signal => process.on(signal, () => proc.kill(signal))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH I never remember what SIGWINCH does, hence why I wrote it in a slightly more verbose way. If you consider this a blocker, I will change it, of course.

@blakeembrey
Copy link
Member

blakeembrey commented Oct 11, 2017

Are you absolutely sure we need to be running in detached mode? Even NPM doesn't run in detached mode and based on https://github.com/npm/lifecycle/blob/latest/index.js they just forward the two signals specifically.

Edit: This was based on a reference to NPM I can no longer see in the issue, sorry if I'm mixing the issues up 😄

@stelcheck
Copy link
Contributor Author

#419 (comment) Gives the explanation. We forward terminal size change as well since programs may be long-running and meant to be executed within a terminal (e.g a REPL interface, or something like htop)

@narthur157
Copy link

Would like to see this PR go in, currently this kills my workflow because I have a zombie proc every time I Ctrl+C out of ts-node, leaving the port binded

@stelcheck
Copy link
Contributor Author

@narthur157 I take it you tested this PR against your use case? If not, could you give it a spin to confirm it would solve your problem properly?

@endel
Copy link

endel commented Nov 5, 2017

I've just sent a simpler alternative to the same problem here: #458

src/bin.ts Outdated
})
// Ignore signals, and instead forward them to the
// child process
const forward = (signal: string) => process.on(signal, () => proc.kill(signal))
Copy link

Choose a reason for hiding this comment

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

On the latest version of NodeJS type definitions, signal should be of type NodeJS.Signals instead of string. (it's not compiling)

src/bin.ts Outdated

// If this process is exited with any other signals,
// kill the subprocess with the same signal
process.on('exit', (_code: number, signal: string) => proc.kill(signal))
Copy link

@endel endel Nov 11, 2017

Choose a reason for hiding this comment

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

On the latest version of NodeJS type definitions, the "exit" callback only accepts one argument. (it's not compiling)

signals are passed to the spawned child process; this means
that the main process should not react to them, and instead
wait for the spawned process to exit as a result of receiving
the forwarded signal.
@coveralls
Copy link

coveralls commented Nov 22, 2017

Coverage Status

Coverage increased (+0.8%) to 77.616% when pulling 31d5a34 on stelcheck:fixes/signals into c57ded2 on TypeStrong:master.

@stelcheck
Copy link
Contributor Author

@endel fixed the two issues you reported.

What do we need to do to get this in? I'd like to do everything I can ASAP to get this merged (and hopefully released in a timely fashion), so if there is anything else I would need to do or any other concerns I would need to research or address, please let me know.

@stelcheck
Copy link
Contributor Author

Apologies for insisting, but is there anything that needs to be done to get this merged?

@coveralls
Copy link

coveralls commented Dec 10, 2017

Coverage Status

Coverage increased (+0.6%) to 77.427% when pulling 986ffa8 on stelcheck:fixes/signals into c57ded2 on TypeStrong:master.

@blakeembrey blakeembrey merged commit be7895c into TypeStrong:master Dec 10, 2017
@stelcheck
Copy link
Contributor Author

Thank you! Let me know if I can ever be of any further help.

@endel
Copy link

endel commented Jan 20, 2018

@stelcheck @blakeembrey It seems the problem still persists when using child processes (cluster module). Here's an issue and example project demonstrating the problem: #519

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.

5 participants