src,lib: make ^C print a JS stack trace #29207

legendecas · 2019-08-19T17:20:15Z

If terminating the process with ctrl-c / SIGINT, prints a JS stack trace leading up to the currently executing code.

The feature would be enabled under option --trace-sigint.

Conditions of no stacktrace on sigint:

has (an) active sigint listener(s);
main thread is idle (i.e. uv polling), a message instead of stacktrace would be printed.

Related: #24937

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

lib/internal/main/run_main_module.js

mscdex · 2019-08-19T17:53:32Z

Maybe we should only print a stack trace if stdin is a TTY (especially if it's meant to only happen when pressing ctrl-c)?

lib/internal/main/run_main_module.js

src/node_watchdog.cc

bnoordhuis

Left some comments but nice work!

src/node_watchdog.cc

addaleax

report: is the subsystem for the node-report feature, i.e. node --experimental-report … this touches a related topic, but I think it might be confusing to name it that way

src/node_watchdog.h

src/node_watchdog.cc

lib/internal/main/run_main_module.js

legendecas · 2019-08-24T13:32:20Z

report: is the subsystem for the node-report feature, i.e. node --experimental-report … this touches a related topic, but I think it might be confusing to name it that way

Yes, this work doesn't touch report specifically in codebase. Yet I don't find a more proper subsystem here https://github.com/nodejs/core-validate-commit/blob/master/lib/rules/subsystem.js . Since both src and lib are touched in this work, neither of those one might be appropriate IMO.

addaleax · 2019-08-24T13:33:28Z

@legendecas You can also use src,lib: .... 🙂 Honestly, I don’t have a better idea than that either.

lib/internal/bootstrap/pre_execution.js

legendecas · 2019-08-24T17:08:10Z

I think this PR is out of WIP, ready to be reviewed thoroughly :)

src/node_watchdog.h

nodejs-github-bot · 2019-08-25T14:54:14Z

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

addaleax · 2019-08-25T14:54:58Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1980/

addaleax · 2019-08-25T15:04:11Z

@legendecas Looks like there are still issues with the postmortem tests…

legendecas · 2019-08-25T16:45:35Z

@legendecas Looks like there are still issues with the postmortem tests…

I'll try to fix it :P

lib/internal/bootstrap/pre_execution.js

lib/internal/watchdog.js

Concerns appear to have been addressed. Unresponsive to pings. Dismissing.

Fishrock123 · 2020-01-12T18:34:38Z

I will attempt to take a look but I am in transit today. If I don’t see it by mid Monday just go ahead.

Fishrock123 · 2020-01-14T19:50:14Z

I don't have any substantive opinions for this.

legendecas · 2020-01-17T02:56:17Z

Since there are no objections anymore and multiple approves, I'd tag this as author-ready.

If terminating the process with ctrl-c / SIGINT, prints a JS stacktrace leading up to the currently executing code. The feature would be enabled under option --trace-sigint. Conditions of no stacktrace on sigint: - has (an) active sigint listener(s); - main thread is idle (i.e. uv polling), a message instead of stacktrace would be printed.

legendecas · 2020-01-25T15:18:31Z

Rebased to resolve conflicts.

nodejs-github-bot · 2020-01-26T12:04:58Z

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

If terminating the process with ctrl-c / SIGINT, prints a JS stacktrace leading up to the currently executing code. The feature would be enabled under option `--trace-sigint`. Conditions of no stacktrace on sigint: - has (an) active sigint listener(s); - main thread is idle (i.e. uv polling), a message instead of stacktrace would be printed. PR-URL: #29207 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Christopher Hiller <boneskull@boneskull.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

legendecas · 2020-01-28T05:53:59Z

Landed with 7b7e7bd 🎉

ruyadorno · 2020-01-28T14:51:36Z

/cc @nodejs/tooling this landed 🎉 yay! we might want to talk about it in the next tooling WG meeting

If terminating the process with ctrl-c / SIGINT, prints a JS stacktrace leading up to the currently executing code. The feature would be enabled under option `--trace-sigint`. Conditions of no stacktrace on sigint: - has (an) active sigint listener(s); - main thread is idle (i.e. uv polling), a message instead of stacktrace would be printed. PR-URL: #29207 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Christopher Hiller <boneskull@boneskull.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

If terminating the process with ctrl-c / SIGINT, prints a JS stacktrace leading up to the currently executing code. The feature would be enabled under option `--trace-sigint`. Conditions of no stacktrace on sigint: - has (an) active sigint listener(s); - main thread is idle (i.e. uv polling), a message instead of stacktrace would be printed. PR-URL: nodejs#29207 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Christopher Hiller <boneskull@boneskull.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

If terminating the process with ctrl-c / SIGINT, prints a JS stacktrace leading up to the currently executing code. The feature would be enabled under option `--trace-sigint`. Conditions of no stacktrace on sigint: - has (an) active sigint listener(s); - main thread is idle (i.e. uv polling), a message instead of stacktrace would be printed. PR-URL: #29207 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Christopher Hiller <boneskull@boneskull.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 19, 2019

mscdex reviewed Aug 19, 2019

View reviewed changes

