[feature] Write data to file on SIGINT unless specified #11

Fishrock123 · 2016-08-26T23:45:11Z

Allows better debugging of applications that have handles that are preventing them from closing.

AndreasMadsen · 2016-08-27T08:17:56Z

dprof.js

@@ -147,7 +147,17 @@ asyncHook.enable();
 // Print result
 //

-process.on('exit', function () {
+if (process.argv.indexOf('--raw-sigint') === -1 ||


perhaps call it --dprof-sigint. The raw part doesn't really make sense to me.

Also shouldn't this be !== -1.

No, the logic is correct -- I think it is better UX to have it as the default, as I suspect that is what most folks will expect.

That's also why I called it --raw-sigint, as in, don't touch the default.

It could be something like --no-sigint-handler or --no-dprof-sigint.

The ENV variable exists for programs that may error on new flags.

Should it not be !process.env.DPROF_RAW_SIGINT then?

AndreasMadsen · 2016-08-27T08:26:30Z

Thanks. You should document this.

I guess it is not easy to test with the my odd testrunner, but at the very least you can have the script signal itself.

// one of these
process.argv.push('--raw-sigint');
process.env.DPROF_RAW_SIGINT = '';
require('../../dprof.js');

setInterval(function () {}, 1000):
setTimeout(function () {
  process.kill(process.pid, 'SIGINT');
}, 200);

to make the test work:

add the script file (e.g. sigint-env.json) under scripts
add an empty json file (e.g. sigint-env.json) under expected
edit update to true in test/test.js (and back when your done).
run npm test
check the output manually
check the output visually using node test/show.js sigint-env.

Fishrock123 · 2016-08-27T14:55:08Z

@AndreasMadsen updated

AndreasMadsen · 2016-08-27T15:10:59Z

dprof.js

@@ -147,7 +147,17 @@ asyncHook.enable();
 // Print result
 //

-process.on('exit', function () {
+if (process.argv.indexOf('--dprof-no-sigint') === -1 ||
+    process.env.DPROF_NO_SIGINT) {


No, the logic is correct -- I think it is better UX to have it as the default, as I suspect that is what most folks will expect.

Should it not be !process.env.DPROF_NO_SIGINT then?

Fishrock123 · 2016-08-27T15:18:12Z

@AndreasMadsen yup, good catch. Fixed I think.

AndreasMadsen · 2016-08-27T16:59:13Z

dprof.js

+    !process.env.DPROF_NO_SIGINT) {
+  process.on('SIGINT', () => {
+    writeDataFile();
+    process.exit();


This indicates a successful exit, however when I SIGINT a process I get the exit code 130. According to http://unix.stackexchange.com/questions/99112/default-exit-code-when-process-is-terminated exit codes >= 128 indicates the process exited because of a signal.

I don't know if that is the case here, but surely the exit code should be >= 1.

Fishrock123 · 2016-08-27T17:38:04Z

Hmmm, the test doesn't seem to fail with either the flag or variable uncommented, not really sure what's up.

AndreasMadsen · 2016-08-27T20:07:08Z

Hmmm, the test doesn't seem to fail with either the flag or variable uncommented, not really sure what's up.

I've done a more thorough review ...

process.on('exit') is called when process.exit() is called from SIGINT causing two dumps.
process.env.DPROF_NO_SIGINT = '' is falsy, we need to use !process.env.hasOwnProperty('DPROF_NO_SIGINT')
if either --dprof-no-sigint or DPROF_NO_SIGINT exists, the if statement shouldn't run. This means that || should be &&. (De Morgan's laws in formal logic :p).

AndreasMadsen · 2016-08-27T20:08:47Z

I've also updated the test runner to be more sensitive to some of these issues and more robust to other. There are some conflicts, I think you can just drop 8fc3cb7 and create a new one.

Fishrock123 · 2016-08-29T17:48:21Z

process.on('exit') is called when process.exit() is called from SIGINT causing two dumps.

Arrgh, right. I did some tracing of where the current node behavior comes from, and found this old issue. Apparently generically calling exit handles is a bad idea. I'm not actually sure how to exit without calling them... process.abort() could do but that produces a core file if possible...

AndreasMadsen · 2016-08-29T19:15:09Z

Arrgh, right. I did some tracing of where the current node behavior comes from, and found this old issue. Apparently generically calling exit handles is a bad idea. I'm not actually sure how to exit without calling them... process.abort() could do but that produces a core file if possible...

"some digging", wow that is a very old issue :) A crazy idea is to remove all SIGINT handlers and have the process SIGINT itself. But I'm also fine with just calling process.exit().

Allows better debugging of applications that have handles that are preventing them from closing.

Fishrock123 · 2016-08-30T15:15:49Z

Some things:

Note sure why Signal says NaN for these:

Also, should we disable asyncHook around our own Signal handler so that it doesn't show up?

AndreasMadsen · 2016-08-30T16:09:33Z

Note sure why Signal says NaN for these.

Thought I fixed that. The wait part is because the destroy property is null (happens when the destroy doesn't fire). The solution is to use root.total time instead when destroy: null (code: https://github.com/AndreasMadsen/dprof/blob/master/visualizer/info.js#L40). It is not related to this PR so lets fix that separately, I would like this to land.

I don't know why callback is NaN too.

Also, should we disable asyncHook around our own Signal handler so that it doesn't show up?

Was, thinking about that too. If Signal handler isn't reused by another on('SIGINT') then its safe. But if it is reused then we are removing some information from the user. It may not be a big problem, as the user can pass --dprof-no-sigint for full transparency. What do you think?

Fishrock123 · 2016-08-30T16:22:50Z

The Signal handling code is at https://github.com/nodejs/node/blob/master/lib/internal/process.js#L180-L219

tl;dr: they are shared.

Maybe we should just go with this for now then?

Allows better debugging of applications that have handles that are preventing them from closing. PR: #11

PR: #11

AndreasMadsen · 2016-08-30T19:28:30Z

The Signal handling code is at https://github.com/nodejs/node/blob/master/lib/internal/process.js#L180-L219

Thanks for looking that up.

Maybe we should just go with this for now then?

Definitely. Landed in 2548239 ee3ee18 – Note I rename the test from sigint-env to just sigint.

AndreasMadsen reviewed Aug 27, 2016
View reviewed changes

Fishrock123 force-pushed the sig-handlers branch from 7faab1a to 0e9caa3 Compare August 27, 2016 14:54

AndreasMadsen reviewed Aug 27, 2016
View reviewed changes

Fishrock123 force-pushed the sig-handlers branch from 0e9caa3 to 1dcc7c6 Compare August 27, 2016 15:17

AndreasMadsen reviewed Aug 27, 2016
View reviewed changes

Fishrock123 force-pushed the sig-handlers branch from 1dcc7c6 to 8fc3cb7 Compare August 27, 2016 17:37

Fishrock123 force-pushed the sig-handlers branch from 8fc3cb7 to 6c8a7b2 Compare August 30, 2016 15:11

Fishrock123 added 2 commits August 30, 2016 11:13

[feature] Write data to file on SIGINT unless specified

0321fcd

Allows better debugging of applications that have handles that are preventing them from closing.

[test] Update tests with sigint listener

8b83818

Fishrock123 force-pushed the sig-handlers branch from 6c8a7b2 to 8b83818 Compare August 30, 2016 15:13

AndreasMadsen pushed a commit that referenced this pull request Aug 30, 2016

[feature] Write data to file on SIGINT unless specified

2548239

Allows better debugging of applications that have handles that are preventing them from closing. PR: #11

AndreasMadsen pushed a commit that referenced this pull request Aug 30, 2016

[test] Update tests with sigint listener

ee3ee18

PR: #11

AndreasMadsen closed this Aug 30, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] Write data to file on SIGINT unless specified #11

[feature] Write data to file on SIGINT unless specified #11

Fishrock123 commented Aug 26, 2016

AndreasMadsen Aug 27, 2016

AndreasMadsen Aug 27, 2016

Fishrock123 Aug 27, 2016

AndreasMadsen Aug 27, 2016

AndreasMadsen commented Aug 27, 2016 •

edited

Loading

Fishrock123 commented Aug 27, 2016 •

edited

Loading

AndreasMadsen Aug 27, 2016

Fishrock123 commented Aug 27, 2016

AndreasMadsen Aug 27, 2016

Fishrock123 commented Aug 27, 2016

AndreasMadsen commented Aug 27, 2016 •

edited

Loading

AndreasMadsen commented Aug 27, 2016 •

edited

Loading

Fishrock123 commented Aug 29, 2016

AndreasMadsen commented Aug 29, 2016 •

edited

Loading

Fishrock123 commented Aug 30, 2016

AndreasMadsen commented Aug 30, 2016

Fishrock123 commented Aug 30, 2016

AndreasMadsen commented Aug 30, 2016

[feature] Write data to file on SIGINT unless specified #11

[feature] Write data to file on SIGINT unless specified #11

Conversation

Fishrock123 commented Aug 26, 2016

AndreasMadsen Aug 27, 2016

Choose a reason for hiding this comment

AndreasMadsen Aug 27, 2016

Choose a reason for hiding this comment

Fishrock123 Aug 27, 2016

Choose a reason for hiding this comment

AndreasMadsen Aug 27, 2016

Choose a reason for hiding this comment

AndreasMadsen commented Aug 27, 2016 • edited Loading

Fishrock123 commented Aug 27, 2016 • edited Loading

AndreasMadsen Aug 27, 2016

Choose a reason for hiding this comment

Fishrock123 commented Aug 27, 2016

AndreasMadsen Aug 27, 2016

Choose a reason for hiding this comment

Fishrock123 commented Aug 27, 2016

AndreasMadsen commented Aug 27, 2016 • edited Loading

AndreasMadsen commented Aug 27, 2016 • edited Loading

Fishrock123 commented Aug 29, 2016

AndreasMadsen commented Aug 29, 2016 • edited Loading

Fishrock123 commented Aug 30, 2016

AndreasMadsen commented Aug 30, 2016

Fishrock123 commented Aug 30, 2016

AndreasMadsen commented Aug 30, 2016

AndreasMadsen commented Aug 27, 2016 •

edited

Loading

Fishrock123 commented Aug 27, 2016 •

edited

Loading

AndreasMadsen commented Aug 27, 2016 •

edited

Loading

AndreasMadsen commented Aug 27, 2016 •

edited

Loading

AndreasMadsen commented Aug 29, 2016 •

edited

Loading