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

Understanding the behaviour of SIGINT #85

Open
rprieto opened this issue Dec 16, 2017 · 13 comments
Open

Understanding the behaviour of SIGINT #85

rprieto opened this issue Dec 16, 2017 · 13 comments

Comments

@rprieto
Copy link

rprieto commented Dec 16, 2017

I'm trying to understand how Listr responds to SIGINT, compared for example to async.series. Here's two simple examples where we see the different behaviour.

When running async.series

  • pressing SIGINT forwards the signal to any child processes (in this example: sleep)
  • the process terminates, and async.series moves on to the next one
  • if I keep pressing, eventually all tasks terminate
  • the event loop becomes empty, the beforeExit event is emitted, and Node terminates
Click here to expand the async.series sample code
const childProcess = require('child_process')
const async = require('async')

process.on('beforeExit', code => console.error('beforeExit', code))
process.on('exit', code => console.error('exit', code))
process.on('SIGINT', () => console.error('SIGINT'))

function sleep (done) {
const child = childProcess.spawn('sleep', ['5'])
child.on('exit', (code, signal) => {
console.log('sleep exit', code, signal)
done()
})
}

async.series([sleep, sleep, sleep], err => {
console.log('done', err)
})

When running Listr

  • pressing SIGINT seems to terminate the process forcefully
  • we don't see the message about the current sleep process exiting
  • the beforeExit event is not triggered
  • Node terminates with exit code 130
$ node test.js
 ⠦ Sleep
   Sleep
   Sleep
^C
SIGINT
exit 130
Click here to expand the Listr version
const childProcess = require('child_process')
const Listr = require('listr')

process.on('beforeExit', code => console.error('beforeExit', code))
process.on('exit', code => console.error('exit', code))
process.on('SIGINT', () => console.error('SIGINT'))

function sleep (done) {
const child = childProcess.spawn('sleep', ['5'])
child.on('exit', (code, signal) => {
console.log('sleep exit', code, signal)
done()
})
}

const tasks = new Listr([
{ title: 'Sleep', task: () => new Promise(sleep) },
{ title: 'Sleep', task: () => new Promise(sleep) },
{ title: 'Sleep', task: () => new Promise(sleep) }
])

tasks.run().then(ctx => {
console.log('done')
}).catch(err => {
console.log('done with errors', err)
})


Do you mind explaining how Listr handles this? I'd really like beforeExit to be emitted as normal so I can run some cleanup tasks.

@anteprimorac
Copy link

@SamVerschueren is there any other way to do cleanups?

@SamVerschueren
Copy link
Owner

I don't have anything special set up to handle SIGINT calls. So what you are asking is a way to send SIGINT to a subprocess?

@anteprimorac
Copy link

I think @rprieto explained the problem.

@rprieto
Copy link
Author

rprieto commented Mar 9, 2018

Thanks it's interesting to hear there's no special handling. The small code samples above are all that's needed to reproduce. I'll run some more tests to understand why Ctrl-C abruptly stops the Listr example, maybe it's to do with Promises?

@anteprimorac
Copy link

@rprieto when silent renderer is used then it’s working ok

@okonet
Copy link
Contributor

okonet commented Sep 8, 2018

I have similar problem where I'd like to run some cleanup code after terminating child processes that were run by Listr. I'm looking at https://github.com/Tapppi/async-exit-hook and try to integrate it but it seems not to affect rendering process of Listr which I'd like it to do.

@SamVerschueren do you think this can be added to Listr?

@okonet
Copy link
Contributor

okonet commented Sep 8, 2018

Weird, but https://github.com/Tapppi/async-exit-hook isn't working either with a promise when used together with Listr.

@okonet
Copy link
Contributor

okonet commented Sep 11, 2018

I can confirm it's working with silent renderer, too. So it seems to be related to how Listr renders. @SamVerschueren any pointers in this regard?

@okonet
Copy link
Contributor

okonet commented Sep 12, 2018

I tracked down the issue to sindresorhus/log-update#17 which kinda makes sense since both listr-update-renderer and listr-verbose-renderer are using cli-cursor as a dependency. But since it's closed, I'm not sure if this is still affect it or something else. Maybe @sindresorhus could help?

@rprieto
Copy link
Author

rprieto commented Sep 12, 2018

Thanks for getting to the bottom of this!! 👍 The update renderer uses [email protected] but the fix was released in [email protected]. Hopefully it's an easy upgrade.

I'm not sure about the verbose renderer. The bug was apparently fixed in [email protected] but the verbose renderer already uses 2.x.x. Is the bug definitely there in the verbose renderer?

@okonet
Copy link
Contributor

okonet commented Sep 12, 2018

Hmm, I’m not sure anymore. I can test it later today and let you know. I also noticed outdated dependency and I’ll do a PR with an update after I tested it.

okonet pushed a commit to okonet/listr-update-renderer that referenced this issue Sep 13, 2018
This version should fix issues related to processing of SIGINT signal. See sindresorhus/log-update#17. Related to SamVerschueren/listr#85
@okonet
Copy link
Contributor

okonet commented Sep 13, 2018

Just an update here: after upgrading log-update to latest version 2.3.0 I was able to add SIGINT handling to Listr! 🎊 PR for the listr-update-renderer is in place so let's hope @SamVerschueren can find some time to release the fix.

However this doesn't solve all edge cases in regard of termination, but at least this allows us to hook into and modify the behavior ourselves. One issue I'm still seeing is when using execa to run tasks. In this case the termination signal won't be passed to the child process. This leads to an issue where Listr never exits after pressing Ctrl+C. I hope I can work around this by manually doing this.

@okonet
Copy link
Contributor

okonet commented Sep 13, 2018

@rprieto

The bug was apparently fixed in [email protected] but the verbose renderer already uses 2.x.x. Is the bug definitely there in the verbose renderer?

I tested with verbose renderer and I still can see the bug. I'm not sure why, though.

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

4 participants