From 0e8a73cb6bcf8dcb379717a84f624c5497efe19d Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Sat, 2 Apr 2022 19:03:34 +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 --- 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 b04e4f6228b15e..ca38d2ff703de2 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 83334fa52695a4..0a68077d5d9b24 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 5a33dad196dd95..07177f00ae3a81 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 9b45aa41992fd1..cb84bca4cd96da 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; });