doc,v8: clarify `v8.writeHeapSnapshot` may return `undefined` by kyranet · Pull Request #41365 · nodejs/node · GitHub

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

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

v8.writeHeapSnapshot() calls the following code:

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.

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.

Suggested change

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

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

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

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; 
        
           }

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

We can close this now that #41373 has landed

doc,v8: clarify v8.writeHeapSnapshot may return undefined

2bdbaf6

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

RaisinTen requested changes Dec 31, 2021

View reviewed changes

kyranet mentioned this pull request Jan 1, 2022

fix,v8: make v8.writeHeapSnapshot throw if fopen/fclose fail #41373

Merged

RaisinTen closed this Jan 12, 2022

kyranet deleted the doc-v8/write-heap-snapshot-may-return-undefined branch January 12, 2022 18:56

                 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.

doc,v8: clarify `v8.writeHeapSnapshot` may return `undefined` by kyranet · Pull Request #41365 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

doc,v8: clarify v8.writeHeapSnapshot may return undefined#41365
kyranet wants to merge 1 commit into
nodejs:masterfrom
kyranet:doc-v8/write-heap-snapshot-may-return-undefined

kyranet commented Dec 31, 2021 •
edited
Loading

Uh oh!

Uh oh!

RaisinTen left a comment

Uh oh!

RaisinTen Dec 31, 2021

Uh oh!

addaleax commented Dec 31, 2021

Uh oh!

kyranet commented Dec 31, 2021 •
edited
Loading

Uh oh!

Uh oh!

RaisinTen commented Jan 1, 2022

Uh oh!

kyranet commented Jan 1, 2022

Uh oh!

RaisinTen commented Jan 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	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);
	}

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

doc,v8: clarify `v8.writeHeapSnapshot` may return `undefined` by kyranet · Pull Request #41365 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

doc,v8: clarify v8.writeHeapSnapshot may return undefined#41365kyranet wants to merge 1 commit intonodejs:masternodejs/node:masterfrom kyranet:doc-v8/write-heap-snapshot-may-return-undefinedkyranet/node:doc-v8/write-heap-snapshot-may-return-undefinedCopy head branch name to clipboard

Conversation

kyranet commented Dec 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RaisinTen left a comment

Choose a reason for hiding this comment

Uh oh!

RaisinTen Dec 31, 2021

Choose a reason for hiding this comment

Uh oh!

addaleax commented Dec 31, 2021

Uh oh!

kyranet commented Dec 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RaisinTen commented Jan 1, 2022

Uh oh!

kyranet commented Jan 1, 2022

Uh oh!

RaisinTen commented Jan 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

doc,v8: clarify v8.writeHeapSnapshot may return undefined#41365
kyranet wants to merge 1 commit into
nodejs:masterfrom
kyranet:doc-v8/write-heap-snapshot-may-return-undefined

kyranet commented Dec 31, 2021 •
edited
Loading

kyranet commented Dec 31, 2021 •
edited
Loading