v8.writeHeapSnapshot may return undefined if the file fails to write

[kyranet]

Version

v16.13.1

Platform

Linux bd035f8a23f8 4.19.0-18-amd64 # 1 SMP Debian 4.19.208-1 (2021-09-29) x86_64 GNU/Linux

Subsystem

v8

What steps will reproduce the bug?

Call v8.writeHeapSnapshot() on a folder where the process has no permissions to write to (e.g. inside a Docker container, example dockerfile below).

Sample files

package.json

{
    "main": "index.js",
    "scripts": {
        "start": "node ."
    }
}

Dockerfile

FROM node:16-buster-slim

WORKDIR /usr/src/app

COPY --chown=node:node package.json .
COPY --chown=node:node index.js .

USER node

CMD [ "npm", "run-script", "start" ]

This will cause the following code, which returns early (and thus, returning undefined):

node/src/heap_utils.cc

Lines 387 to 388 in e4aa575

	if (!WriteSnapshot(isolate, *path))
	return;

As the code the line below (which sets the return value) is not executed:

node/src/heap_utils.cc

Line 389 in e4aa575

return args.GetReturnValue().Set(filename_v);

How often does it reproduce? Is there a required condition?

Always, requires the module to attempt to write in a folder without write permissions.

What is the expected behavior?

Either to throw an error (permissions error, out of space, etc) or to document that it may return undefined

What do you see instead?

The documentation specifies that it returns a string and does not mention that it may return undefined.

Additional information

No response

[RaisinTen]

The upstream module returns an error for this - https:/bnoordhuis/node-heapdump/blob/3537b67cebdbd602d8572fcc533f223ed25e5cbc/src/heapdump.cc#L94 but I'm not really sure if it's worth the breakage to incorporate that here. Would you like to send a doc fix PR first? Here's the relevant documentation -

node/doc/api/v8.md

Line 278 in f81c627

* Returns: {string} The filename where the snapshot was saved.

.

[kyranet]

Would it be breakage, though? The method does not document the behaviour of the function when an error during the write happens, so I believe it's completely acceptable by the versioning system to make it throw an error instead.

Documenting it as returning undefined would make the change to throwing an error a breaking change one.

If you're sure with this behaviour/decision, I can try making a docs PR in a few hours once I have time.

[RaisinTen]

Sometimes people depend on undocumented features and we tend to prefer not breaking their code, so yes a noticeable change in behavior does appear to be breaking to me.

If you're sure with this behaviour/decision, I can try making a docs PR in a few hours once I have time.

Nope, I'm not sure. Throwing an exception would have been my first choice if the function was a new addition but given that the function has existed in core for 3 years, we need to take breakage into consideration. The only reason I suggested sending a doc fix pr first was because it's easier to come up with doc only changes and you won't feel like your work has gone to waste if people think that throwing an exception is better. However, if you start off with a change that throws an exception which requires more work, your pr might get blocked and you would have to revert to a doc only change and that might not feel great.

Please feel free to send a PR to throw an exception if you feel strongly about it tho!