Repository navigation
v8.writeHeapSnapshot may return undefined if the file fails to write #41346
Description
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):
Lines 387 to 388 in e4aa575
As the code the line below (which sets the return value) is not executed:
Line 389 in e4aa575
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
Activity
RaisinTen commentedon Dec 29, 2021
The upstream module returns an error for this - https://github.com/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
kyranet commentedon Dec 29, 2021
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
undefinedwould 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 commentedon Dec 30, 2021
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.
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!
v8: make writeHeapSnapshot throw if fopen fails
v8: make writeHeapSnapshot throw if fopen fails