raszi / node-tmp

Temporary file and directory creator for node.js
MIT License
736 stars 92 forks source link

SIGINT handlers defined in app are not executed #192

Closed alexandervain closed 4 years ago

alexandervain commented 5 years ago

Operating System

NodeJS Version

Tmp Version

Expected Behavior

Allow executing app defined SIGINT handlers

Experienced Behavior

App exits before custom handlers are executed.

Here is the reason: https://github.com/raszi/node-tmp/blob/master/lib/tmp.js#L635 Code to reproduce:

const tmp = require('tmp');
console.log(`Run from terminal: kill -SIGINT ${process.pid}`);

process.on('SIGINT', () => {
    console.log('Custom SIGINT handler - do some clean up')
    process.exit(0);
});

setTimeout(() => {}, 100000)

Custom SIGINT handler - do some clean up not printed.

The same code works OK with tmp@0.0.33

silkentrance commented 5 years ago

@alexandervain Very good find. This always went unnoticed in the tests as we do not have any custom SIGINT handlers installed. I wonder why the routine even checks the doExit parameter... I think that this can be removed completely.

silkentrance commented 5 years ago

Ok, the doExit was there for a reason, having to do with jest and how it is managing its sandboxes, which eventually will run the initialising code of tmp multiple times.

I think I have nailed it down and will prepare a PR now.

alubbe commented 5 years ago

@alexandervain @papandreou Does this commit fix the bug? https://github.com/raszi/node-tmp/commit/df8df51ce4d5272c4bafb3bcfa394ec22fffb6a3

eugene1g commented 5 years ago

There is also the case of async handlers - we have process.on() handlers that return promise, and NodeJS will wait for those promises to resolve before existing. As I understand https://github.com/raszi/node-tmp/commit/df8df51ce4d5272c4bafb3bcfa394ec22fffb6a3, it will call original handlers in a sync matter, which will not give async functions enough time to complete.

Tyler-Murphy commented 4 years ago

@alubbe, I encountered the same bug while doing asynchronous operations in a SIGINT event listener. It was fixed by df8df51.

alubbe commented 4 years ago

sounds good - how about merging https://github.com/raszi/node-tmp/pull/193 ?