From 0187bc5cdcffdb80763792d32666d0814bb265fc Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Tue, 5 Apr 2022 19:25:51 +0530 Subject: [PATCH] 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 PR-URL: https://github.com/nodejs/node/pull/42577 Reviewed-By: James M Snell Reviewed-By: Joyee Cheung Reviewed-By: Anna Henningsen --- doc/api/v8.md | 3 + src/heap_utils.cc | 94 ++++++++++++++++++++++++-------- src/node_internals.h | 2 +- test/sequential/test-heapdump.js | 4 +- 4 files changed, 77 insertions(+), 26 deletions(-) diff --git a/doc/api/v8.md b/doc/api/v8.md index b04e4f6228b..ca38d2ff703 100644 --- a/doc/api/v8.md +++ b/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 diff --git a/src/heap_utils.cc b/src/heap_utils.cc index 83334fa5269..0a68077d5d9 100644 --- a/src/heap_utils.cc +++ b/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 // _S_IREAD _S_IWRITE +#ifndef S_IRUSR +#define S_IRUSR _S_IREAD +#endif // S_IRUSR +#ifndef S_IWUSR +#define S_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,7 +220,7 @@ void BuildEmbedderGraph(const FunctionCallbackInfo& args) { namespace { class FileOutputStream : public v8::OutputStream { public: - explicit FileOutputStream(FILE* stream) : stream_(stream) {} + FileOutputStream(const int fd, uv_fs_t* req) : fd_(fd), req_(req) {} int GetChunkSize() override { return 65536; // big chunks == faster @@ -214,18 +228,36 @@ class FileOutputStream : public v8::OutputStream { void EndOfStream() override {} - WriteResult WriteAsciiChunk(char* data, int size) override { - const size_t len = static_cast(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, const int size) override { + DCHECK_EQ(status_, 0); + int offset = 0; + while (offset < size) { + const uv_buf_t buf = uv_buf_init(data + offset, size - offset); + const int 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; + return kAbort; + } + DCHECK_LE(num_bytes_written, buf.len); + offset += num_bytes_written; + } + DCHECK_EQ(offset, size); + return kContinue; } + int status() const { return status_; } + private: - FILE* stream_; + const int fd_; + uv_fs_t* req_; + int status_ = 0; }; class HeapSnapshotStream : public AsyncWrap, @@ -316,19 +348,37 @@ inline void TakeSnapshot(Environment* env, v8::OutputStream* out) { } // namespace -bool WriteSnapshot(Environment* env, const char* filename) { - FILE* fp = fopen(filename, "w"); - if (fp == nullptr) { - env->ThrowErrnoException(errno, "open"); - return false; +Maybe WriteSnapshot(Environment* env, const char* filename) { + uv_fs_t req; + int err; + + const int 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(); } - FileOutputStream stream(fp); + + FileOutputStream stream(fd, &req); TakeSnapshot(env, &stream); - if (fclose(fp) == EOF) { - env->ThrowErrnoException(errno, "close"); - return false; + if ((err = stream.status()) < 0) { + env->ThrowUVException(err, "write", nullptr, filename); + return Nothing(); } - return true; + + err = uv_fs_close(nullptr, &req, fd, nullptr); + uv_fs_req_cleanup(&req); + if (err < 0) { + env->ThrowUVException(err, "close", nullptr, filename); + return Nothing(); + } + + return JustVoid(); } void DeleteHeapSnapshot(const HeapSnapshot* snapshot) { @@ -379,7 +429,7 @@ void TriggerHeapSnapshot(const FunctionCallbackInfo& 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& 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); } diff --git a/src/node_internals.h b/src/node_internals.h index 1d184954f0f..bf760f4d995 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -379,7 +379,7 @@ class DiagnosticFilename { }; namespace heap { -bool WriteSnapshot(Environment* env, const char* filename); +v8::Maybe WriteSnapshot(Environment* env, const char* filename); } class TraceEventScope { diff --git a/test/sequential/test-heapdump.js b/test/sequential/test-heapdump.js index 9b45aa41992..cb84bca4cd9 100644 --- a/test/sequential/test-heapdump.js +++ b/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'); return true; });