FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: updated language around adding listeners on SIGUSR1 by willhayslett · Pull Request #19709 · nodejs/node · GitHub

/ node Public

doc: updated language around adding listeners on SIGUSR1 - #19709

Closed
willhayslett wants to merge 1 commit into
nodejs:masterfrom
willhayslett:doc-process-inspector-sigusr1
ConversationCommits (1)ChecksFiles changed
Closed

doc: updated language around adding listeners on SIGUSR1#19709
willhayslett wants to merge 1 commit into
nodejs:masterfrom
willhayslett:doc-process-inspector-sigusr1

Conversation

willhayslett commented Mar 30, 2018

Copy link
Copy Markdown
Contributor

Updated the doc/api/process.md documentation to reflect that
listening on SIGUSR1 could impact the debugger.

Fixes: #19619

Checklist

nodejs-github-bot added doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem. labels Mar 30, 2018

Trott commented Mar 30, 2018

Copy link
Copy Markdown
Member

Question/suggestion: Would it make sense to be more specific? Instead of "might interfere with the debugger", maybe something like "might override the default behavior of causing the process to start listening for debugging messages"? Well, something a bit more clear than that, but that sort of thing? Maybe "might stop the signal's default behavior of activating the inspector"?

Trott approved these changes Mar 30, 2018
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Despite my question/suggestion, this looks good to me as it is.

Trott commented Mar 30, 2018

Copy link
Copy Markdown
Member

@bnoordhuis

Trott commented Mar 30, 2018

Copy link
Copy Markdown
Member

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/370/

Trott added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 30, 2018
willhayslett changed the title doc: updated language around adding listens on SIGUSR1 doc: updated language around adding listeners on SIGUSR1 Mar 30, 2018

willhayslett commented Mar 30, 2018

Copy link
Copy Markdown
Contributor Author

Good call out @Trott. The language is vague in it's current form. Are we only worried about the debugger not starting if we register listeners on SIGUSR1 or are there other side effects that we'd want to warn users about? If it's the latter and those side effects are unknown or potentially robust and represent behavior we've no intention of testing/validating because adding the custom listener isn't "supported" for the debugger, does the ambiguity make more sense? Maybe we could be more stern or explicit in letting them know the results could be unexpected in such a case?

gireeshpunathil commented Mar 31, 2018

Copy link
Copy Markdown
Member

how about this?
Installing a listener for this signal will lead to race condition with the in-built debugger that installs handler for the same signal. Unexpected behavior can result, depending on the order of handler registration (late comer overrides existing one), so it is recommended not to install one in user programs.

bnoordhuis approved these changes Mar 31, 2018
View reviewed changes

bnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

To @Trott et al, the reason I suggested not being too specific is to allow for a little leeway in case the current implementation needs to change.

All the reader needs to know is that listening for SIGUSR1 is not a good idea, the details aren't important.

Comment thread doc/api/process.md Outdated

bnoordhuis Mar 31, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Typo: interfere

Trott commented Mar 31, 2018

Copy link
Copy Markdown
Member

I'm sold on @bnoordhuis's explanation and +1 to the current wording (with the one typo fixed, of course).

doc: updated language around adding listens on SIGUSR1
20b9987 
Updated the doc/api/process.md documentation to reflect that
listening on SIGUSR1 could impact the debugger.

Fixes: nodejs#19619
willhayslett force-pushed the doc-process-inspector-sigusr1 branch from e6ceea4 to 20b9987 Compare March 31, 2018 13:26

willhayslett commented Mar 31, 2018

Copy link
Copy Markdown
Contributor Author

Typo fixed. Thanks, everyone!

Trott commented Mar 31, 2018

Copy link
Copy Markdown
Member

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/374/

trivikr commented Apr 3, 2018

Copy link
Copy Markdown
Member

Landed in 67bbc84

trivikr closed this Apr 3, 2018
trivikr pushed a commit that referenced this pull request Apr 3, 2018
doc: update to adding listens on SIGUSR1
67bbc84 
Updated the doc/api/process.md documentation to reflect that
listening on SIGUSR1 could impact the debugger.

Fixes: #19619

PR-URL: #19709
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 3, 2018
doc: update to adding listens on SIGUSR1
c1b83fc 
Updated the doc/api/process.md documentation to reflect that
listening on SIGUSR1 could impact the debugger.

Fixes: #19619

PR-URL: #19709
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos mentioned this pull request Apr 4, 2018
Merged
BethGriggs pushed a commit that referenced this pull request Dec 4, 2018
doc: update to adding listens on SIGUSR1
6d8c65e 
Updated the doc/api/process.md documentation to reflect that
listening on SIGUSR1 could impact the debugger.

Fixes: #19619

PR-URL: #19709
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs mentioned this pull request Dec 4, 2018
Merged
This was referenced Dec 18, 2018
Closed
Closed
Closed
Closed
beevelop mentioned this pull request Dec 18, 2018
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis approved these changes
jasnell jasnell approved these changes
Trott Trott approved these changes
trivikr trivikr approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Listening to SIGUSR1 blocks debugger

7 participants

Back | FazBrowse Home | New Git URL