lib/internal/main/run_main_module.js Outdated Show resolved Hide resolved

devsnek added the wip Issues and PRs that are still a work in progress. label Aug 19, 2019

addaleax reviewed Aug 20, 2019

View reviewed changes

bnoordhuis reviewed Aug 20, 2019

View reviewed changes

jasnell reviewed Aug 20, 2019

View reviewed changes

src/node_watchdog.cc Outdated Show resolved Hide resolved

legendecas force-pushed the issue-24937 branch 3 times, most recently from 503aca6 to f175639 Compare August 24, 2019 13:14

legendecas changed the title ~~cli: make ^C print a JS stack trace~~ report: make ^C print a JS stack trace Aug 24, 2019

addaleax approved these changes Aug 24, 2019

View reviewed changes

src/node_watchdog.h Outdated Show resolved Hide resolved

src/node_watchdog.h Outdated Show resolved Hide resolved

src/node_watchdog.cc Outdated Show resolved Hide resolved

lib/internal/main/run_main_module.js Outdated Show resolved Hide resolved

legendecas changed the title ~~report: make ^C print a JS stack trace~~ src,lib: make ^C print a JS stack trace Aug 24, 2019

legendecas force-pushed the issue-24937 branch 2 times, most recently from 3797e8e to 822e56a Compare August 24, 2019 14:13

addaleax approved these changes Aug 24, 2019

View reviewed changes

lib/internal/bootstrap/pre_execution.js Outdated Show resolved Hide resolved

legendecas force-pushed the issue-24937 branch 2 times, most recently from 124c8c3 to b7ccab2 Compare August 24, 2019 16:44

addaleax removed the wip Issues and PRs that are still a work in progress. label Aug 24, 2019

addaleax reviewed Aug 24, 2019

View reviewed changes

src/node_watchdog.h Outdated Show resolved Hide resolved

mscdex reviewed Aug 25, 2019

View reviewed changes

lib/internal/bootstrap/pre_execution.js Outdated Show resolved Hide resolved

mscdex reviewed Aug 25, 2019

View reviewed changes

lib/internal/watchdog.js Show resolved Hide resolved

mscdex reviewed Aug 25, 2019

View reviewed changes

lib/internal/watchdog.js Show resolved Hide resolved

Fishrock123 removed their request for review January 14, 2020 19:49

legendecas added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 17, 2020

legendecas removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 25, 2020

legendecas force-pushed the issue-24937 branch from d5694b3 to df403e3 Compare January 25, 2020 15:18

This comment has been minimized.

Sign in to view

legendecas added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 26, 2020

legendecas closed this Jan 28, 2020

legendecas deleted the issue-24937 branch January 28, 2020 05:54

bnoordhuis mentioned this pull request Jan 29, 2020

cli: make ^C print a JS stack trace #24937

Closed

codebytere mentioned this pull request Feb 17, 2020

v13.9.0 proposal #31837

Merged

This was referenced Apr 18, 2020

test: fix flaky test-trace-sigint-on-idle #31645

Closed

src: add missing namespace using statements in node_watchdog.h #32117

Closed

targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 25, 2020

targos mentioned this pull request May 2, 2020

v12.17.0 release proposal #33197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src,lib: make ^C print a JS stack trace #29207

src,lib: make ^C print a JS stack trace #29207

legendecas commented Aug 19, 2019 •

edited

mscdex commented Aug 19, 2019 •

edited

bnoordhuis left a comment

addaleax left a comment

legendecas commented Aug 24, 2019

addaleax commented Aug 24, 2019

legendecas commented Aug 24, 2019

nodejs-github-bot commented Aug 25, 2019

addaleax commented Aug 25, 2019

addaleax commented Aug 25, 2019

legendecas commented Aug 25, 2019

Fishrock123 commented Jan 12, 2020

Fishrock123 commented Jan 14, 2020 •

edited

legendecas commented Jan 17, 2020

legendecas commented Jan 25, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Jan 26, 2020

legendecas commented Jan 28, 2020

ruyadorno commented Jan 28, 2020 •

edited

src,lib: make ^C print a JS stack trace #29207

src,lib: make ^C print a JS stack trace #29207

Conversation

legendecas commented Aug 19, 2019 • edited

Checklist

mscdex commented Aug 19, 2019 • edited

bnoordhuis left a comment

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

legendecas commented Aug 24, 2019

addaleax commented Aug 24, 2019

legendecas commented Aug 24, 2019

nodejs-github-bot commented Aug 25, 2019

addaleax commented Aug 25, 2019

addaleax commented Aug 25, 2019

legendecas commented Aug 25, 2019

Fishrock123 commented Jan 12, 2020

Fishrock123 commented Jan 14, 2020 • edited

legendecas commented Jan 17, 2020

legendecas commented Jan 25, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Jan 26, 2020

legendecas commented Jan 28, 2020

ruyadorno commented Jan 28, 2020 • edited

legendecas commented Aug 19, 2019 •

edited

mscdex commented Aug 19, 2019 •

edited

Fishrock123 commented Jan 14, 2020 •

edited

ruyadorno commented Jan 28, 2020 •

edited