v8: make v8.writeHeapSnapshot() error codes consistent

This change makes the error codes returned by v8.writeHeapSnapshot() consistent across all platforms by using the libuv APIs instead of fopen(), fwrite() and fclose(). This also starts reporting potential errors that might happen during the write operations. Signed-off-by: Darshan Sen <[email protected]> PR-URL: #42577 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Author: RaisinTen

doc/api/v8.md

@@ -278,6 +278,9 @@ changes:
  - version: REPLACEME
  pr-url: https://github.com/nodejs/node/pull/41373
  description: An exception will now be thrown if the file could not be written.
+ - version: REPLACEME
+ pr-url: https://github.com/nodejs/node/pull/42577
+ description: Make the returned error codes consistent across all platforms.
 -->
 
 *`filename`{string} The file path where the V8 heap snapshot is to be

src/heap_utils.cc

@@ -5,6 +5,17 @@
 #include"stream_base-inl.h"
 #include"util-inl.h"
 
+// Copied from https://github.com/nodejs/node/blob/b07dc4d19fdbc15b4f76557dc45b3ce3a43ad0c3/src/util.cc#L36-L41.
+#ifdef _WIN32
+#include<io.h>// _S_IREAD _S_IWRITE
+#ifndef S_IRUSR
+#defineS_IRUSR _S_IREAD
+#endif// S_IRUSR
+#ifndef S_IWUSR
+#defineS_IWUSR _S_IWRITE
+#endif// S_IWUSR
+#endif
+
 using v8::Array;
 using v8::Boolean;
 using v8::Context;
@@ -16,8 +27,11 @@ using v8::Global;
 using v8::HandleScope;
 using v8::HeapSnapshot;
 using v8::Isolate;
+using v8::JustVoid;
 using v8::Local;
+using v8::Maybe;
 using v8::MaybeLocal;
+using v8::Nothing;
 using v8::Number;
 using v8::Object;
 using v8::ObjectTemplate;
@@ -206,26 +220,44 @@ void BuildEmbedderGraph(const FunctionCallbackInfo<Value>& args){
 namespace{
 classFileOutputStream : publicv8::OutputStream{
 public:
-explicitFileOutputStream(FILE* stream) : stream_(stream){}
+FileOutputStream(constint fd, uv_fs_t* req) : fd_(fd), req_(req){}
 
 intGetChunkSize() override{
 return65536; // big chunks == faster
  }
 
 voidEndOfStream() override{}
 
- WriteResult WriteAsciiChunk(char* data, int size) override{
-constsize_t len = static_cast<size_t>(size);
-size_t off = 0;
-
-while (off < len && !feof(stream_) && !ferror(stream_))
- off += fwrite(data + off, 1, len - off, stream_);
-
-return off == len ? kContinue : kAbort;
+ WriteResult WriteAsciiChunk(char* data, constint size) override{
+DCHECK_EQ(status_, 0);
+int offset = 0;
+while (offset < size){
+constuv_buf_t buf = uv_buf_init(data + offset, size - offset);
+constint num_bytes_written = uv_fs_write(nullptr,
+ req_,
+ fd_,
+ &buf,
+1,
+ -1,
+nullptr);
+uv_fs_req_cleanup(req_);
+if (num_bytes_written < 0){
+ status_ = num_bytes_written;
+returnkAbort;
+ }
+DCHECK_LE(num_bytes_written, buf.len);
+ offset += num_bytes_written;
+ }
+DCHECK_EQ(offset, size);
+returnkContinue;
  }
 
+intstatus() const{return status_}
+
 private:
- FILE* stream_;
+constint fd_;
+uv_fs_t* req_;
+int status_ = 0;
 };
 
 classHeapSnapshotStream : publicAsyncWrap,
@@ -316,19 +348,37 @@ inline void TakeSnapshot(Environment* env, v8::OutputStream* out){
 
 } // namespace
 
-boolWriteSnapshot(Environment* env, constchar* filename){
- FILE* fp = fopen(filename, "w");
-if (fp == nullptr){
- env->ThrowErrnoException(errno, "open");
-returnfalse;
+Maybe<void> WriteSnapshot(Environment* env, constchar* filename){
+uv_fs_t req;
+int err;
+
+constint fd = uv_fs_open(nullptr,
+ &req,
+ filename,
+ O_WRONLY | O_CREAT | O_TRUNC,
+ S_IWUSR | S_IRUSR,
+nullptr);
+uv_fs_req_cleanup(&req);
+if ((err = fd) < 0){
+ env->ThrowUVException(err, "open", nullptr, filename);
+return Nothing<void>();
  }
- FileOutputStream stream(fp);
+
+ FileOutputStream stream(fd, &req);
 TakeSnapshot(env, &stream);
-if (fclose(fp) == EOF){
- env->ThrowErrnoException(errno, "close");
-returnfalse;
+if ((err = stream.status()) < 0){
+ env->ThrowUVException(err, "write", nullptr, filename);
+returnNothing<void>();
  }
-returntrue;
+
+ err = uv_fs_close(nullptr, &req, fd, nullptr);
+uv_fs_req_cleanup(&req);
+if (err < 0){
+ env->ThrowUVException(err, "close", nullptr, filename);
+return Nothing<void>();
+ }
+
+returnJustVoid();
 }
 
 voidDeleteHeapSnapshot(const HeapSnapshot* snapshot){
@@ -379,7 +429,7 @@ void TriggerHeapSnapshot(const FunctionCallbackInfo<Value>& args){
 
 if (filename_v->IsUndefined()){
  DiagnosticFilename name(env, "Heap", "heapsnapshot");
-if (!WriteSnapshot(env, *name))
+if (WriteSnapshot(env, *name).IsNothing())
 return;
 if (String::NewFromUtf8(isolate, *name).ToLocal(&filename_v)){
  args.GetReturnValue().Set(filename_v);
@@ -389,7 +439,7 @@ void TriggerHeapSnapshot(const FunctionCallbackInfo<Value>& args){
 
  BufferValue path(isolate, filename_v);
 CHECK_NOT_NULL(*path);
-if (!WriteSnapshot(env, *path))
+if (WriteSnapshot(env, *path).IsNothing())
 return;
 return args.GetReturnValue().Set(filename_v);
 }

src/node_internals.h

@@ -379,7 +379,7 @@ class DiagnosticFilename{
 };
 
 namespaceheap{
-boolWriteSnapshot(Environment* env, constchar* filename);
+v8::Maybe<void>WriteSnapshot(Environment* env, constchar* filename);
 }
 
 classTraceEventScope{

test/sequential/test-heapdump.js

@@ -31,9 +31,7 @@ process.chdir(tmpdir.path);
 writeHeapSnapshot(directory);
 },(e)=>{
 assert.ok(e,'writeHeapSnapshot should error');
-// TODO(RaisinTen): This should throw EISDIR on Windows too.
-assert.strictEqual(e.code,
-process.platform==='win32' ? 'EACCES' : 'EISDIR');
+assert.strictEqual(e.code,'EISDIR');
 assert.strictEqual(e.syscall,'open');
 returntrue;
 });