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

Kill child when exiting #51

Closed
dignifiedquire opened this issue Aug 2, 2016 · 9 comments
Closed

Kill child when exiting #51

dignifiedquire opened this issue Aug 2, 2016 · 9 comments

Comments

@dignifiedquire
Copy link
Contributor

If I use execa.shell to spawn a long running process, but the original process dies for some reason it would be great if the spawned process gets terminated automatically.

A small example

'use strict'

const execa = require('execa')

execa.shell('ipfs daemon')
  .then((res) => {
    console.log(res.stdout)
  })
  .catch((err) => {
    console.log('failed', err)
  })

console.log('starting')
setTimeout(() => {
  console.log('exiting')
  process.exit()
}, 10 * 1000)

Running this will print exiting after 10s and exit but when I grep for ipfs dameon the process is still running.

@sindresorhus
Copy link
Owner

This is how the native childProcess.exec() works too. It would indeed be useful, but I don't really see how it would be possible to know what processes are executed as it's the shell that spawns them, not Node.js. Thoughts? I would suggest you instead use execa('ipfs', ['daemon']) both for performance and security reasons.

@dignifiedquire
Copy link
Contributor Author

This is how the native childProcess.exec() works too

Yes, sadly :( That's why I thought this might be a good place to improve on it :)

but I don't really see how it would be possible to know what processes are executed

What I currently do is keep track of the processes I spawn and attach handlers to exit and SIGINT on process to clean them up immediately before node.js exits. I totally understand if you think this is out of scope for this library but I wanted to mention as I can imagine me not being the only person needing this.
It probably would make sense to have this behind an option though rather than enabling it by default.

I would suggest you instead use execa('ipfs', ['daemon']) both for performance and security reasons.

The thing is that ipfs is not a node.js script but rather a binary out of my control.

@sindresorhus
Copy link
Owner

execa('ipfs', ['daemon']) works with any binary, not just Node.js.

@dignifiedquire
Copy link
Contributor Author

execa('ipfs', ['daemon']) works with any binary, not just Node.js.

Right..silly me. But I encounter the same issue with that, the process is spawned but not terminated when the parent exits.

@sindresorhus
Copy link
Owner

sindresorhus commented Aug 2, 2016

Hmm. I assumed Node.js would clean up direct child processes when exiting. Sounds like something it should do. Can you try opening an issue on Node.js first? And comment the link here. Would be better to try to fix it for everyone, not just this module. If they decline, we can add it here.

@dignifiedquire
Copy link
Contributor Author

Sure, I will do that later tonight

@dignifiedquire
Copy link
Contributor Author

Just filed an issue with a minimal example: nodejs/node#7951

@dignifiedquire
Copy link
Contributor Author

So as you can see from the issue this is the expected unixy behaviour. I still think it would be a great addition to optionally destroy the spawned process in this module. To catch that as referenced in the issue we could use https://www.npmjs.com/package/signal-exit.

Let me know if you are interested and I can work on a PR for adding this tracking behaviour.

@sindresorhus
Copy link
Owner

I'm not surprised. Unexpected behavior usually is expected UNIX behavior...

Sure, PR welcome :)

dignifiedquire added a commit to dignifiedquire/execa that referenced this issue Aug 3, 2016
As spawned processes are not cleaned up when the main process dies
this option adds the abilitiy to kill those when the spawning process
dies.

Closes sindresorhus#51
dignifiedquire added a commit to dignifiedquire/execa that referenced this issue Aug 4, 2016
As spawned processes are not cleaned up when the main process dies
this option adds the abilitiy to kill those when the spawning process
dies.

Closes sindresorhus#51
dignifiedquire added a commit to dignifiedquire/execa that referenced this issue Aug 4, 2016
As spawned processes are not cleaned up when the main process dies
this option adds the abilitiy to kill those when the spawning process
dies.

Closes sindresorhus#51
dignifiedquire added a commit to dignifiedquire/execa that referenced this issue Aug 4, 2016
As spawned processes are not cleaned up when the main process dies
this option adds the abilitiy to kill those when the spawning process
dies.

Closes sindresorhus#51
sindresorhus pushed a commit that referenced this issue Aug 12, 2016
As spawned processes are not cleaned up when the main process dies
this option adds the abilitiy to kill those when the spawning process
dies.

Closes #51
sindresorhus pushed a commit that referenced this issue Aug 12, 2016
As spawned processes are not cleaned up when the main process dies
this option adds the abilitiy to kill those when the spawning process
dies.

Closes #51
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

No branches or pull requests

2 participants