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

lib,src,doc: add --heapsnapshot-signal CLI flag #27133

Merged
merged 1 commit into from
Apr 12, 2019

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 8, 2019

This flag allows heap snapshots to be captured without modifying application code. IMO, this is a big part of the "heapdump in core" use case.

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 8, 2019
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with @joyeecheung's comment addressed

@richardlau
Copy link
Member

Does this work on Windows? (I'm guessing not, at least based on the equivalent test in

if (!common.isWindows) {
// Verify that process.report.signal behaves properly.
assert.strictEqual(process.report.signal, 'SIGUSR2');
common.expectsError(() => {
process.report.signal = {};
}, { code: 'ERR_INVALID_ARG_TYPE' });
common.expectsError(() => {
process.report.signal = 'foo';
}, { code: 'ERR_UNKNOWN_SIGNAL' });
assert.strictEqual(process.report.signal, 'SIGUSR2');
process.report.signal = 'SIGUSR1';
assert.strictEqual(process.report.signal, 'SIGUSR1');
// Verify that the interaction between reportOnSignal and signal is correct.
process.report.signal = 'SIGUSR2';
process.report.reportOnSignal = false;
assert.strictEqual(process.listenerCount('SIGUSR2'), 0);
process.report.reportOnSignal = true;
assert.strictEqual(process.listenerCount('SIGUSR2'), 1);
process.report.signal = 'SIGUSR1';
assert.strictEqual(process.listenerCount('SIGUSR2'), 0);
assert.strictEqual(process.listenerCount('SIGUSR1'), 1);
process.report.reportOnSignal = false;
assert.strictEqual(process.listenerCount('SIGUSR1'), 0);
}
).

lib/internal/bootstrap/pre_execution.js Outdated Show resolved Hide resolved
test/sequential/test-heapdump-flag.js Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
test/sequential/test-heapdump-flag.js Outdated Show resolved Hide resolved
test/sequential/test-heapdump-flag.js Outdated Show resolved Hide resolved
@cjihrig cjihrig added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 11, 2019
@cjihrig cjihrig force-pushed the heap-signal branch 2 times, most recently from e89e9d6 to 2520bf7 Compare April 11, 2019 01:25
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 12, 2019

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

EDIT(cjihrig): CI was green.

This flag allows heap snapshots to be captured without
modifying application code.

PR-URL: nodejs#27133
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants