cli: add --heapsnapshot-near-heap-limit option

[joyeecheung]

This patch adds a --heapsnapshot-near-heap-limit CLI option
that takes heap snapshots when the V8 heap is approaching
the heap size limit. It will try to write the snapshots
to disk before the program crashes due to OOM.

This currently takes a number specifying the maximum number
of heap snapshots Node.js will write - doing heap snapshots
triggers GC eagerly and could keep the process in limbo for longer
than it should in a heap with unbounded growth, and it may consume
a lot of CPU and I/O resources to serialize these snapshots while the
process is irresponsive, so users may want to limit the number of
heap snapshots taken. The heap limit could be approached multiple
times due to the GC trigger by taking heap snapshots, thus multiple
heap snapshots may be taken before the program crashes.

Refs: #27552

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

[jasnell]

Definitely like that this is moving forward but think we should make this experimental for a while until we understand the full effects on various platforms with different memory limitations.

[mmarchini]

👍 on explicitly stating this is an experimental feature for now.

[sam-github]

Feature looks useful, great to have it.

I'm not sure it will be so easily used in Kubernetes, or even Docker deployments with immutable FS.

At a minimum, it would be necessary to select the FS directory that snapshots should be written to so a Volume could be mounted there writable and the data would be persisted. The name should be somewhat uniqueish to the container, not sure exactly how to manage that.

Other possibility would be to write the data to stdout, so it goes out as JSON through the standard logging mechanisms. If its absolutely unreasonably large, maybe thats not reasonable, but worth a shot, its definitely the "least configuration required" way to get the data into persistent system storage and available to SREs.

[jasnell]

We actually have the same problem with other diagnostic files such as trace events, where we can specify a file pattern but not a location for those to be written. We should likely work on solving that problem more generally such that we can specify a process-level destination for all diagnostic files.

[sam-github]

--report-directory=... could be made an alias to a slightly more generic sounding option, and used by all the diagnostic file writing functions, instead of just report.

Similarly with report-filename, though its perhaps not as directly useable as-is. stdout/stderr are directly meaningful, but for the templates, each feature would need is own specific file extension, I guess? Or a larger part of the name should be specific to the file type?

[mmarchini]

Sending to stdout is concerning because a) the snapshot can get quite large; and b) it can get mixed up with other logs. But I understand some users might want that, I just wouldn't like to see it as the default behavior.

Sending to a configurable directory sounds reasonable. As an alternative, we could also pipe it to a named pipe (provided via a flag), so the users can send it straight to a storage service like S3 without touching disk, or to encrypt/compress/obfuscate before letting the heap snapshot touches the disk. Although this will probably not be an option for Windows.

[sam-github]

I don't think writing to stdout should be default either, just possible.

Note that stderr is currently used for random unstructured output, v8 will write there, for example, and its where warnings show up, etc. Diagnostic reports, when written, get a blurb written to stderr saying that the write occurred (EDIT, and naming the file), so perhaps something similar makes sense for this feature.

For named pipe, no specific support should be required beyond being able to specify the filename: --heapdump-filename=/path/to/pipe would work for example.

[joyeecheung]

I simply used the CWD for writing heap snapshots with the default pattern since this is what --heapsnapshot-signal already does. We could add support for more configurations for the output, but I guess whatever we do need to be effective for --heapsnapshot-signal as well, so we should probably spin up a separate issue for that. (and the changes for that would likely to be reflected in WriteSnapshot() which is shared by this PR and the --heapsnapshot-signal implementation.)

[nodejs-github-bot]

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

[joyeecheung]

I bumped the max old space size to 50mb, because otherwise the heap is too small and also the old generation size could be smaller than the young generation size (by default) which isn't too realistic. This should be fine for the test machines.

[paulrutter]

@joyeecheung Thanks for landing this feature in 14.18.0 👍
While testing, i came across the fact that it's now not possible to configure a name for the heapsnapshot file, thus making it unrelatable to a specific process.

In my case, a lot of Node.js processes are forked from a main process. For those processes, --heapsnapshot-near-heap-limit is passed. It would be very handy to be able to name those heapsnapshots, so i can recognize them afterwards.

I would have expected the --heap-prof-name and --heap-prof-dir to have worked in combination with --heapsnapshot-near-heap-limit, but it doesn't.
Any thoughts on this?

Since the process is gone, i cannot relate the pid to the process after the OoM, unless i keep some kind of mapping from pid to a friendly name.

[joyeecheung]

@paulrutter This feature uses the common default naming scheme used by diagnostic reports, --heapsnapshot-signal and --cpu-prof/--heap-prof, which should include the pid (the format is Heap.YYYYMMDD.HHmmSS.${pid}.${threadid}.${count}.heapsnapshot, it only lacks a way to customize the name and the directory directly, similar to --heapsnapshot-signal. Support for those customization should be a separate feature (and IMO --heapsnapshot-near-heap-limit should share the options with --heapsnapshot-signal, not --heap-prof, because --heap-prof generates a completely different profile)

[paulrutter]

Thanks for the explanation, i agree that that should be a separate feature request.
Will create a new issue for it that references this one.

Also agree on --heap-prof vs --heapsnapshot part, i just mentioned it because it would have worked with those cli options (even though it makes more sense to add a new one for heapsnapshots).

[paulrutter]

@joyeecheung created #40363