tracing: remove shutdown-on-signal #22734
Conversation
This feature cannot be reasonably implemented this way without inherently being susceptible to race conditions that lead to hangs, crashes, etc. What’s more, implementing this for some signals only (and it can only be implemented for some signals at all) may lead to the impression that it is a guaranteed feature, when really consumers of the tracing output *need* to be able to handle abrupt ends meaningfully. Fixes: nodejs#14802 Fixes: nodejs#22528
nodejs-github-bot
added
the
c++
addaleax
added
the
trace_events
|
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 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? |
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 You can read about signal safty here: http://man7.org/linux/man-pages/man7/signal-safety.7.html. |
@AndreasMadsen I haven't dig into how |
@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 |
|
Just thinking out loud... Could we potentially handle this by setting a flag within |
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. |
Yes, my suggestions were based on the same list, just a different source.
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. |
addaleax
added
the
author ready
|
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). |
|
Landed in 80076cb |
This feature cannot be reasonably implemented this way without inherently being susceptible to race conditions that lead to hangs, crashes, etc. What’s more, implementing this for some signals only (and it can only be implemented for some signals at all) may lead to the impression that it is a guaranteed feature, when really consumers of the tracing output *need* to be able to handle abrupt ends meaningfully. Fixes: #14802 Fixes: #22528 PR-URL: #22734 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This feature cannot be reasonably implemented this way without inherently being susceptible to race conditions that lead to hangs, crashes, etc. What’s more, implementing this for some signals only (and it can only be implemented for some signals at all) may lead to the impression that it is a guaranteed feature, when really consumers of the tracing output *need* to be able to handle abrupt ends meaningfully. Fixes: #14802 Fixes: #22528 PR-URL: #22734 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This feature cannot be reasonably implemented this way without inherently being susceptible to race conditions that lead to hangs, crashes, etc. What’s more, implementing this for some signals only (and it can only be implemented for some signals at all) may lead to the impression that it is a guaranteed feature, when really consumers of the tracing output *need* to be able to handle abrupt ends meaningfully. Fixes: #14802 Fixes: #22528 PR-URL: #22734 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This feature cannot be reasonably implemented this way without inherently being susceptible to race conditions that lead to hangs, crashes, etc. What’s more, implementing this for some signals only (and it can only be implemented for some signals at all) may lead to the impression that it is a guaranteed feature, when really consumers of the tracing output *need* to be able to handle abrupt ends meaningfully. Fixes: #14802 Fixes: #22528 PR-URL: #22734 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This feature cannot be reasonably implemented this way
without inherently being susceptible to race conditions
that lead to hangs, crashes, etc.
What’s more, implementing this for some signals only
(and it can only be implemented for some signals at all)
may lead to the impression that it is a guaranteed
feature, when really consumers of the tracing output
need to be able to handle abrupt ends meaningfully.
Fixes: #14802
Fixes: #22528
@nodejs/diagnostics @jasnell @BridgeAR
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes