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

doc: add note for handling signal events in trace events #41438

Merged
merged 2 commits into from
Jan 21, 2022

Conversation

thedull
Copy link
Contributor

@thedull thedull commented Jan 7, 2022

Keeping in mind issues like #18476 and #14802, I went ahead and added a note to the docs for users to handle signal events properly, so that trace events are correctly logged into files.

I'm happy to make any changes to this PR as needed since it's my first time contributing to the Node.js project.

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jan 7, 2022
@benjamingr
Copy link
Member

@nodejs/diagnostics

Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@benjamingr
Copy link
Member

Can someone from diagnostics explain why the signal handler is needed in this case :)?

doc/api/tracing.md Outdated Show resolved Hide resolved
@Flarna
Copy link
Member

Flarna commented Jan 18, 2022

Reading the linked issues and #22734 tells that flushing the trace events in a signal handler is not trivial and would require quite some work (if possible at all). There is #22883 since a while which asks for a fix.

By installing a signal handler from JS the default action of this signal (end process) is not executed anymore. By calling process.exit() from the JS handler a shutdown triggered via signal is moved to a graceful shutdown which is not limited to only calling signal safe functions and flush works.

@thedull thedull marked this pull request as draft January 18, 2022 18:18
@thedull thedull marked this pull request as ready for review January 18, 2022 18:39
@Ayase-252 Ayase-252 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 21, 2022
@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jan 21, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 21, 2022
@nodejs-github-bot nodejs-github-bot merged commit 806c7c1 into nodejs:master Jan 21, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 806c7c1

BethGriggs pushed a commit that referenced this pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.