breaks other process.on('SIGINT', ...) handlers

[stefanpenner]

When using ora, other exit related signal handlers fail to run:

// remove the next line, and "cleanup" prints.
require('ora')('Loading unicorns').start().stop();

process.on('SIGINT', function() {
  // never called if ora was ever enabled
  console.log('cleanup');
});

process.kill(process.pid, 'SIGINT');

This is due to: exit-hook used by restore-cursor not being friendly with other on-exit related strategies.

[sindresorhus]

Any ideas on how to resolve this? Maybe re-emit the event in exit-hook if there are other SIGINT handlers?

[stefanpenner]

@sindresorhus not 100% sure yet.

My current thought is (to make ember-cli resilient) to have something like:

var exit;
var handlers = [];
var Promsie = require('../ext/promise');

module.exports.captureExit = function() {
  exit = process.exit;

  process.exit = function(code) {
    var args = arguments;
    var work = handlers.splice(0, handlers.length);

    Promise.all(work.map(function(handler) {
      return handler.appy(null, args);
    })).finally(function() {
      exit.apply(process, args);
    });
  };
};

module.exports.onExit = function(cb) {
  handlers.push(cb);
};

module.exports.realExit = function() {
  exit.apply(process, arguments);
};

[stefanpenner]

if there are other SIGINT handlers?

can this be detected?

[sindresorhus]

process.listenerCount('SIGINT') > 0;

https://nodejs.org/api/events.html#events_emitter_listenercount_eventname

[stefanpenner]

@sindresorhus that may should do the trick

[stefanpenner]

@sindresorhus what are your thoughts, should I do ^ on my side regardless or..

[stefanpenner]

@sindresorhus that may should do the trick

Well that actually does not quite do the trick, although it may improve the situation and should be considered.

The scenario is as follows:

Will work 👍 :

require('ora')('Loading unicorns').start().stop();

process.on('SIGINT', function() {
  asyncCleanup().finally(function() {
    process.exit(code);
  });
});

Will exit prematurely 👎 :

process.on('SIGINT', function() {
  asyncCleanup().finally(function() {
    process.exit(code);
  });
});
require('ora')('Loading unicorns').start().stop();

The reason the last scenario is problematic, is that it now becomes a race: If the async cleanup code is added last, everything will appear to work and if it is added first it will fail to work.

---

Reasoning for async cleanup, is due to (at the very least) windows systems are often quite unhappy with a large number of synchronous rmdir and unlinks. Although I can imagine other legitimate scenarios where cleanup is inherently async.

[sindresorhus]

Ideally, it should handled by exit-hook.

To be clear, here's what I was thinking (works):

// `exit-hook`
process.on('SIGINT', () => {
    if (process.listenerCount('SIGINT') === 1) {
        console.log('exit code 2');
        process.exit(2);
    }
});

// your hook
process.on('SIGINT', () => {
    // your async cleanup code
    setTimeout(() => {
        console.log('own sigint');
        process.exit();
    }, 500);
});

setTimeout(() => {
    process.kill(process.pid, 'SIGINT');
}, 1000);

[stefanpenner]

@sindresorhus ah, the missing bit for me was: process.listenerCount('SIGINT') also includes already fired listeners (which in retrospec makes sense).

[stefanpenner]

there may be a further issue:

2 or more exit-hook modules (due to npm dupes) may result in deadlock, as they both assume the other will do the actual exit.

If this was a "core" module, or npm has some sort of highlander rule this would be ok. Alternatively, making exit-hook guard against being implemented twice via some shared state my enable this to once again work.

[sindresorhus]

Alternatively, making exit-hook guard against being implemented twice via some shared state my enable this to once again work.

Yeah, that could work.

[sindresorhus]

Actually, I just remembered that there's another exit hook module that is more comprehensive. We use it in nyc. Wonder if switching from exit-hook to signal-exit would solve our problems. Mind trying it out? It seems to have a shared emitter: https://github.com/tapjs/signal-exit/blob/6859aff54f5198c63fff91baef279b86026bde69/index.js#L13-L20

[hjdivad]

@sindresorhus it looks like signal-exit won't handle async cleanup as in @stefanpenner's earlier example

process.on('SIGINT', function() {
  asyncCleanup().finally(function() {
    process.exit(code);
  });
});

or have I missed something?

[sindresorhus]

@hjdivad I don't know. Did you actually try it?

[novemberborn]

One cop-out might be to have an option so that Ora doesn't hide the cursor. You may still want to hide it yourself but you can then also manage hooks yourself.

[tcodes0]

Hi! First and foremost thanks for the project. I'm learning how to write spinners for node by looking here 🎉. Also I'd like to give feedback but I don't feel like it needs to be a new issue. Me and a friend ran on this SIGINT issue using Ora, and this thread was the only link between that behavior and the hideCursor option. We weren't understanding why Ora was interfering in killing our CLI using ctrl+c. I think this should be mentioned in the readme, at the very least. Did you guys make hideCursor default to on to avoid introducing a breaking change?

One cop-out might be to have an option so that Ora doesn't hide the cursor. You may still want to hide it yourself but you can then also manage hooks yourself.

👆🏻That leads me to believe it should default to off.