Inquirer.js icon indicating copy to clipboard operation
Inquirer.js copied to clipboard

Have to send two SIGINT to kill when prompt is open

Open mbleigh opened this issue 8 years ago • 20 comments

If you Ctrl-C out of a command while an inquirer prompt is open, it just adds a couple lines of space and then you have to Ctrl-C again to actually exit. I'd expect the calling program to be the one controlling what happens on SIGINT here.

Is this working as intended? If so, is there a way to be notified when Inquirer has trapped a SIGINT?

mbleigh avatar Oct 02 '15 18:10 mbleigh

Inquirer shouldn't trap SIGINT. What's your OS/terminal/node version?

SBoudrias avatar Oct 02 '15 18:10 SBoudrias

Interesting...I tried to create a minimal repro but couldn't reproduce the behavior. I thought I had seen the behavior in bower init but now can't reproduce that either. Feel free to mark as closed, I must be mistaken about the cause of my issue.

mbleigh avatar Oct 02 '15 18:10 mbleigh

I ran into this issue recently. Because of Inquirer.js's usage of readline, SIGINT only gets sent to Inquirer, which is fine when Inquirer is in the only running process.

For me, the problem came up when I had a child process running. Since Inquirer does not propagate the signal, it is never exposed to the main process. This essentially removes any ability to listen for any signals with process.on.

The real question is, should SIGINT only go to Inquirer, and be required a second time for the process, or should Inquirer be responsible and ensure that SIGINT is sent properly?

ChristianAlexander avatar Nov 24 '15 19:11 ChristianAlexander

@ChristianAlexander can you link to the Node documentation stating that readlines won't propagate signals to the process?

SBoudrias avatar Nov 24 '15 19:11 SBoudrias

After looking into this further, there is no documentation explicitly stating that signals do not propagate. In fact, manually sending a SIGINT (ex: kill -2 <node process id>) does work.

The problem is that readline itself listens to the TTY directly, so a 'real' SIGINT isn't produced. See: https://github.com/nodejs/node/blob/master/lib/readline.js#L689-L700

ChristianAlexander avatar Nov 24 '15 20:11 ChristianAlexander

@ChristianAlexander mind opening an issue on Node and linking this issue?

SBoudrias avatar Nov 24 '15 20:11 SBoudrias

I'm unsure if this should be considered an issue at the Node level. Perhaps there could be a case where it wouldn't be desirable to send a SIGINT on ctrl+c in a project directly dependent on readline.

However, I can not come up with a scenario where any Inquirer-based projects would use ctrl+c. Even if such a scenario were to exist, the event wouldn't be exposed to consumers of Inquirer.

If Inquirer doesn't end up sending SIGINTs to the process itself, would it be possible to have Inquirer pass the event along after it is done closing up? If not, could Inquirer send an error object in its callback where results are sent?

ChristianAlexander avatar Nov 24 '15 21:11 ChristianAlexander

I'm unsure if this should be considered an issue at the Node level. Perhaps there could be a case where it wouldn't be desirable to send a SIGINT on ctrl+c in a project directly dependent on readline.

I don't know, but that seems like a conversation worth having with the Node community.

If Inquirer doesn't end up sending SIGINTs to the process itself, would it be possible to have Inquirer pass the event along after it is done closing up? If not, could Inquirer send an error object in its callback where results are sent?

Feel free to send a PR.

SBoudrias avatar Nov 24 '15 21:11 SBoudrias

So... I want to capture the ctrl+c to, let's say, output a "good bye" before exiting, killing a socket, and other small things like logs. But I also want to listen to ctrl+l to clear the console, and if possible, esc! If inquirer does receive these events somehow, it could have an API so we could have access to them.

felipenmoura avatar Jan 16 '16 01:01 felipenmoura

@felipenmoura these events are global, you don't need to listen to it on inquirer.

SBoudrias avatar Jan 16 '16 01:01 SBoudrias

Well, they are not being triggered! I am using inquirer, with wait, and also using the process.on('SIGINT', ... ), and it is never called! I am not sure if I am doing something wrong, or if it has something to do with this thread.

felipenmoura avatar Jan 17 '16 00:01 felipenmoura

@felipenmoura that's what this thread is saying. That's due to the Node.js readline. You guys should open an issue on the Node project.

SBoudrias avatar Jan 17 '16 01:01 SBoudrias

What about some kind of solution like exposing the readline instance as a workaround?

See this comment from a Node.js Foundation Member: https://github.com/nodejs/node/issues/4758#issuecomment-231155557

dcrockwell avatar Jul 07 '16 17:07 dcrockwell

Does anyone have any new insights or workarounds for this issue? It's still present one year on and quite annoying that one has to CTRL-C twice to exit a process where inquirer prompt is active.

Looks like the suggestion as above by @dcrockwell is already present in the code but to no avail:

https://github.com/SBoudrias/Inquirer.js/blob/master/lib/ui/baseUI.js#L21

Further investigation shows that the called onClose handler doesn't really do anything except output a new line:

UI.prototype.onForceClose = function () {
  this.close();
  console.log('');
};

I'll create a PR to fix this.

adamreisnz avatar Jul 09 '17 02:07 adamreisnz

I disagree with the recently merged process.exit(0) solution to this issue.

The application making use of inquirer may wish to do unrelated cleanup via process.on('SIGINT', ...) before exiting normally. Conversely, the application may be running in a shell script where a normal exit after Ctrl+C would incorrectly execute subsequent commands in the script. https://www.cons.org/cracauer/sigint.html explains this much better than I could.

The correct way to handle this is process.kill(process.pid, 'SIGINT'). If the application has a listener handling this then it can deal with it how it wants, if it's not listening then the signal is correctly propagated to other processes.

welwood08 avatar Jul 18 '17 03:07 welwood08

@welwood08 I can confirm that using process.kill(process.pid, 'SIGINT'); has the desired effect as well. I will create a PR for that. Thanks for pointing this out.

adamreisnz avatar Jul 18 '17 04:07 adamreisnz

I just tried with the version 6.x and my SIGINT handler never gets called unless I remove this line: process.kill(process.pid, 'SIGINT') from onForceClose function.. that is strange, is not supposed to forward the call to any listener? In my case it just kill the process, no SIGINT/exit handlers are called

b4dnewz avatar Jul 30 '18 09:07 b4dnewz

I also have the same problem in the 6.x version.I found the problem is in this.close() method in ui/baseUI.js.More specific is the combination of

this.rl.pause()
this.rl.close().

I also look the pause and close method in node readline.js.But I can not find the reason. I hope someone can help me to understand it. Thanks!

junfeisu avatar Aug 17 '18 08:08 junfeisu

EDIT: I guess this is more a comment on the issue that b4dnewz & junfeisu had, rather than the OP, in that I couldn't catch my SIGINT at all (I did not have to send it to my process twice). Still, I hope this is helpful for others that land here looking for signal handling issues

I thought I was hitting this issue with version 6.2, but it turned out not to be an issue with inquirer, it was more with the way signal handlers work in node.

Posting my solution here in case it's helpful for others:

My issue was, in short That when inquirer encounters a SIGINT, it closes the underlying readline interface, which I believe cancels any handlers for readline input. If the only thing your app is doing is waiting for the inquirer prompt result, that means that the event loop will no longer be considered "alive", so the process will exit with a zero exit code.

You might think that your signal handler would be ref'ed by the event loop, as any socket listener or file read request is, but that is not the case. So even though you may have set a process.on("SIGINT", ...), the process will actually exit before it gets a chance to run your signal handler.

The solution was To maintain a ref'ed handle in the event loop until I'm done listening for signals (or, equivalently in my case, until I'm done with my inquirer prompting).

My implementation of this solution

/**
 * This class is required for proper signal handling of
 * inquirer-based programs.
 *
 * USAGE:
 *
 *    async function myTask() {
 *      // Create a signal ref for the signal you'd like to catch
 *      const signalRef = new SignalRef(
 *        'SIGINT',
 *        () => { process.exit(1); }
 *      );
 *
 *      try {
 *        await doYourInquirerStuff();
 *      } finally {
 *        // Release the ref to avoid keeping the loop alive forever.
 *        signalRef.unref();
 *      }
 *    }
 *
 * NOTE: If you're not going to exit in your signal handler,
 * consider whether you want to catch the signal only once
 * (in which case maybe have it unref itself) or many times.
 *
 * See: https://github.com/SBoudrias/Inquirer.js/issues/293
 */

module.exports = class SignalRef {
    constructor(signal, handler) {
        this.signal = signal;
        this.handler = handler;

        process.on(this.signal, this.handler);

        // This will run a no-op function every 10 seconds.
        // This is to keep the event loop alive, since a
        // signal handler otherwise does _not_. This is the
        // fundamental difference between using `process.on`
        // directly and using a SignalRef instance.
        this.interval = setInterval(() => {}, 10000);
    }

    unref() {
        clearInterval(this.interval);
        process.removeListener(this.signal, this.handler);
    }
};

jbreeden-splunk avatar Sep 19 '18 17:09 jbreeden-splunk

@jbreeden-splunk thanks!

Is there a way to go back to the last prompt by inquirer after handling the signal? (ideally without bookkeeping from my end)

justin-calleja avatar Sep 21 '19 16:09 justin-calleja

Closing this ticket as stale. Open new issues if you encounter problem with the new Inquirer API.

SBoudrias avatar Jun 03 '23 21:06 SBoudrias