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

doc,v8: clarify v8.writeHeapSnapshot may return undefined #41365

Conversation

kyranet
Copy link
Contributor

@kyranet kyranet commented Dec 31, 2021

Fixes: #41346

Or at least, it's one of the possible approaches. There are two we can do:

  1. Keep current behavior, document as undefined.
  2. Throw an exception and document as such.

v8.writeHeapSnapshot() calls the following code:

node/src/heap_utils.cc

Lines 369 to 390 in 0de6a63

void TriggerHeapSnapshot(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Isolate* isolate = args.GetIsolate();
Local<Value> filename_v = args[0];
if (filename_v->IsUndefined()) {
DiagnosticFilename name(env, "Heap", "heapsnapshot");
if (!WriteSnapshot(isolate, *name))
return;
if (String::NewFromUtf8(isolate, *name).ToLocal(&filename_v)) {
args.GetReturnValue().Set(filename_v);
}
return;
}
BufferValue path(isolate, filename_v);
CHECK_NOT_NULL(*path);
if (!WriteSnapshot(isolate, *path))
return;
return args.GetReturnValue().Set(filename_v);
}

As we can see, the lack of args.GetReturnValue().Set(...) when WriteSnapshot(isolate, *name) returns false (lines 377 and 387) makes the function always return undefined when the file fails to write.

It does seem that the upstream module sets an error, which can be a lot more useful: https://github.com/bnoordhuis/node-heapdump/blob/3537b67cebdbd602d8572fcc533f223ed25e5cbc/src/heapdump.cc#L93-L96

I believe node-heapdump's approach is more sensible, as there are several reasons why a file write can fail (out of space, missing permissions...) and said error helps us determine the cause, where an undefined only tells us that the write failed.

The usage of v8.writeHeapSnapshot() is quite small in the ecosystem (according to GitHub's Code Search, 16 usages in JavaScript and 5 in TypeScript outside of Node.js forks) all of them rely on it returning a string or to write a file successfully, none of the matches seem to compare against undefined or a falsy/truthy value.

I don't know what would be the exact implication of defining the error state as an error being thrown rather than silently erroring with undefined, however, one of the matches was from DefinitelyTyped, a file used in CI:

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/2aae4db6bbe2e078c0cbc84cc9cd43cb1e01d8fd/types/node/v14/test/v8.ts#L13

I believe we are still able to change this behavior to throw an exception because:

  • Low adoption in the (open-source) ecosystem* despite the function existing for three years.
  • Developers seem to assume the write never fails or that it always returns a string*.
  • The error state is not documented (and typically, changes in undocumented features usually don't fall into semantic versioning), if we merge this PR, changing the behavior to throwing an exception would result on a breaking change, which we are not allowed to do if we want to keep upwards compatibility.

*: According to cs.github.com's data.


As recommended by @RaisinTen in the aforementioned issue, I'm opening a docs PR. I would have opted in for making a PR for Approach 2 (throw an exception) instead if I had more availability and I was more confident to contribute in the code.

I'll be more than happy if somebody else can make the PR if the team decides to take the second approach, otherwise this PR should have everything for the first one. I checked all other heap snapshot methods, and none seems to have this behavior-documentation mismatch.

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Dec 31, 2021
Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

Thank you for sending the PR!
Could you please update the return type in the JSDoc declaration of the function?

node/lib/v8.js

Line 66 in 0de6a63

* @returns {string}

We also needs a test in https://github.com/nodejs/node/blob/HEAD/test/sequential/test-heapdump.js that asserts that the function indeed returns undefined when it fails to write the file.

@@ -275,7 +275,8 @@ added: v11.13.0
generated, where `{pid}` will be the PID of the Node.js process,
`{thread_id}` will be `0` when `writeHeapSnapshot()` is called from
the main Node.js thread or the id of a worker thread.
* Returns: {string} The filename where the snapshot was saved.
* Returns: {string|undefined} The filename where the snapshot was saved, or
`undefined` if the file failed to write.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`undefined` if the file failed to write.
`undefined` if the process failed to write the file.

@addaleax
Copy link
Member

Why are we documenting this rather than making it throw an error?

@kyranet
Copy link
Contributor Author

kyranet commented Dec 31, 2021

In the linked PR, I was advised to do a docs PR in case throwing an exception would eventually become blocked, as seen in #41346 (comment)

I also lack of the knowledge to do such changes at the moment as well as my free time is limited until I finish my finals in 20 days. I can try to get back to this and make another PR making it throw, but I can't make promises I'll succeed in that task, this is my first time contributing to this project so I'm not very familiar with the code.

As I mentioned at the end of this PR's description, I'll be happy if somebody more familiar with the code can take the task if we want the function to throw an error on failure :P

@RaisinTen
Copy link
Contributor

The usage of v8.writeHeapSnapshot() is quite small in the ecosystem (according to GitHub's Code Search, 16 usages in JavaScript and 5 in TypeScript outside of Node.js forks) all of them rely on it returning a string or to write a file successfully, none of the matches seem to compare against undefined or a falsy/truthy value.

In that case, I think it would be fine if we throw an exception.

Passing env instead of the isolate to WriteSnapshot() and calling env->ThrowErrnoException(errno, "function-name"); and returning false after the failing fopen/fclose calls should be enough:

node/src/heap_utils.cc

Lines 319 to 327 in d0c1176

bool WriteSnapshot(Isolate* isolate, const char* filename) {
FILE* fp = fopen(filename, "w");
if (fp == nullptr)
return false;
FileOutputStream stream(fp);
TakeSnapshot(isolate, &stream);
fclose(fp);
return true;
}

@kyranet
Copy link
Contributor Author

kyranet commented Jan 1, 2022

Thank you @RaisinTen, I opened #41373 following your indications.

@RaisinTen
Copy link
Contributor

We can close this now that #41373 has landed

@RaisinTen RaisinTen closed this Jan 12, 2022
@kyranet kyranet deleted the doc-v8/write-heap-snapshot-may-return-undefined branch January 12, 2022 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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