★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/27133Nouvelle recherche | Portail wanayoo
Skip to content

/node

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
+74 −0

Conversation

Reviewers

@bnoordhuis bnoordhuis

@joyeecheung joyeecheung

@sam-github sam-github

@jasnell jasnell

@addaleax addaleax

@richardlau richardlau

Assignees
No one assigned
Labels
C++ semver-minor
Projects
None yet
Milestone
No milestone
8 participants
@cjihrig @nodejs-github-bot @richardlau @sam-github @bnoordhuis @jasnell @addaleax @joyeecheung
@cjihrig
Copy link
Contributor

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

This comment has been minimized.

Sign in to view
Copy link

commented Apr 8, 2019

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

@nodejs-github-bot nodejs-github-bot added the C++ label Apr 8, 2019

@joyeecheung

joyeecheung reviewed Apr 8, 2019
View changes

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

@cjihrig cjihrig force-pushed the cjihrig:heap-signal branch from bca5f9e to ce07ec7 Apr 8, 2019

@joyeecheung

joyeecheung reviewed Apr 8, 2019
View changes

Show resolved Hide resolved lib/internal/bootstrap/pre_execution.js
@jasnell

jasnell approved these changes Apr 8, 2019
View changes

Copy link
Member

left a comment

LGTM with @joyeecheung's comment addressed

@cjihrig cjihrig force-pushed the cjihrig:heap-signal branch from ce07ec7 to b20b6f7 Apr 8, 2019

@richardlau

This comment has been minimized.

Sign in to view
Copy link
Member

commented Apr 8, 2019

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

node/test/report/test-report-config.js

Lines 63 to 87 in cc4e8e0

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);
}
).
@bnoordhuis

bnoordhuis reviewed Apr 8, 2019
View changes

Show resolved Hide resolved lib/internal/bootstrap/pre_execution.js Outdated
Show resolved Hide resolved test/sequential/test-heapdump-flag.js Outdated
@sam-github

sam-github approved these changes Apr 8, 2019
View changes

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

@cjihrig cjihrig added the semver-minor label Apr 11, 2019

@cjihrig cjihrig force-pushed the cjihrig:heap-signal branch from b20b6f7 to a446608 Apr 11, 2019

@richardlau

richardlau reviewed Apr 11, 2019
View changes

Show resolved Hide resolved doc/api/cli.md

@cjihrig cjihrig force-pushed the cjihrig:heap-signal branch 2 times, most recently from e89e9d6 to 2520bf7 Apr 11, 2019

@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Apr 11, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/22362/
@richardlau

richardlau approved these changes Apr 11, 2019
View changes

@cjihrig cjihrig force-pushed the cjihrig:heap-signal branch from 2520bf7 to f84eddf Apr 12, 2019

@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Apr 12, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/22383/
@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Apr 12, 2019 •
edited by cjihrig

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

EDIT(cjihrig): CI was green.
@addaleax

addaleax approved these changes Apr 12, 2019
View changes

@cjihrig
lib,src,doc: add --heapsnapshot-signal CLI flag 
This flag allows heap snapshots to be captured without
modifying application code.

PR-URL: #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>
Verified
This commit was signed with a verified signature.
@cjihrig cjihrig Colin Ihrig
GPG key ID: 7434390BDBE9B9C5 Learn about signing commits
Loading status checks…
9b6b567

@cjihrig cjihrig force-pushed the cjihrig:heap-signal branch from f84eddf to 9b6b567 Apr 12, 2019

@cjihrig cjihrig merged commit 9b6b567 into nodejs:master Apr 12, 2019

@cjihrig cjihrig deleted the cjihrig:heap-signal branch Apr 12, 2019

This was referenced Apr 23, 2019

Open
Open
Open
Open
Open

This was referenced Apr 23, 2019

Open
Open
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.