X Tutup
The Wayback Machine - https://web.archive.org/web/20250815144051/https://github.com/nodejs/node/pull/59040
Skip to content

lib: add trace-sigint APIs #59040

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 11, 2025
Merged

lib: add trace-sigint APIs #59040

merged 1 commit into from
Aug 11, 2025

Conversation

theanarkh
Copy link
Contributor

@theanarkh theanarkh commented Jul 12, 2025 •
edited
Loading

trace-sigint command line option is useful for debugging the busy code, expose it as API for using at runtime, such as APM SDK.

  • 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

@theanarkh theanarkh marked this pull request as ready for review July 12, 2025 08:44
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Jul 12, 2025
@theanarkh theanarkh force-pushed the make_trace_sigint_programmable branch 4 times, most recently from d3eadac to e8583be Compare July 12, 2025 18:47
@codecov Codecov
Copy link

codecov bot commented Jul 12, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.91%. Comparing base (60a58f6) to head (2cdca4e).
⚠️ Report is 19 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #59040      +/-   ##
==========================================
- Coverage   89.91%   89.91%   -0.01%     
==========================================
  Files         655      656       +1     
  Lines      192828   192860      +32     
  Branches    37805    37821      +16     
==========================================
+ Hits       173385   173411      +26     
- Misses      12029    12035       +6     
  Partials     7414     7414
Files with missing lines Coverage Δ
lib/internal/process/pre_execution.js 90.39% <100.00%> (-0.04%) ⬇️
lib/internal/util/trace_sigint.js 100.00% <100.00%> (ø)
lib/util.js 97.97% <100.00%> (+0.02%) ⬆️

... and 34 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@theanarkh
Copy link
Contributor Author

@legendecas Hi ! Could you help review this PR ? Thanks !

legendecas
legendecas reviewed Jul 29, 2025
View reviewed changes
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Not a strong opinion but maybe these two methods can be merged into process.setTraceSigIntEnabled(enabled).

legendecas

This comment was marked as resolved.

Sign in to view

@theanarkh theanarkh force-pushed the make_trace_sigint_programmable branch from e8583be to 034cd81 Compare July 29, 2025 13:04
@theanarkh
Copy link
Contributor Author

Thanks, i have updated the API to process.setTraceSigInt(enable).

jasnell
jasnell reviewed Jul 29, 2025
View reviewed changes
doc/api/process.md Outdated

* `enable` {boolean}

Enable or disable printing a stack trace on `SIGINT`. The API is only available on the main thread.
Copy link
Member

@jasnell jasnell Jul 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Just as a general rule I'm fairly reluctant to add new APIs to process. Could this be an environment variable instead? Specifically, why does this need to be an API? Not blocking, just wanting to understand the use case more.

Copy link
Contributor Author

@theanarkh theanarkh Jul 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In the APM scenario, we can not set the --trace-sigint flag for user at startup. So need a API which can be called at runtime.

Copy link
Member

Choose a reason for hiding this comment

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

Could this be added to util instead? Like I said, I'm always a bit reluctant to add new APIs to the process global.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done !

Copy link
Member

Choose a reason for hiding this comment

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

I feel like signal related APIs are already on process. So it might not hurt to have another signal API on process... But I don't feel strongly about this.

@theanarkh theanarkh force-pushed the make_trace_sigint_programmable branch 2 times, most recently from f4aebda to 450d165 Compare July 29, 2025 18:46
@nodejs-github-bot
Copy link
Collaborator

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

@legendecas
Copy link
Member

The test is failing on windows.

@theanarkh theanarkh force-pushed the make_trace_sigint_programmable branch from 450d165 to 8ec2621 Compare August 5, 2025 17:11
@theanarkh theanarkh force-pushed the make_trace_sigint_programmable branch from 8ec2621 to 2cdca4e Compare August 7, 2025 13:29
@theanarkh theanarkh requested review from jasnell and legendecas August 8, 2025 11:31
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@theanarkh theanarkh added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 9, 2025
@legendecas legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 11, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 11, 2025
@nodejs-github-bot nodejs-github-bot merged commit b87312b into nodejs:main Aug 11, 2025
60 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in b87312b

RafaelGSS pushed a commit that referenced this pull request Aug 11, 2025
@theanarkh @RafaelGSS
lib: add trace-sigint APIs
9ca93a5 
PR-URL: #59040
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@github-actions github-actions bot mentioned this pull request Aug 11, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request Aug 12, 2025
@theanarkh @RafaelGSS
lib: add trace-sigint APIs
e055539 
PR-URL: #59040
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legendecas legendecas legendecas approved these changes

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. 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.
4 participants
@theanarkh @nodejs-github-bot @legendecas @jasnell
X Tutup