Don't listen to SIGPROF

[paulirish]

We're working on a new debugging protocol for Node that exposes all functionality Chrome DevTools uses. Context:

• Inspecting Node.js with Chrome DevTools nodejs/node#2546 (comment)
• https://github.com/ofrobots/node/tree/v8-inspector
• https://chromedevtools.github.io/debugger-protocol-viewer/v8/

---

Anyhow, the SIGPROF listener in this project is causing problems when we're debugging an app using sindresorhus/meow and thus sindresorhus/loud-rejection

As a result, the debugging client is conflicting with the listener done here.

Can you remove this listener?

+cc meow folks: @sindresorhus @jamestalmage
+cc node debugger folks: @repenaxa

[bcoe]

@paulirish 👍 I'll get a release out for you this weekend.

[bcoe]

@sindresorhus @jamestalmage @isaacs, I don't think there's any good reason to continue catching SIGPROF? especially if it's interfering with debugging.

[jamestalmage]

From the signalExit docs:

Note that the function only fires for signals if the signal would cause the process to exit. That is, there are no other listeners, and it is a fatal signal.

SIGPROF appears to be a fatal signal (at least Node.js thinks so, childProcess.kill("SIGPROF") definitely kills the process). Disabling handling of that signal seems to break that contract.

@paulirish - What about just adding a noop SIGPROF listener when using debug mode:

process.on("SIGPROF", function () {});

That will prevent any listeners registered using signal-exit from firing (including loud-rejection, which I am guessing is the problem).

[jamestalmage]

I don't think there's any good reason to continue catching SIGPROF? especially if it's interfering with debugging.

I totally agree, BTW. Just playing devils advocate / wondering if there is a better way.

[bcoe]

@jamestalmage another thought, what if we made it so you could configure the signal-exit for edge-cases where you wanted to stop a certain signal -- is there an easy way we could bubble this new config all the way up to loud-rejection?

[bcoe]

shucks, I guess we'd need to bubble the config all the way through meow to loud-rejection to signal-exit ... that gets a bit ugly; wonder what a good approach is that doesn't change the contract, but gives @paulirish what he needs.

[jamestalmage]

Is Node launched with a special flag for debugging? Can we detect that from process.execArgv and selectively disable?

[jamestalmage]

what if we made it so you could configure the signal-exit for edge-cases where you wanted to stop a certain signal
...
I guess we'd need to bubble the config all the way through meow to loud-rejection to signal-exit

There basically is a way to do that already, via a noop listener. signal-exit only fires registered callbacks when no other listeners are attached for a particular signal.

[parshap]

Sorry to interject here, but #20 is somewhat related and it'd be awesome if we could remove listening to SIGPIPE along with SIGPROF (or at least add an option).

[jamestalmage]

#19 / #20 is different. SIGPIPE does not cause the process to exit under normal circumstances. So we should almost certainly remove it. The current behavior breaks the contract.

That said, I don't see a big problem with removing SIGPROF either. Node currently treats it as a fatal signal, but I am not even sure that is right. It's a profiling signal, so why would it be considered fatal? (My knowledge of what these signals mean is pretty limited, so take all my comments with a grain of salt).

I do think adding an option should be avoided. That would leave it up to users of the library to avoid interfering with the debugger.

[isaacs]

It's not node that considers it fatal, it's the OS. Since Node doesn't add a listener to the SIGPROF handler by default, it gets terminated when that signal is sent to the process.

The point of this library is to ensure that your exit callback will definitely get called on any would-be-fatal signal if at all possible. (For example, in the case of SIGKILL, it's not possible; if you add another handle, then it would not be fatal, so signal-exit does nothing.)

@paulirish When you say it's "causing problems" can you provide more details, or a link to where these problems are described? It shouldn't conflict to have multiple listeners to an event, and just removing SIGPROF from the list would violate the signal-exit contract.

[bcoe]

@iarna is running into an issue with npm/npm where npm scripts are breaking profiling; we believe to have narrowed it down to this same SIGPROF issue.

In the short term, I would like to stop hooking into SIGPROF to solve both @paulirish and npm's issue.

In the medium term, I would like to detect whether Node has been invoked in the context of a profiler and enable/disable hooking into SIGPROF accordingly.

Does this seem reasonable @isaacs?

[bcoe]

@paulirish would it be possible for you to try this candidate release:

npm i signal-exit@next

This has SIGPROF removed.

[bcoe]

@paulirish [email protected] removes the listener on SIGPROF:

https://github.com/tapjs/signal-exit/blob/master/CHANGELOG.md