@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/820/pipeline

Yeah, buy how can we flush the tracking stream. That is also a pretty big problem. I have honestly been thinking about just making a while (check) {} loop. But I don't think that is a particular good solution either.

I definitely don't see how it fixes #14802 #22528. I agree that tracing consumers should be able to parse a partial file. But there is a big difference between a SIGKILL and a SIGINT. For the purpose of #22528, the implementation actually SIGINTs the process, which alreay can lead to no tracking output at all. I can't imagine this makes it any better.

Also please keep in mind that it is only users of tracking that can get a segfault from this.

I think this fixes the crashes, but it worsen the problem of flushing things out. That last piece of data could be extremely important in case of an error situation.

If we cannot really do this in the C++ signal handler, can we do it afterwards, e.g. before the process exits?

If we cannot really do this in the C++ signal handler, can we do it afterwards, e.g. before the process exits?

As far as I know, there is nothing after the signal handler. I think the solution is to use something that doesn't depend on mutex. I'm no POSIX expert, but maybe kill/wait, send/recv, or connect/close.

You can read about signal safty here: http://man7.org/linux/man-pages/man7/signal-safety.7.html.

As far as I know, there is nothing after the signal handler. I think the solution is to use something that doesn't depend on mutex. I'm no POSIX expert, but maybe kill/wait, send/recv, or connect/close.

@AndreasMadsen I haven't dig into how process.on('SIGINT') is hooked, but I bet the JS code is signal-safe.

As far as I know, there is nothing after the signal handler. I think the solution is to use something that doesn't depend on mutex. I'm no POSIX expert, but maybe kill/wait, send/recv, or connect/close.

@AndreasMadsen https://linux.die.net/man/7/signal has a list of things that are okay to call. It’s not just mutexes – even allocating or freeing memory is not okay (and not just in theory).

We could write data to and/or close file descriptors, yes. That’s really far from trivial to do for the tracing buffers in the current design, though. (I’d be happy to review any PR that replaces the current code with something that is not broken, but for now, this ‘feature’ is broken so it should be removed.)

@mcollina process.on('SIGINT') is asynchronous, which means that we can’t use it.

Just thinking out loud... Could we potentially handle this by setting a flag within SignalExit that the trace agent watches for then flushes?

@jasnell I think @mcollina’s suggestion was along those lines, but that only works if there is code running after the SignalExit method; but SignalExit exits the current process…

That last piece of data could be extremely important in case of an error situation.

IMHO, dealing with this kinds of crashes requires tooling that digs out the data from a core-dump. It is going to be fairly hard to write this flushing correctly and what we have today doesn't work.

https://linux.die.net/man/7/signal has a list of things that are okay to call. It’s not just mutexes – even allocating or freeing memory is not okay (and not just in theory).

Yes, my suggestions were based on the same list, just a different source.

(I’d be happy to review any PR that replaces the current code with something that is not broken, but for now, this ‘feature’ is broken so it should be removed.)

That is acceptable. But we can't say that it fixes #14802 or #22528. If you insist on that, another issue should be creased.

CI: https://ci.nodejs.org/job/node-test-pull-request/17093/

I do think that it fixes those issues – just not in a way that makes anybody happy. I’ll create a new one after this PR has been merged.

Windows-only rebuild: https://ci.nodejs.org/job/node-test-commit-windows-fanned/20682/

(needs re-run)

Last CI run was not successful for unrelated reasons (nodejs/build#1495).
Resumed: https://ci.nodejs.org/job/node-test-pull-request/17195/
Windows rebuild: https://ci.nodejs.org/job/node-test-commit-windows-fanned/20720/

Landed in 80076cb