From fa78aa51a37e482284c57f384d8dad728e9ed13d Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 11 Jan 2018 16:14:33 -0800 Subject: [PATCH] fs: encapsulate FSReqWrap more In further preparation for the Promises enabled fs API, further encapsulate FSReqWrap details. The intent here is to isolate, as much as possible, the functionality of the various fs functions from the details of how the results are notified. The promises implementation will use a `FSReqPromise` alternative to `FSReqWrap` that will use the same API so that both models can be used without changing any of the actual implementation details for the various methods. --- src/node_file.cc | 120 +++++++----------- src/node_file.h | 59 +++------ test/sequential/test-async-wrap-getasyncid.js | 3 +- 3 files changed, 67 insertions(+), 115 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index c9bbea1ee5f621..c8cdc7959153f5 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -94,6 +94,7 @@ using v8::MaybeLocal; using v8::Number; using v8::Object; using v8::String; +using v8::Undefined; using v8::Value; #ifndef MIN @@ -102,32 +103,16 @@ using v8::Value; #define GET_OFFSET(a) ((a)->IsNumber() ? (a)->IntegerValue() : -1) -FSReqWrap* FSReqWrap::New(Environment* env, - Local req, - const char* syscall, - const char* data, - enum encoding encoding, - Ownership ownership) { - const bool copy = (data != nullptr && ownership == COPY); - const size_t size = copy ? 1 + strlen(data) : 0; - FSReqWrap* that; - char* const storage = new char[sizeof(*that) + size]; - that = new(storage) FSReqWrap(env, req, syscall, data, encoding); - if (copy) - that->data_ = static_cast(memcpy(that->inline_data(), data, size)); - return that; +void FSReqWrap::Reject(Local reject) { + MakeCallback(env()->oncomplete_string(), 1, &reject); } - -void FSReqWrap::Dispose() { - this->~FSReqWrap(); - delete[] reinterpret_cast(this); +void FSReqWrap::FillStatsArray(const uv_stat_t* stat) { + node::FillStatsArray(env()->fs_stats_field_array(), stat); } - -void FSReqWrap::Reject(Local reject) { - Local argv[1] { reject }; - MakeCallback(env()->oncomplete_string(), arraysize(argv), argv); +void FSReqWrap::ResolveStat() { + Resolve(Undefined(env()->isolate())); } void FSReqWrap::Resolve(Local value) { @@ -138,9 +123,26 @@ void FSReqWrap::Resolve(Local value) { MakeCallback(env()->oncomplete_string(), arraysize(argv), argv); } +void FSReqWrap::Init(const char* syscall, + const char* data, + size_t len, + enum encoding encoding) { + syscall_ = syscall; + encoding_ = encoding; + + if (data != nullptr) { + CHECK_EQ(data_, nullptr); + buffer_.AllocateSufficientStorage(len + 1); + buffer_.SetLengthAndZeroTerminate(len); + memcpy(*buffer_, data, len); + data_ = *buffer_; + } +} + void NewFSReqWrap(const FunctionCallbackInfo& args) { CHECK(args.IsConstructCall()); - ClearWrap(args.This()); + Environment* env = Environment::GetCurrent(args.GetIsolate()); + new FSReqWrap(env, args.This()); } @@ -150,12 +152,11 @@ FSReqAfterScope::FSReqAfterScope(FSReqWrap* wrap, uv_fs_t* req) handle_scope_(wrap->env()->isolate()), context_scope_(wrap->env()->context()) { CHECK_EQ(wrap_->req(), req); - wrap_->ReleaseEarly(); // Free memory that's no longer used now. } FSReqAfterScope::~FSReqAfterScope() { uv_fs_req_cleanup(wrap_->req()); - wrap_->Dispose(); + delete wrap_; } // TODO(joyeecheung): create a normal context object, and @@ -195,12 +196,10 @@ void AfterNoArgs(uv_fs_t* req) { void AfterStat(uv_fs_t* req) { FSReqWrap* req_wrap = static_cast(req->data); FSReqAfterScope after(req_wrap, req); - Environment* env = req_wrap->env(); if (after.Proceed()) { - FillStatsArray(env->fs_stats_field_array(), - static_cast(req->ptr)); - req_wrap->Resolve(Undefined(req_wrap->env()->isolate())); + req_wrap->FillStatsArray(&req->statbuf); + req_wrap->ResolveStat(); } } @@ -222,7 +221,7 @@ void AfterStringPath(uv_fs_t* req) { if (after.Proceed()) { link = StringBytes::Encode(req_wrap->env()->isolate(), static_cast(req->path), - req_wrap->encoding_, + req_wrap->encoding(), &error); if (link.IsEmpty()) req_wrap->Reject(error); @@ -241,7 +240,7 @@ void AfterStringPtr(uv_fs_t* req) { if (after.Proceed()) { link = StringBytes::Encode(req_wrap->env()->isolate(), static_cast(req->ptr), - req_wrap->encoding_, + req_wrap->encoding(), &error); if (link.IsEmpty()) req_wrap->Reject(error); @@ -278,7 +277,7 @@ void AfterScanDir(uv_fs_t* req) { MaybeLocal filename = StringBytes::Encode(env->isolate(), ent.name, - req_wrap->encoding_, + req_wrap->encoding(), &error); if (filename.IsEmpty()) return req_wrap->Reject(error); @@ -317,12 +316,12 @@ class fs_req_wrap { template inline FSReqWrap* AsyncDestCall(Environment* env, const FunctionCallbackInfo& args, - const char* syscall, const char* dest, - enum encoding enc, FSReqWrap::Ownership ownership, - uv_fs_cb after, Func fn, Args... fn_args) { + const char* syscall, const char* dest, size_t len, + enum encoding enc, uv_fs_cb after, Func fn, Args... fn_args) { Local req = args[args.Length() - 1].As(); - FSReqWrap* req_wrap = FSReqWrap::New(env, req, - syscall, dest, enc, ownership); + FSReqWrap* req_wrap = Unwrap(req); + CHECK_NE(req_wrap, nullptr); + req_wrap->Init(syscall, dest, len, enc); int err = fn(env->event_loop(), req_wrap->req(), fn_args..., after); req_wrap->Dispatched(); if (err < 0) { @@ -339,38 +338,16 @@ inline FSReqWrap* AsyncDestCall(Environment* env, return req_wrap; } -// Defaults to COPY ownership. -template -inline FSReqWrap* AsyncDestCall(Environment* env, - const FunctionCallbackInfo& args, - const char* syscall, const char* dest, enum encoding enc, - uv_fs_cb after, Func fn, Args... fn_args) { - return AsyncDestCall(env, args, - syscall, dest, enc, FSReqWrap::COPY, - after, fn, fn_args...); -} - template inline FSReqWrap* AsyncCall(Environment* env, const FunctionCallbackInfo& args, - const char* syscall, enum encoding enc, FSReqWrap::Ownership ownership, + const char* syscall, enum encoding enc, uv_fs_cb after, Func fn, Args... fn_args) { return AsyncDestCall(env, args, - syscall, nullptr, enc, ownership, + syscall, nullptr, 0, enc, after, fn, fn_args...); } -// Defaults to COPY ownership. -template -inline FSReqWrap* AsyncCall(Environment* env, - const FunctionCallbackInfo& args, - const char* syscall, enum encoding enc, - uv_fs_cb after, Func fn, Args... fn_args) { - return AsyncCall(env, args, - syscall, enc, FSReqWrap::COPY, - after, fn, fn_args...); -} - // Template counterpart of SYNC_CALL, except that it only puts // the error number and the syscall in the context instead of // creating an error in the C++ land. @@ -623,8 +600,8 @@ static void Symlink(const FunctionCallbackInfo& args) { if (args[3]->IsObject()) { // symlink(target, path, flags, req) CHECK_EQ(args.Length(), 4); - AsyncDestCall(env, args, "symlink", *path, UTF8, AfterNoArgs, - uv_fs_symlink, *target, *path, flags); + AsyncDestCall(env, args, "symlink", *path, path.length(), UTF8, + AfterNoArgs, uv_fs_symlink, *target, *path, flags); } else { // symlink(target, path, flags) SYNC_DEST_CALL(symlink, *target, *path, *target, *path, flags) } @@ -643,8 +620,8 @@ static void Link(const FunctionCallbackInfo& args) { if (args[2]->IsObject()) { // link(src, dest, req) CHECK_EQ(args.Length(), 3); - AsyncDestCall(env, args, "link", *dest, UTF8, AfterNoArgs, - uv_fs_link, *src, *dest); + AsyncDestCall(env, args, "link", *dest, dest.length(), UTF8, + AfterNoArgs, uv_fs_link, *src, *dest); } else { // link(src, dest) SYNC_DEST_CALL(link, *src, *dest, *src, *dest) } @@ -695,8 +672,8 @@ static void Rename(const FunctionCallbackInfo& args) { if (args[2]->IsObject()) { CHECK_EQ(args.Length(), 3); - AsyncDestCall(env, args, "rename", *new_path, UTF8, AfterNoArgs, - uv_fs_rename, *old_path, *new_path); + AsyncDestCall(env, args, "rename", *new_path, new_path.length(), + UTF8, AfterNoArgs, uv_fs_rename, *old_path, *new_path); } else { SYNC_DEST_CALL(rename, *old_path, *new_path, *old_path, *new_path) } @@ -1041,7 +1018,6 @@ static void WriteString(const FunctionCallbackInfo& args) { char* buf = nullptr; int64_t pos; size_t len; - FSReqWrap::Ownership ownership = FSReqWrap::COPY; // will assign buf and len if string was external if (!StringBytes::GetExternalParts(string, @@ -1053,7 +1029,6 @@ static void WriteString(const FunctionCallbackInfo& args) { // StorageSize may return too large a char, so correct the actual length // by the write size len = StringBytes::Write(env->isolate(), buf, len, args[1], enc); - ownership = FSReqWrap::MOVE; } pos = GET_OFFSET(args[2]); @@ -1061,8 +1036,7 @@ static void WriteString(const FunctionCallbackInfo& args) { if (args[4]->IsObject()) { CHECK_EQ(args.Length(), 5); - AsyncCall(env, args, - "write", UTF8, ownership, AfterInteger, + AsyncCall(env, args, "write", UTF8, AfterInteger, uv_fs_write, fd, &uvbuf, 1, pos); } else { // SYNC_CALL returns on error. Make sure to always free the memory. @@ -1071,7 +1045,7 @@ static void WriteString(const FunctionCallbackInfo& args) { inline ~Delete() { delete[] pointer_; } char* const pointer_; }; - Delete delete_on_return(ownership == FSReqWrap::MOVE ? buf : nullptr); + Delete delete_on_return(buf); SYNC_CALL(write, nullptr, fd, &uvbuf, 1, pos) return args.GetReturnValue().Set(SYNC_RESULT); } @@ -1373,7 +1347,7 @@ void InitFs(Local target, Local wrapString = FIXED_ONE_BYTE_STRING(env->isolate(), "FSReqWrap"); fst->SetClassName(wrapString); - target->Set(wrapString, fst->GetFunction()); + target->Set(context, wrapString, fst->GetFunction()).FromJust(); } } // namespace fs diff --git a/src/node_file.h b/src/node_file.h index db85451b67a9df..878d02c8ba4989 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -17,60 +17,39 @@ using v8::Value; namespace fs { -class FSReqWrap: public ReqWrap { +class FSReqWrap : public ReqWrap { public: - enum Ownership { COPY, MOVE }; + FSReqWrap(Environment* env, Local req) + : ReqWrap(env, req, AsyncWrap::PROVIDER_FSREQWRAP) { + Wrap(object(), this); + } - inline static FSReqWrap* New(Environment* env, - Local req, - const char* syscall, - const char* data = nullptr, - enum encoding encoding = UTF8, - Ownership ownership = COPY); + virtual ~FSReqWrap() { + ClearWrap(object()); + } - inline void Dispose(); + void Init(const char* syscall, + const char* data = nullptr, + size_t len = 0, + enum encoding encoding = UTF8); + virtual void FillStatsArray(const uv_stat_t* stat); virtual void Reject(Local reject); virtual void Resolve(Local value); - - void ReleaseEarly() { - if (data_ != inline_data()) { - delete[] data_; - data_ = nullptr; - } - } + virtual void ResolveStat(); const char* syscall() const { return syscall_; } const char* data() const { return data_; } - const enum encoding encoding_; + enum encoding encoding() const { return encoding_; } size_t self_size() const override { return sizeof(*this); } - protected: - FSReqWrap(Environment* env, - Local req, - const char* syscall, - const char* data, - enum encoding encoding) - : ReqWrap(env, req, AsyncWrap::PROVIDER_FSREQWRAP), - encoding_(encoding), - syscall_(syscall), - data_(data) { - Wrap(object(), this); - } - - virtual ~FSReqWrap() { - ReleaseEarly(); - ClearWrap(object()); - } - - void* operator new(size_t size) = delete; - void* operator new(size_t size, char* storage) { return storage; } - char* inline_data() { return reinterpret_cast(this + 1); } - private: + enum encoding encoding_ = UTF8; const char* syscall_; - const char* data_; + + const char* data_ = nullptr; + MaybeStackBuffer buffer_; DISALLOW_COPY_AND_ASSIGN(FSReqWrap); }; diff --git a/test/sequential/test-async-wrap-getasyncid.js b/test/sequential/test-async-wrap-getasyncid.js index 58d6b7746985c5..f5b6843ef92fb9 100644 --- a/test/sequential/test-async-wrap-getasyncid.js +++ b/test/sequential/test-async-wrap-getasyncid.js @@ -112,9 +112,8 @@ if (common.hasCrypto) { // eslint-disable-line crypto-check const req = new FSReqWrap(); req.oncomplete = () => { }; - testUninitialized(req, 'FSReqWrap'); - binding.access(path.toNamespacedPath('../'), fs.F_OK, req); testInitialized(req, 'FSReqWrap'); + binding.access(path.toNamespacedPath('../'), fs.F_OK, req); const StatWatcher = binding.StatWatcher; testInitialized(new StatWatcher(), 'StatWatcher');