Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tracing: unsafe Agent::Stop() call in signal handler #14802

Closed
bnoordhuis opened this issue Aug 13, 2017 · 7 comments
Closed

tracing: unsafe Agent::Stop() call in signal handler #14802

bnoordhuis opened this issue Aug 13, 2017 · 7 comments
Labels
trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events.

Comments

@bnoordhuis
Copy link
Member

Commit ba4847e introduces a call to tracing::Agent::Stop() in the SignalExit() function.

Signal handlers are only allowed to call async-signal-safe functions, which the implementation of tracing::Agent::Stop() is not. This can cause erratic behavior; crashes, hangs, etc.

@jasongin @matthewloring

@bnoordhuis bnoordhuis added the trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. label Aug 13, 2017
@matthewloring
Copy link

This was added so that traces would be flushed even if the process did not exit cleanly. Is there an alternate place where this kind of shutdown logic can safely live and still run on all types of process exit?

@bnoordhuis
Copy link
Member Author

Not in case of SIGINT or SIGTERM, no, although you are allowed to call write(2) and close(2). Perhaps tracing::Agent needs to grow a SignalSafeStop() method? (3x alliteration bonus)

@matthewloring
Copy link

Does this code ever run on windows or are linux-only operations allowed?

@bnoordhuis
Copy link
Member Author

No, never runs on Windows. (Weirdly, it's not surrounded by an #ifdef __POSIX__. It should be, it's dead code.)

If you mean 'POSIX-only' rather than 'linux-only', yes, that's okay. Signals aren't a thing on Windows.

@matthewloring
Copy link

I got a chance to look into this a bit. I think the primary issue is that safely flushing the buffer requires locking a mutex which cannot be safely done inside a signal handler (I think). I'm not sure it will be possible to write this safely without ensuring the other thread isn't concurrently flushing the same buffer or swapping buffers mid flush.

@bnoordhuis
Copy link
Member Author

No longer hypothetical: #11052 (comment)

cc @nodejs/trace-events

@jasnell
Copy link
Member

jasnell commented Aug 12, 2018

@addaleax ... with the recent refactoring that you did, does this need to remain open?

addaleax added a commit to addaleax/node that referenced this issue Sep 6, 2018
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
targos pushed a commit that referenced this issue Sep 17, 2018
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>
targos pushed a commit that referenced this issue Sep 19, 2018
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>
targos pushed a commit that referenced this issue Sep 20, 2018
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>
thedull added a commit to thedull/node that referenced this issue Jan 7, 2022
thedull added a commit to thedull/node that referenced this issue Jan 18, 2022
nodejs-github-bot pushed a commit that referenced this issue Jan 21, 2022
Refs: #14802

PR-URL: #41438
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
BethGriggs pushed a commit that referenced this issue Jan 25, 2022
Refs: #14802

PR-URL: #41438
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Jan 31, 2022
Refs: nodejs#14802

PR-URL: nodejs#41438
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
danielleadams pushed a commit that referenced this issue Feb 28, 2022
Refs: #14802

PR-URL: #41438
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
danielleadams pushed a commit that referenced this issue Mar 2, 2022
Refs: #14802

PR-URL: #41438
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
danielleadams pushed a commit that referenced this issue Mar 3, 2022
Refs: #14802

PR-URL: #41438
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
danielleadams pushed a commit that referenced this issue Mar 14, 2022
Refs: #14802

PR-URL: #41438
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants