diff --git a/doc/api/n-api.md b/doc/api/n-api.md index 739213c155755a..3748e252fc8964 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -886,6 +886,58 @@ If still valid, this API returns the `napi_value` representing the JavaScript `Object` associated with the `napi_ref`. Otherwise, result will be NULL. +### Cleanup on exit of the current Node.js instance + +While a Node.js process typically releases all its resources when exiting, +embedders of Node.js, or future Worker support, may require addons to register +clean-up hooks that will be run once the current Node.js instance exits. + +N-API provides functions for registering and un-registering such callbacks. +When those callbacks are run, all resources that are being held by the addon +should be freed up. + +#### napi_add_env_cleanup_hook + +```C +NODE_EXTERN napi_status napi_add_env_cleanup_hook(napi_env env, + void (*fun)(void* arg), + void* arg); +``` + +Registers `fun` as a function to be run with the `arg` parameter once the +current Node.js environment exits. + +A function can safely be specified multiple times with different +`arg` values. In that case, it will be called multiple times as well. +Providing the same `fun` and `arg` values multiple times is not allowed +and will lead the process to abort. + +The hooks will be called in reverse order, i.e. the most recently added one +will be called first. + +Removing this hook can be done by using `napi_remove_env_cleanup_hook`. +Typically, that happens when the resource for which this hook was added +is being torn down anyway. + +#### napi_remove_env_cleanup_hook + +```C +NAPI_EXTERN napi_status napi_remove_env_cleanup_hook(napi_env env, + void (*fun)(void* arg), + void* arg); +``` + +Unregisters `fun` as a function to be run with the `arg` parameter once the +current Node.js environment exits. Both the argument and the function value +need to be exact matches. + +The function must have originally been registered +with `napi_add_env_cleanup_hook`, otherwise the process will abort. + ## Module registration N-API modules are registered in a manner similar to other modules except that instead of using the `NODE_MODULE` macro the following diff --git a/lib/internal/process/stdio.js b/lib/internal/process/stdio.js index 75ede6a8e7e157..ce84142938f066 100644 --- a/lib/internal/process/stdio.js +++ b/lib/internal/process/stdio.js @@ -77,13 +77,15 @@ function setupStdio() { stdin = new net.Socket({ handle: process.channel, readable: true, - writable: false + writable: false, + manualStart: true }); } else { stdin = new net.Socket({ fd: fd, readable: true, - writable: false + writable: false, + manualStart: true }); } // Make sure the stdin can't be `.end()`-ed @@ -107,15 +109,22 @@ function setupStdio() { stdin._handle.readStop(); } - // if the user calls stdin.pause(), then we need to stop reading - // immediately, so that the process can close down. + // If the user calls stdin.pause(), then we need to stop reading + // once the stream implementation does so (one nextTick later), + // so that the process can close down. stdin.on('pause', () => { + process.nextTick(onpause); + }); + + function onpause() { if (!stdin._handle) return; - stdin._readableState.reading = false; - stdin._handle.reading = false; - stdin._handle.readStop(); - }); + if (stdin._handle.reading && !stdin._readableState.flowing) { + stdin._readableState.reading = false; + stdin._handle.reading = false; + stdin._handle.readStop(); + } + } return stdin; } diff --git a/src/async_wrap.cc b/src/async_wrap.cc index f7a6d4e68dd483..b20e2b746f7f89 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -141,6 +141,7 @@ static void DestroyAsyncIdsCallback(Environment* env, void* data) { do { std::vector destroy_async_id_list; destroy_async_id_list.swap(*env->destroy_async_id_list()); + if (!env->can_call_into_js()) return; for (auto async_id : destroy_async_id_list) { // Want each callback to be cleaned up after itself, instead of cleaning // them all up after the while() loop completes. @@ -166,7 +167,7 @@ void Emit(Environment* env, double async_id, AsyncHooks::Fields type, Local fn) { AsyncHooks* async_hooks = env->async_hooks(); - if (async_hooks->fields()[type] == 0) + if (async_hooks->fields()[type] == 0 || !env->can_call_into_js()) return; v8::HandleScope handle_scope(env->isolate()); @@ -625,8 +626,10 @@ void AsyncWrap::EmitTraceEventDestroy() { } void AsyncWrap::EmitDestroy(Environment* env, double async_id) { - if (env->async_hooks()->fields()[AsyncHooks::kDestroy] == 0) + if (env->async_hooks()->fields()[AsyncHooks::kDestroy] == 0 || + !env->can_call_into_js()) { return; + } if (env->destroy_async_id_list()->empty()) { env->SetUnrefImmediate(DestroyAsyncIdsCallback, nullptr); diff --git a/src/base_object-inl.h b/src/base_object-inl.h index 786e1f26b48256..3bd854639b2c6d 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -37,10 +37,13 @@ BaseObject::BaseObject(Environment* env, v8::Local object) CHECK_EQ(false, object.IsEmpty()); CHECK_GT(object->InternalFieldCount(), 0); object->SetAlignedPointerInInternalField(0, static_cast(this)); + env_->AddCleanupHook(DeleteMe, static_cast(this)); } BaseObject::~BaseObject() { + env_->RemoveCleanupHook(DeleteMe, static_cast(this)); + if (persistent_handle_.IsEmpty()) { // This most likely happened because the weak callback below cleared it. return; @@ -80,6 +83,12 @@ T* BaseObject::FromJSObject(v8::Local object) { } +void BaseObject::DeleteMe(void* data) { + BaseObject* self = static_cast(data); + delete self; +} + + void BaseObject::MakeWeak() { persistent_handle_.SetWeak( this, diff --git a/src/base_object.h b/src/base_object.h index 2a4967c1aaf303..e0b60843401681 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -71,6 +71,8 @@ class BaseObject { private: BaseObject(); + static inline void DeleteMe(void* data); + // persistent_handle_ needs to be at a fixed offset from the start of the // class because it is used by src/node_postmortem_metadata.cc to calculate // offsets and generate debug symbols for BaseObject, which assumes that the diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 4208c02d4fe445..f829eb2b01d5c9 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -267,9 +267,8 @@ void ares_poll_cb(uv_poll_t* watcher, int status, int events) { } -void ares_poll_close_cb(uv_handle_t* watcher) { - node_ares_task* task = ContainerOf(&node_ares_task::poll_watcher, - reinterpret_cast(watcher)); +void ares_poll_close_cb(uv_poll_t* watcher) { + node_ares_task* task = ContainerOf(&node_ares_task::poll_watcher, watcher); free(task); } @@ -347,8 +346,7 @@ void ares_sockstate_cb(void* data, "When an ares socket is closed we should have a handle for it"); channel->task_list()->erase(it); - uv_close(reinterpret_cast(&task->poll_watcher), - ares_poll_close_cb); + channel->env()->CloseHandle(&task->poll_watcher, ares_poll_close_cb); if (channel->task_list()->empty()) { uv_timer_stop(channel->timer_handle()); @@ -517,10 +515,7 @@ ChannelWrap::~ChannelWrap() { void ChannelWrap::CleanupTimer() { if (timer_handle_ == nullptr) return; - uv_close(reinterpret_cast(timer_handle_), - [](uv_handle_t* handle) { - delete reinterpret_cast(handle); - }); + env()->CloseHandle(timer_handle_, [](uv_timer_t* handle) { delete handle; }); timer_handle_ = nullptr; } @@ -610,8 +605,7 @@ class QueryWrap : public AsyncWrap { static_cast(this)); } - static void CaresAsyncClose(uv_handle_t* handle) { - uv_async_t* async = reinterpret_cast(handle); + static void CaresAsyncClose(uv_async_t* async) { auto data = static_cast(async->data); delete data->wrap; delete data; @@ -636,7 +630,7 @@ class QueryWrap : public AsyncWrap { free(host); } - uv_close(reinterpret_cast(handle), CaresAsyncClose); + wrap->env()->CloseHandle(handle, CaresAsyncClose); } static void Callback(void *arg, int status, int timeouts, @@ -1933,13 +1927,11 @@ void GetAddrInfo(const FunctionCallbackInfo& args) { hints.ai_socktype = SOCK_STREAM; hints.ai_flags = flags; - int err = uv_getaddrinfo(env->event_loop(), - req_wrap->req(), - AfterGetAddrInfo, - *hostname, - nullptr, - &hints); - req_wrap->Dispatched(); + int err = req_wrap->Dispatch(uv_getaddrinfo, + AfterGetAddrInfo, + *hostname, + nullptr, + &hints); if (err) delete req_wrap; @@ -1963,12 +1955,10 @@ void GetNameInfo(const FunctionCallbackInfo& args) { GetNameInfoReqWrap* req_wrap = new GetNameInfoReqWrap(env, req_wrap_obj); - int err = uv_getnameinfo(env->event_loop(), - req_wrap->req(), - AfterGetNameInfo, - (struct sockaddr*)&addr, - NI_NAMEREQD); - req_wrap->Dispatched(); + int err = req_wrap->Dispatch(uv_getnameinfo, + AfterGetNameInfo, + reinterpret_cast(&addr), + NI_NAMEREQD); if (err) delete req_wrap; diff --git a/src/connection_wrap.h b/src/connection_wrap.h index afb168c614aa97..72030a00901daf 100644 --- a/src/connection_wrap.h +++ b/src/connection_wrap.h @@ -23,8 +23,6 @@ class ConnectionWrap : public LibuvStreamWrap { ConnectionWrap(Environment* env, v8::Local object, ProviderType provider); - ~ConnectionWrap() { - } UVType handle_; }; diff --git a/src/env-inl.h b/src/env-inl.h index 6202e50548a3ce..0268879f5c7823 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -349,8 +349,35 @@ inline void Environment::RegisterHandleCleanup(uv_handle_t* handle, handle_cleanup_queue_.push_back(HandleCleanup{handle, cb, arg}); } -inline void Environment::FinishHandleCleanup(uv_handle_t* handle) { - handle_cleanup_waiting_--; +template +inline void Environment::CloseHandle(T* handle, OnCloseCallback callback) { + handle_cleanup_waiting_++; + static_assert(sizeof(T) >= sizeof(uv_handle_t), "T is a libuv handle"); + static_assert(offsetof(T, data) == offsetof(uv_handle_t, data), + "T is a libuv handle"); + static_assert(offsetof(T, close_cb) == offsetof(uv_handle_t, close_cb), + "T is a libuv handle"); + struct CloseData { + Environment* env; + OnCloseCallback callback; + void* original_data; + }; + handle->data = new CloseData { this, callback, handle->data }; + uv_close(reinterpret_cast(handle), [](uv_handle_t* handle) { + std::unique_ptr data { static_cast(handle->data) }; + data->env->handle_cleanup_waiting_--; + handle->data = data->original_data; + data->callback(reinterpret_cast(handle)); + }); +} + +void Environment::IncreaseWaitingRequestCounter() { + request_waiting_++; +} + +void Environment::DecreaseWaitingRequestCounter() { + request_waiting_--; + CHECK_GE(request_waiting_, 0); } inline uv_loop_t* Environment::event_loop() const { @@ -532,6 +559,14 @@ void Environment::SetUnrefImmediate(native_immediate_callback cb, CreateImmediate(cb, data, obj, false); } +inline bool Environment::can_call_into_js() const { + return can_call_into_js_; +} + +inline void Environment::set_can_call_into_js(bool can_call_into_js) { + can_call_into_js_ = can_call_into_js; +} + inline performance::performance_state* Environment::performance_state() { return performance_state_.get(); } @@ -629,6 +664,29 @@ inline void Environment::SetTemplateMethod(v8::Local that, t->SetClassName(name_string); // NODE_SET_METHOD() compatibility. } +void Environment::AddCleanupHook(void (*fn)(void*), void* arg) { + auto insertion_info = cleanup_hooks_.emplace(CleanupHookCallback { + fn, arg, cleanup_hook_counter_++ + }); + // Make sure there was no existing element with these values. + CHECK_EQ(insertion_info.second, true); +} + +void Environment::RemoveCleanupHook(void (*fn)(void*), void* arg) { + CleanupHookCallback search { fn, arg, 0 }; + cleanup_hooks_.erase(search); +} + +size_t Environment::CleanupHookCallback::Hash::operator()( + const CleanupHookCallback& cb) const { + return std::hash()(cb.arg_); +} + +bool Environment::CleanupHookCallback::Equal::operator()( + const CleanupHookCallback& a, const CleanupHookCallback& b) const { + return a.fn_ == b.fn_ && a.arg_ == b.arg_; +} + #define VP(PropertyName, StringValue) V(v8::Private, PropertyName) #define VS(PropertyName, StringValue) V(v8::String, PropertyName) #define V(TypeName, PropertyName) \ diff --git a/src/env.cc b/src/env.cc index 08d719a51011d1..ab5de3e2ee1b1d 100644 --- a/src/env.cc +++ b/src/env.cc @@ -107,7 +107,6 @@ Environment::Environment(IsolateData* isolate_data, #if HAVE_INSPECTOR inspector_agent_(new inspector::Agent(this)), #endif - handle_cleanup_waiting_(0), http_parser_buffer_(nullptr), fs_stats_field_array_(isolate_, kFsStatsFieldsLength * 2), context_(context->GetIsolate(), context) { @@ -134,6 +133,10 @@ Environment::Environment(IsolateData* isolate_data, } Environment::~Environment() { + // Make sure there are no re-used libuv wrapper objects. + // CleanupHandles() should have removed all of them. + CHECK(file_handle_read_wrap_freelist_.empty()); + v8::HandleScope handle_scope(isolate()); #if HAVE_INSPECTOR @@ -209,9 +212,7 @@ void Environment::RegisterHandleCleanups() { void* arg) { handle->data = env; - uv_close(handle, [](uv_handle_t* handle) { - static_cast(handle->data)->FinishHandleCleanup(handle); - }); + env->CloseHandle(handle, [](uv_handle_t* handle) {}); }; RegisterHandleCleanup( @@ -233,14 +234,23 @@ void Environment::RegisterHandleCleanups() { } void Environment::CleanupHandles() { - for (HandleCleanup& hc : handle_cleanup_queue_) { - handle_cleanup_waiting_++; + for (ReqWrap* request : req_wrap_queue_) + request->Cancel(); + + for (HandleWrap* handle : handle_wrap_queue_) + handle->Close(); + + for (HandleCleanup& hc : handle_cleanup_queue_) hc.cb_(this, hc.handle_, hc.arg_); - } handle_cleanup_queue_.clear(); - while (handle_cleanup_waiting_ != 0) + while (handle_cleanup_waiting_ != 0 || + request_waiting_ != 0 || + !handle_wrap_queue_.IsEmpty()) { uv_run(event_loop(), UV_RUN_ONCE); + } + + file_handle_read_wrap_freelist_.clear(); } void Environment::StartProfilerIdleNotifier() { @@ -305,6 +315,37 @@ void Environment::PrintSyncTrace() const { fflush(stderr); } +void Environment::RunCleanup() { + CleanupHandles(); + + while (!cleanup_hooks_.empty()) { + // Copy into a vector, since we can't sort an unordered_set in-place. + std::vector callbacks( + cleanup_hooks_.begin(), cleanup_hooks_.end()); + // We can't erase the copied elements from `cleanup_hooks_` yet, because we + // need to be able to check whether they were un-scheduled by another hook. + + std::sort(callbacks.begin(), callbacks.end(), + [](const CleanupHookCallback& a, const CleanupHookCallback& b) { + // Sort in descending order so that the most recently inserted callbacks + // are run first. + return a.insertion_order_counter_ > b.insertion_order_counter_; + }); + + for (const CleanupHookCallback& cb : callbacks) { + if (cleanup_hooks_.count(cb) == 0) { + // This hook was removed from the `cleanup_hooks_` set during another + // hook that was run earlier. Nothing to do here. + continue; + } + + cb.fn_(cb.arg_); + cleanup_hooks_.erase(cb); + } + CleanupHandles(); + } +} + void Environment::RunBeforeExitCallbacks() { for (ExitCallback before_exit : before_exit_functions_) { before_exit.cb_(before_exit.arg_); diff --git a/src/env.h b/src/env.h index c0d79883d0ff1c..15d417ba606860 100644 --- a/src/env.h +++ b/src/env.h @@ -42,6 +42,7 @@ #include #include #include +#include struct nghttp2_rcbuf; @@ -576,10 +577,14 @@ class Environment { void RegisterHandleCleanups(); void CleanupHandles(); + + // Register clean-up cb to be called on environment destruction. inline void RegisterHandleCleanup(uv_handle_t* handle, HandleCleanupCb cb, void *arg); - inline void FinishHandleCleanup(uv_handle_t* handle); + + template + inline void CloseHandle(T* handle, OnCloseCallback callback); inline void AssignToContext(v8::Local context, const ContextInfo& info); @@ -596,6 +601,9 @@ class Environment { inline uv_check_t* immediate_check_handle(); inline uv_idle_t* immediate_idle_handle(); + inline void IncreaseWaitingRequestCounter(); + inline void DecreaseWaitingRequestCounter(); + inline AsyncHooks* async_hooks(); inline ImmediateInfo* immediate_info(); inline TickInfo* tick_info(); @@ -671,6 +679,12 @@ class Environment { const char* path = nullptr, const char* dest = nullptr); + // If this flag is set, calls into JS (if they would be observable + // from userland) must be avoided. This flag does not indicate whether + // calling into JS is allowed from a VM perspective at this point. + inline bool can_call_into_js() const; + inline void set_can_call_into_js(bool can_call_into_js); + inline void ThrowError(const char* errmsg); inline void ThrowTypeError(const char* errmsg); inline void ThrowRangeError(const char* errmsg); @@ -775,6 +789,10 @@ class Environment { v8::Local GetNow(); + inline void AddCleanupHook(void (*fn)(void*), void* arg); + inline void RemoveCleanupHook(void (*fn)(void*), void* arg); + void RunCleanup(); + private: inline void CreateImmediate(native_immediate_callback cb, void* data, @@ -809,6 +827,7 @@ class Environment { std::unique_ptr performance_state_; std::unordered_map performance_marks_; + bool can_call_into_js_ = true; #if HAVE_INSPECTOR std::unique_ptr inspector_agent_; @@ -824,7 +843,8 @@ class Environment { HandleWrapQueue handle_wrap_queue_; ReqWrapQueue req_wrap_queue_; std::list handle_cleanup_queue_; - int handle_cleanup_waiting_; + int handle_cleanup_waiting_ = 0; + int request_waiting_ = 0; double* heap_statistics_buffer_ = nullptr; double* heap_space_statistics_buffer_ = nullptr; @@ -863,6 +883,32 @@ class Environment { void RunAndClearNativeImmediates(); static void CheckImmediate(uv_check_t* handle); + struct CleanupHookCallback { + void (*fn_)(void*); + void* arg_; + + // We keep track of the insertion order for these objects, so that we can + // call the callbacks in reverse order when we are cleaning up. + uint64_t insertion_order_counter_; + + // Only hashes `arg_`, since that is usually enough to identify the hook. + struct Hash { + inline size_t operator()(const CleanupHookCallback& cb) const; + }; + + // Compares by `fn_` and `arg_` being equal. + struct Equal { + inline bool operator()(const CleanupHookCallback& a, + const CleanupHookCallback& b) const; + }; + }; + + // Use an unordered_set, so that we have efficient insertion and removal. + std::unordered_set cleanup_hooks_; + uint64_t cleanup_hook_counter_ = 0; + static void EnvPromiseHook(v8::PromiseHookType type, v8::Local promise, v8::Local parent); diff --git a/src/fs_event_wrap.cc b/src/fs_event_wrap.cc index ed74f36719db79..579e446fc5c485 100644 --- a/src/fs_event_wrap.cc +++ b/src/fs_event_wrap.cc @@ -78,11 +78,12 @@ FSEventWrap::FSEventWrap(Environment* env, Local object) : HandleWrap(env, object, reinterpret_cast(&handle_), - AsyncWrap::PROVIDER_FSEVENTWRAP) {} + AsyncWrap::PROVIDER_FSEVENTWRAP) { + MarkAsUninitialized(); +} FSEventWrap::~FSEventWrap() { - CHECK_EQ(initialized_, false); } void FSEventWrap::GetInitialized(const FunctionCallbackInfo& args) { @@ -153,6 +154,7 @@ void FSEventWrap::Start(const FunctionCallbackInfo& args) { } err = uv_fs_event_start(&wrap->handle_, OnEvent, *path, flags); + wrap->MarkAsInitialized(); wrap->initialized_ = true; if (err != 0) { diff --git a/src/handle_wrap.cc b/src/handle_wrap.cc index 49bf0c55bea0a1..20356b94a5775a 100644 --- a/src/handle_wrap.cc +++ b/src/handle_wrap.cc @@ -61,29 +61,40 @@ void HandleWrap::HasRef(const FunctionCallbackInfo& args) { void HandleWrap::Close(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - HandleWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); - // Guard against uninitialized handle or double close. - if (!IsAlive(wrap)) - return; + wrap->Close(args[0]); +} - if (wrap->state_ != kInitialized) +void HandleWrap::Close(v8::Local close_callback) { + if (state_ != kInitialized) return; - CHECK_EQ(false, wrap->persistent().IsEmpty()); - uv_close(wrap->handle_, OnClose); - wrap->state_ = kClosing; + CHECK_EQ(false, persistent().IsEmpty()); + uv_close(handle_, OnClose); + state_ = kClosing; - if (args[0]->IsFunction()) { - wrap->object()->Set(env->onclose_string(), args[0]); - wrap->state_ = kClosingWithCallback; + if (!close_callback.IsEmpty() && close_callback->IsFunction()) { + object()->Set(env()->context(), env()->onclose_string(), close_callback) + .FromJust(); + state_ = kClosingWithCallback; } } +void HandleWrap::MarkAsInitialized() { + env()->handle_wrap_queue()->PushBack(this); + state_ = kInitialized; +} + + +void HandleWrap::MarkAsUninitialized() { + handle_wrap_queue_.Remove(); + state_ = kClosed; +} + + HandleWrap::HandleWrap(Environment* env, Local object, uv_handle_t* handle, @@ -110,6 +121,8 @@ void HandleWrap::OnClose(uv_handle_t* handle) { const bool have_close_callback = (wrap->state_ == kClosingWithCallback); wrap->state_ = kClosed; + wrap->OnClose(); + if (have_close_callback) wrap->MakeCallback(env->onclose_string(), 0, nullptr); diff --git a/src/handle_wrap.h b/src/handle_wrap.h index e7a335f5140253..b2b09f5010d1f7 100644 --- a/src/handle_wrap.h +++ b/src/handle_wrap.h @@ -70,11 +70,18 @@ class HandleWrap : public AsyncWrap { inline uv_handle_t* GetHandle() const { return handle_; } + virtual void Close( + v8::Local close_callback = v8::Local()); + protected: HandleWrap(Environment* env, v8::Local object, uv_handle_t* handle, AsyncWrap::ProviderType provider); + virtual void OnClose() {} + + void MarkAsInitialized(); + void MarkAsUninitialized(); private: friend class Environment; diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 4e0c04a7b95527..391d3bc037927e 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -576,6 +576,8 @@ std::unique_ptr Agent::Connect( void Agent::WaitForDisconnect() { CHECK_NE(client_, nullptr); + // TODO(addaleax): Maybe this should use an at-exit hook for the Environment + // or something similar? client_->contextDestroyed(parent_env_->context()); if (io_ != nullptr) { io_->WaitForDisconnect(); diff --git a/src/node.cc b/src/node.cc index d71303570b1a56..0a1b35bfc591ca 100644 --- a/src/node.cc +++ b/src/node.cc @@ -903,6 +903,22 @@ void AddPromiseHook(v8::Isolate* isolate, promise_hook_func fn, void* arg) { env->AddPromiseHook(fn, arg); } +void AddEnvironmentCleanupHook(v8::Isolate* isolate, + void (*fun)(void* arg), + void* arg) { + Environment* env = Environment::GetCurrent(isolate); + env->AddCleanupHook(fun, arg); +} + + +void RemoveEnvironmentCleanupHook(v8::Isolate* isolate, + void (*fun)(void* arg), + void* arg) { + Environment* env = Environment::GetCurrent(isolate); + env->RemoveCleanupHook(fun, arg); +} + + CallbackScope::CallbackScope(Isolate* isolate, Local object, async_context asyncContext) @@ -937,6 +953,11 @@ InternalCallbackScope::InternalCallbackScope(Environment* env, CHECK(!object.IsEmpty()); } + if (!env->can_call_into_js()) { + failed_ = true; + return; + } + HandleScope handle_scope(env->isolate()); // If you hit this assertion, you forgot to enter the v8::Context first. CHECK_EQ(Environment::GetCurrent(env->isolate()), env); @@ -980,6 +1001,7 @@ void InternalCallbackScope::Close() { Environment::TickInfo* tick_info = env_->tick_info(); + if (!env_->can_call_into_js()) return; if (!tick_info->has_scheduled()) { env_->isolate()->RunMicrotasks(); } @@ -997,6 +1019,8 @@ void InternalCallbackScope::Close() { Local process = env_->process_object(); + if (!env_->can_call_into_js()) return; + if (env_->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) { env_->tick_info()->set_has_thrown(true); failed_ = true; @@ -4434,7 +4458,7 @@ Environment* CreateEnvironment(IsolateData* isolate_data, void FreeEnvironment(Environment* env) { - env->CleanupHandles(); + env->RunCleanup(); delete env; } @@ -4532,11 +4556,15 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, env.set_trace_sync_io(false); const int exit_code = EmitExit(&env); + + WaitForInspectorDisconnect(&env); + + env.set_can_call_into_js(false); + env.RunCleanup(); RunAtExit(&env); v8_platform.DrainVMTasks(isolate); v8_platform.CancelVMTasks(isolate); - WaitForInspectorDisconnect(&env); #if defined(LEAK_SANITIZER) __lsan_do_leak_check(); #endif diff --git a/src/node.h b/src/node.h index 5a491c1abf5457..23e2e9995ce209 100644 --- a/src/node.h +++ b/src/node.h @@ -583,6 +583,19 @@ NODE_EXTERN void AddPromiseHook(v8::Isolate* isolate, promise_hook_func fn, void* arg); +/* This is a lot like node::AtExit, except that the hooks added via this + * function are run before the AtExit ones and will always be registered + * for the current Environment instance. + * These functions are safe to use in an addon supporting multiple + * threads/isolates. */ +NODE_EXTERN void AddEnvironmentCleanupHook(v8::Isolate* isolate, + void (*fun)(void* arg), + void* arg); + +NODE_EXTERN void RemoveEnvironmentCleanupHook(v8::Isolate* isolate, + void (*fun)(void* arg), + void* arg); + /* Returns the id of the current execution context. If the return value is * zero then no execution has been set. This will happen if the user handles * I/O from native code. */ diff --git a/src/node_api.cc b/src/node_api.cc index 4878cf241edc04..b456ade2d4ca74 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -902,6 +902,28 @@ void napi_module_register(napi_module* mod) { node::node_module_register(nm); } +napi_status napi_add_env_cleanup_hook(napi_env env, + void (*fun)(void* arg), + void* arg) { + CHECK_ENV(env); + CHECK_ARG(env, fun); + + node::AddEnvironmentCleanupHook(env->isolate, fun, arg); + + return napi_ok; +} + +napi_status napi_remove_env_cleanup_hook(napi_env env, + void (*fun)(void* arg), + void* arg) { + CHECK_ENV(env); + CHECK_ARG(env, fun); + + node::RemoveEnvironmentCleanupHook(env->isolate, fun, arg); + + return napi_ok; +} + // Warning: Keep in-sync with napi_status enum static const char* error_messages[] = {nullptr, @@ -3316,7 +3338,7 @@ static napi_status ConvertUVErrorCode(int code) { } // Wrapper around uv_work_t which calls user-provided callbacks. -class Work : public node::AsyncResource { +class Work : public node::AsyncResource, public node::ThreadPoolWork { private: explicit Work(napi_env env, v8::Local async_resource, @@ -3327,15 +3349,14 @@ class Work : public node::AsyncResource { : AsyncResource(env->isolate, async_resource, *v8::String::Utf8Value(env->isolate, async_resource_name)), - _env(env), - _data(data), - _execute(execute), - _complete(complete) { - memset(&_request, 0, sizeof(_request)); - _request.data = this; + ThreadPoolWork(node::Environment::GetCurrent(env->isolate)), + _env(env), + _data(data), + _execute(execute), + _complete(complete) { } - ~Work() { } + virtual ~Work() { } public: static Work* New(napi_env env, @@ -3352,44 +3373,36 @@ class Work : public node::AsyncResource { delete work; } - static void ExecuteCallback(uv_work_t* req) { - Work* work = static_cast(req->data); - work->_execute(work->_env, work->_data); + void DoThreadPoolWork() override { + _execute(_env, _data); } - static void CompleteCallback(uv_work_t* req, int status) { - Work* work = static_cast(req->data); + void AfterThreadPoolWork(int status) { + if (_complete == nullptr) + return; - if (work->_complete != nullptr) { - napi_env env = work->_env; + // Establish a handle scope here so that every callback doesn't have to. + // Also it is needed for the exception-handling below. + v8::HandleScope scope(_env->isolate); - // Establish a handle scope here so that every callback doesn't have to. - // Also it is needed for the exception-handling below. - v8::HandleScope scope(env->isolate); - CallbackScope callback_scope(work); + CallbackScope callback_scope(this); - NAPI_CALL_INTO_MODULE(env, - work->_complete(env, ConvertUVErrorCode(status), work->_data), - [env] (v8::Local local_err) { - // If there was an unhandled exception in the complete callback, - // report it as a fatal exception. (There is no JavaScript on the - // callstack that can possibly handle it.) - v8impl::trigger_fatal_exception(env, local_err); - }); - - // Note: Don't access `work` after this point because it was - // likely deleted by the complete callback. - } - } + NAPI_CALL_INTO_MODULE(_env, + _complete(_env, ConvertUVErrorCode(status), _data), + [this] (v8::Local local_err) { + // If there was an unhandled exception in the complete callback, + // report it as a fatal exception. (There is no JavaScript on the + // callstack that can possibly handle it.) + v8impl::trigger_fatal_exception(_env, local_err); + }); - uv_work_t* Request() { - return &_request; + // Note: Don't access `work` after this point because it was + // likely deleted by the complete callback. } private: napi_env _env; void* _data; - uv_work_t _request; napi_async_execute_callback _execute; napi_async_complete_callback _complete; }; @@ -3466,10 +3479,7 @@ napi_status napi_queue_async_work(napi_env env, napi_async_work work) { uvimpl::Work* w = reinterpret_cast(work); - CALL_UV(env, uv_queue_work(event_loop, - w->Request(), - uvimpl::Work::ExecuteCallback, - uvimpl::Work::CompleteCallback)); + w->ScheduleWork(); return napi_clear_last_error(env); } @@ -3480,7 +3490,7 @@ napi_status napi_cancel_async_work(napi_env env, napi_async_work work) { uvimpl::Work* w = reinterpret_cast(work); - CALL_UV(env, uv_cancel(reinterpret_cast(w->Request()))); + CALL_UV(env, w->CancelWork()); return napi_clear_last_error(env); } diff --git a/src/node_api.h b/src/node_api.h index b010d32db7b086..91c2775a03ed76 100644 --- a/src/node_api.h +++ b/src/node_api.h @@ -118,6 +118,13 @@ EXTERN_C_START NAPI_EXTERN void napi_module_register(napi_module* mod); +NAPI_EXTERN napi_status napi_add_env_cleanup_hook(napi_env env, + void (*fun)(void* arg), + void* arg); +NAPI_EXTERN napi_status napi_remove_env_cleanup_hook(napi_env env, + void (*fun)(void* arg), + void* arg); + NAPI_EXTERN napi_status napi_get_last_error_info(napi_env env, const napi_extended_error_info** result); diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 4db82d4d7ca3e9..23582454cd6f67 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -820,6 +820,8 @@ class ContextifyScript : public BaseObject { const bool display_errors, const bool break_on_sigint, const FunctionCallbackInfo& args) { + if (!env->can_call_into_js()) + return false; if (!ContextifyScript::InstanceOf(env, args.Holder())) { env->ThrowTypeError( "Script methods can only be called on script instances."); diff --git a/src/node_crypto.cc b/src/node_crypto.cc index f611f81f16a819..8235b8b01ca4c6 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -4556,7 +4556,7 @@ bool ECDH::IsKeyPairValid() { } -class PBKDF2Request : public AsyncWrap { +class PBKDF2Request : public AsyncWrap, public ThreadPoolWork { public: PBKDF2Request(Environment* env, Local object, @@ -4566,6 +4566,7 @@ class PBKDF2Request : public AsyncWrap { int keylen, int iteration_count) : AsyncWrap(env, object, AsyncWrap::PROVIDER_PBKDF2REQUEST), + ThreadPoolWork(env), digest_(digest), success_(false), pass_(std::move(pass)), @@ -4574,21 +4575,14 @@ class PBKDF2Request : public AsyncWrap { iteration_count_(iteration_count) { } - uv_work_t* work_req() { - return &work_req_; - } - size_t self_size() const override { return sizeof(*this); } - static void Work(uv_work_t* work_req); - void Work(); + void DoThreadPoolWork() override; + void AfterThreadPoolWork(int status) override; - static void After(uv_work_t* work_req, int status); void After(Local (*argv)[2]); - void After(); private: - uv_work_t work_req_; const EVP_MD* digest_; bool success_; MallocedBuffer pass_; @@ -4598,7 +4592,7 @@ class PBKDF2Request : public AsyncWrap { }; -void PBKDF2Request::Work() { +void PBKDF2Request::DoThreadPoolWork() { success_ = PKCS5_PBKDF2_HMAC( pass_.data, pass_.size, @@ -4611,12 +4605,6 @@ void PBKDF2Request::Work() { } -void PBKDF2Request::Work(uv_work_t* work_req) { - PBKDF2Request* req = ContainerOf(&PBKDF2Request::work_req_, work_req); - req->Work(); -} - - void PBKDF2Request::After(Local (*argv)[2]) { if (success_) { (*argv)[0] = Null(env()->isolate()); @@ -4629,7 +4617,12 @@ void PBKDF2Request::After(Local (*argv)[2]) { } -void PBKDF2Request::After() { +void PBKDF2Request::AfterThreadPoolWork(int status) { + std::unique_ptr req(this); + if (status == UV_ECANCELED) + return; + CHECK_EQ(status, 0); + HandleScope handle_scope(env()->isolate()); Context::Scope context_scope(env()->context()); Local argv[2]; @@ -4638,14 +4631,6 @@ void PBKDF2Request::After() { } -void PBKDF2Request::After(uv_work_t* work_req, int status) { - CHECK_EQ(status, 0); - std::unique_ptr req( - ContainerOf(&PBKDF2Request::work_req_, work_req)); - req->After(); -} - - void PBKDF2(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -4692,13 +4677,10 @@ void PBKDF2(const FunctionCallbackInfo& args) { if (args[5]->IsFunction()) { obj->Set(env->context(), env->ondone_string(), args[5]).FromJust(); - uv_queue_work(env->event_loop(), - req.release()->work_req(), - PBKDF2Request::Work, - PBKDF2Request::After); + req.release()->ScheduleWork(); } else { env->PrintSyncTrace(); - req->Work(); + req->DoThreadPoolWork(); Local argv[2]; req->After(&argv); @@ -4711,7 +4693,7 @@ void PBKDF2(const FunctionCallbackInfo& args) { // Only instantiate within a valid HandleScope. -class RandomBytesRequest : public AsyncWrap { +class RandomBytesRequest : public AsyncWrap, public ThreadPoolWork { public: enum FreeMode { FREE_DATA, DONT_FREE_DATA }; @@ -4721,16 +4703,13 @@ class RandomBytesRequest : public AsyncWrap { char* data, FreeMode free_mode) : AsyncWrap(env, object, AsyncWrap::PROVIDER_RANDOMBYTESREQUEST), + ThreadPoolWork(env), error_(0), size_(size), data_(data), free_mode_(free_mode) { } - uv_work_t* work_req() { - return &work_req_; - } - inline size_t size() const { return size_; } @@ -4768,7 +4747,8 @@ class RandomBytesRequest : public AsyncWrap { size_t self_size() const override { return sizeof(*this); } - uv_work_t work_req_; + void DoThreadPoolWork() override; + void AfterThreadPoolWork(int status) override; private: unsigned long error_; // NOLINT(runtime/int) @@ -4778,21 +4758,17 @@ class RandomBytesRequest : public AsyncWrap { }; -void RandomBytesWork(uv_work_t* work_req) { - RandomBytesRequest* req = - ContainerOf(&RandomBytesRequest::work_req_, work_req); - +void RandomBytesRequest::DoThreadPoolWork() { // Ensure that OpenSSL's PRNG is properly seeded. CheckEntropy(); - const int r = RAND_bytes(reinterpret_cast(req->data()), - req->size()); + const int r = RAND_bytes(reinterpret_cast(data_), size_); // RAND_bytes() returns 0 on error. if (r == 0) { - req->set_error(ERR_get_error()); // NOLINT(runtime/int) + set_error(ERR_get_error()); // NOLINT(runtime/int) } else if (r == -1) { - req->set_error(static_cast(-1)); // NOLINT(runtime/int) + set_error(static_cast(-1)); // NOLINT(runtime/int) } } @@ -4830,16 +4806,16 @@ void RandomBytesCheck(RandomBytesRequest* req, Local (*argv)[2]) { } -void RandomBytesAfter(uv_work_t* work_req, int status) { +void RandomBytesRequest::AfterThreadPoolWork(int status) { + std::unique_ptr req(this); + if (status == UV_ECANCELED) + return; CHECK_EQ(status, 0); - std::unique_ptr req( - ContainerOf(&RandomBytesRequest::work_req_, work_req)); - Environment* env = req->env(); - HandleScope handle_scope(env->isolate()); - Context::Scope context_scope(env->context()); + HandleScope handle_scope(env()->isolate()); + Context::Scope context_scope(env()->context()); Local argv[2]; - RandomBytesCheck(req.get(), &argv); - req->MakeCallback(env->ondone_string(), arraysize(argv), argv); + RandomBytesCheck(this, &argv); + MakeCallback(env()->ondone_string(), arraysize(argv), argv); } @@ -4847,7 +4823,7 @@ void RandomBytesProcessSync(Environment* env, std::unique_ptr req, Local (*argv)[2]) { env->PrintSyncTrace(); - RandomBytesWork(req->work_req()); + req->DoThreadPoolWork(); RandomBytesCheck(req.get(), argv); if (!(*argv)[0]->IsNull()) @@ -4874,10 +4850,7 @@ void RandomBytes(const FunctionCallbackInfo& args) { if (args[1]->IsFunction()) { obj->Set(env->context(), env->ondone_string(), args[1]).FromJust(); - uv_queue_work(env->event_loop(), - req.release()->work_req(), - RandomBytesWork, - RandomBytesAfter); + req.release()->ScheduleWork(); args.GetReturnValue().Set(obj); } else { Local argv[2]; @@ -4913,10 +4886,7 @@ void RandomBytesBuffer(const FunctionCallbackInfo& args) { if (args[3]->IsFunction()) { obj->Set(env->context(), env->ondone_string(), args[3]).FromJust(); - uv_queue_work(env->event_loop(), - req.release()->work_req(), - RandomBytesWork, - RandomBytesAfter); + req.release()->ScheduleWork(); args.GetReturnValue().Set(obj); } else { Local argv[2]; diff --git a/src/node_file.cc b/src/node_file.cc index 97b957eed66b31..713dcbf6331898 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -89,6 +89,11 @@ using v8::Value; TRACE_EVENT_END(TRACING_CATEGORY_NODE2(fs, sync), TRACE_NAME(syscall), \ ##__VA_ARGS__); +// We sometimes need to convert a C++ lambda function to a raw C-style function. +// This is helpful, because ReqWrap::Dispatch() does not recognize lambda +// functions, and thus does not wrap them properly. +typedef void(*uv_fs_callback_t)(uv_fs_t*); + // The FileHandle object wraps a file descriptor and will close it on garbage // collection if necessary. If that happens, a process warning will be // emitted (or a fatal exception will occur if the fd cannot be closed.) @@ -216,7 +221,7 @@ inline MaybeLocal FileHandle::ClosePromise() { if (!closed_ && !closing_) { closing_ = true; CloseReq* req = new CloseReq(env(), promise, object()); - auto AfterClose = [](uv_fs_t* req) { + auto AfterClose = uv_fs_callback_t{[](uv_fs_t* req) { CloseReq* close = static_cast(req->data); CHECK_NE(close, nullptr); close->file_handle()->AfterClose(); @@ -227,9 +232,8 @@ inline MaybeLocal FileHandle::ClosePromise() { close->Resolve(); } delete close; - }; - req->Dispatched(); - int ret = uv_fs_close(env()->event_loop(), req->req(), fd_, AfterClose); + }}; + int ret = req->Dispatch(uv_fs_close, fd_, AfterClose); if (ret < 0) { req->Reject(UVException(isolate, ret, "close")); delete req; @@ -309,17 +313,15 @@ int FileHandle::ReadStart() { recommended_read = read_length_; read_wrap->buffer_ = EmitAlloc(recommended_read); - read_wrap->Dispatched(); current_read_ = std::move(read_wrap); - uv_fs_read(env()->event_loop(), - current_read_->req(), - fd_, - ¤t_read_->buffer_, - 1, - read_offset_, - [](uv_fs_t* req) { + current_read_->Dispatch(uv_fs_read, + fd_, + ¤t_read_->buffer_, + 1, + read_offset_, + uv_fs_callback_t{[](uv_fs_t* req) { FileHandle* handle; { FileHandleReadWrap* req_wrap = FileHandleReadWrap::from_req(req); @@ -342,8 +344,10 @@ int FileHandle::ReadStart() { // once we’re exiting the current scope. constexpr size_t wanted_freelist_fill = 100; auto& freelist = handle->env()->file_handle_read_wrap_freelist(); - if (freelist.size() < wanted_freelist_fill) + if (freelist.size() < wanted_freelist_fill) { + read_wrap->Reset(); freelist.emplace_back(std::move(read_wrap)); + } if (result >= 0) { // Read at most as many bytes as we originally planned to. @@ -370,7 +374,7 @@ int FileHandle::ReadStart() { // Start over, if EmitRead() didn’t tell us to stop. if (handle->reading_) handle->ReadStart(); - }); + }}); return 0; } @@ -389,8 +393,7 @@ ShutdownWrap* FileHandle::CreateShutdownWrap(Local object) { int FileHandle::DoShutdown(ShutdownWrap* req_wrap) { FileHandleCloseWrap* wrap = static_cast(req_wrap); closing_ = true; - wrap->Dispatched(); - uv_fs_close(env()->event_loop(), wrap->req(), fd_, [](uv_fs_t* req) { + wrap->Dispatch(uv_fs_close, fd_, uv_fs_callback_t{[](uv_fs_t* req) { FileHandleCloseWrap* wrap = static_cast( FileHandleCloseWrap::from_req(req)); FileHandle* handle = static_cast(wrap->stream()); @@ -399,7 +402,7 @@ int FileHandle::DoShutdown(ShutdownWrap* req_wrap) { int result = req->result; uv_fs_req_cleanup(req); wrap->Done(result); - }); + }}); return 0; } @@ -616,8 +619,7 @@ inline FSReqBase* AsyncDestCall(Environment* env, enum encoding enc, uv_fs_cb after, Func fn, Args... fn_args) { 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(); + int err = req_wrap->Dispatch(fn, fn_args..., after); if (err < 0) { uv_fs_t* uv_req = req_wrap->req(); uv_req->result = err; diff --git a/src/node_internals.h b/src/node_internals.h index e15df78ffdfee3..8aa46318803985 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -503,6 +503,41 @@ class InternalCallbackScope { bool closed_ = false; }; +class ThreadPoolWork { + public: + explicit inline ThreadPoolWork(Environment* env) : env_(env) {} + inline void ScheduleWork(); + inline int CancelWork(); + + virtual void DoThreadPoolWork() = 0; + virtual void AfterThreadPoolWork(int status) = 0; + + private: + Environment* env_; + uv_work_t work_req_; +}; + +void ThreadPoolWork::ScheduleWork() { + env_->IncreaseWaitingRequestCounter(); + int status = uv_queue_work( + env_->event_loop(), + &work_req_, + [](uv_work_t* req) { + ThreadPoolWork* self = ContainerOf(&ThreadPoolWork::work_req_, req); + self->DoThreadPoolWork(); + }, + [](uv_work_t* req, int status) { + ThreadPoolWork* self = ContainerOf(&ThreadPoolWork::work_req_, req); + self->env_->DecreaseWaitingRequestCounter(); + self->AfterThreadPoolWork(status); + }); + CHECK_EQ(status, 0); +} + +int ThreadPoolWork::CancelWork() { + return uv_cancel(reinterpret_cast(&work_req_)); +} + static inline const char *errno_string(int errorno) { #define ERRNO_CASE(e) case e: return #e; switch (errorno) { diff --git a/src/node_stat_watcher.cc b/src/node_stat_watcher.cc index a2cfb1088c9d25..d8f8a6a362237d 100644 --- a/src/node_stat_watcher.cc +++ b/src/node_stat_watcher.cc @@ -75,11 +75,6 @@ void StatWatcher::Initialize(Environment* env, Local target) { } -static void Delete(uv_handle_t* handle) { - delete reinterpret_cast(handle); -} - - StatWatcher::StatWatcher(Environment* env, Local wrap) : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_STATWATCHER), watcher_(new uv_fs_poll_t) { @@ -93,7 +88,7 @@ StatWatcher::~StatWatcher() { if (IsActive()) { Stop(); } - uv_close(reinterpret_cast(watcher_), Delete); + env()->CloseHandle(watcher_, [](uv_fs_poll_t* handle) { delete handle; }); } diff --git a/src/node_trace_events.cc b/src/node_trace_events.cc index 0c0699f7be9e1f..363d046de1e947 100644 --- a/src/node_trace_events.cc +++ b/src/node_trace_events.cc @@ -21,11 +21,6 @@ using v8::Value; class NodeCategorySet : public BaseObject { public: - ~NodeCategorySet() override { - // Verify that the thing was properly disabled before gc - CHECK_NE(enabled_, true); - } - static void New(const FunctionCallbackInfo& args); static void Enable(const FunctionCallbackInfo& args); static void Disable(const FunctionCallbackInfo& args); diff --git a/src/node_zlib.cc b/src/node_zlib.cc index ec447638e2ae62..c77e6d3297df5d 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -70,10 +70,11 @@ enum node_zlib_mode { /** * Deflate/Inflate */ -class ZCtx : public AsyncWrap { +class ZCtx : public AsyncWrap, public ThreadPoolWork { public: ZCtx(Environment* env, Local wrap, node_zlib_mode mode) : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_ZLIB), + ThreadPoolWork(env), dictionary_(nullptr), dictionary_len_(0), err_(0), @@ -191,9 +192,6 @@ class ZCtx : public AsyncWrap { CHECK(Buffer::IsWithinBounds(out_off, out_len, Buffer::Length(out_buf))); out = reinterpret_cast(Buffer::Data(out_buf) + out_off); - // build up the work request - uv_work_t* work_req = &(ctx->work_req_); - ctx->strm_.avail_in = in_len; ctx->strm_.next_in = in; ctx->strm_.avail_out = out_len; @@ -203,7 +201,7 @@ class ZCtx : public AsyncWrap { if (!async) { // sync version env->PrintSyncTrace(); - Process(work_req); + ctx->DoThreadPoolWork(); if (CheckError(ctx)) { ctx->write_result_[0] = ctx->strm_.avail_out; ctx->write_result_[1] = ctx->strm_.avail_in; @@ -214,17 +212,24 @@ class ZCtx : public AsyncWrap { } // async version - uv_queue_work(env->event_loop(), work_req, ZCtx::Process, ZCtx::After); + ctx->ScheduleWork(); + } + + // TODO(addaleax): Make these methods non-static. It's a significant bunch + // of churn that's better left for a separate PR. + void DoThreadPoolWork() { + Process(this); } + void AfterThreadPoolWork(int status) { + After(this, status); + } // thread pool! // This function may be called multiple times on the uv_work pool // for a single write() call, until all of the input bytes have // been consumed. - static void Process(uv_work_t* work_req) { - ZCtx *ctx = ContainerOf(&ZCtx::work_req_, work_req); - + static void Process(ZCtx* ctx) { const Bytef* next_expected_header_byte = nullptr; // If the avail_out is left at 0, then it means that it ran out @@ -360,11 +365,16 @@ class ZCtx : public AsyncWrap { // v8 land! - static void After(uv_work_t* work_req, int status) { - CHECK_EQ(status, 0); - - ZCtx* ctx = ContainerOf(&ZCtx::work_req_, work_req); + static void After(ZCtx* ctx, int status) { Environment* env = ctx->env(); + ctx->write_in_progress_ = false; + + if (status == UV_ECANCELED) { + ctx->Close(); + return; + } + + CHECK_EQ(status, 0); HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); @@ -374,7 +384,6 @@ class ZCtx : public AsyncWrap { ctx->write_result_[0] = ctx->strm_.avail_out; ctx->write_result_[1] = ctx->strm_.avail_in; - ctx->write_in_progress_ = false; // call the write() cb Local cb = PersistentToLocal(env->isolate(), @@ -678,7 +687,6 @@ class ZCtx : public AsyncWrap { int strategy_; z_stream strm_; int windowBits_; - uv_work_t work_req_; bool write_in_progress_; bool pending_close_; unsigned int refs_; diff --git a/src/pipe_wrap.cc b/src/pipe_wrap.cc index da7cb9e3ab55ba..f3a88a2ecc80c3 100644 --- a/src/pipe_wrap.cc +++ b/src/pipe_wrap.cc @@ -204,6 +204,7 @@ void PipeWrap::Open(const FunctionCallbackInfo& args) { int fd = args[0]->Int32Value(); int err = uv_pipe_open(&wrap->handle_, fd); + wrap->set_fd(fd); if (err != 0) env->isolate()->ThrowException(UVException(err, "uv_pipe_open")); @@ -224,11 +225,10 @@ void PipeWrap::Connect(const FunctionCallbackInfo& args) { ConnectWrap* req_wrap = new ConnectWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_PIPECONNECTWRAP); - uv_pipe_connect(req_wrap->req(), - &wrap->handle_, - *name, - AfterConnect); - req_wrap->Dispatched(); + req_wrap->Dispatch(uv_pipe_connect, + &wrap->handle_, + *name, + AfterConnect); args.GetReturnValue().Set(0); // uv_pipe_connect() doesn't return errors. } diff --git a/src/process_wrap.cc b/src/process_wrap.cc index 96d60cc900583c..6d421fe7c4d4de 100644 --- a/src/process_wrap.cc +++ b/src/process_wrap.cc @@ -88,6 +88,7 @@ class ProcessWrap : public HandleWrap { object, reinterpret_cast(&process_), AsyncWrap::PROVIDER_PROCESSWRAP) { + MarkAsUninitialized(); } static void ParseStdioOptions(Environment* env, @@ -256,6 +257,7 @@ class ProcessWrap : public HandleWrap { } int err = uv_spawn(env->event_loop(), &wrap->process_, &options); + wrap->MarkAsInitialized(); if (err == 0) { CHECK_EQ(wrap->process_.data, wrap); diff --git a/src/req_wrap-inl.h b/src/req_wrap-inl.h index 11b1389fa0e771..7e9e2d9fbbf912 100644 --- a/src/req_wrap-inl.h +++ b/src/req_wrap-inl.h @@ -7,6 +7,7 @@ #include "async_wrap-inl.h" #include "env-inl.h" #include "util-inl.h" +#include "uv.h" namespace node { @@ -19,11 +20,12 @@ ReqWrap::ReqWrap(Environment* env, // FIXME(bnoordhuis) The fact that a reinterpret_cast is needed is // arguably a good indicator that there should be more than one queue. env->req_wrap_queue()->PushBack(reinterpret_cast*>(this)); + + Reset(); } template ReqWrap::~ReqWrap() { - CHECK_EQ(req_.data, this); // Assert that someone has called Dispatched(). CHECK_EQ(false, persistent().IsEmpty()); } @@ -32,11 +34,131 @@ void ReqWrap::Dispatched() { req_.data = this; } +template +void ReqWrap::Reset() { + original_callback_ = nullptr; + req_.data = nullptr; +} + template ReqWrap* ReqWrap::from_req(T* req) { return ContainerOf(&ReqWrap::req_, req); } +template +void ReqWrap::Cancel() { + if (req_.data == this) // Only cancel if already dispatched. + uv_cancel(reinterpret_cast(&req_)); +} + +// Below is dark template magic designed to invoke libuv functions that +// initialize uv_req_t instances in a unified fashion, to allow easier +// tracking of active/inactive requests. + +// Invoke a generic libuv function that initializes uv_req_t instances. +// This is, unfortunately, necessary since they come in three different +// variants that can not all be invoked in the same way: +// - int uv_foo(uv_loop_t* loop, uv_req_t* request, ...); +// - int uv_foo(uv_req_t* request, ...); +// - void uv_foo(uv_req_t* request, ...); +template +struct CallLibuvFunction; + +// Detect `int uv_foo(uv_loop_t* loop, uv_req_t* request, ...);`. +template +struct CallLibuvFunction { + using T = int(*)(uv_loop_t*, ReqT*, Args...); + template + static int Call(T fn, uv_loop_t* loop, ReqT* req, PassedArgs... args) { + return fn(loop, req, args...); + } +}; + +// Detect `int uv_foo(uv_req_t* request, ...);`. +template +struct CallLibuvFunction { + using T = int(*)(ReqT*, Args...); + template + static int Call(T fn, uv_loop_t* loop, ReqT* req, PassedArgs... args) { + return fn(req, args...); + } +}; + +// Detect `void uv_foo(uv_req_t* request, ...);`. +template +struct CallLibuvFunction { + using T = void(*)(ReqT*, Args...); + template + static int Call(T fn, uv_loop_t* loop, ReqT* req, PassedArgs... args) { + fn(req, args...); + return 0; + } +}; + +// This is slightly darker magic: This template is 'applied' to each parameter +// passed to the libuv function. If the parameter type (aka `T`) is a +// function type, it is assumed that this it is the request callback, and a +// wrapper that calls the original callback is created. +// If not, the parameter is passed through verbatim. +template +struct MakeLibuvRequestCallback { + static T For(ReqWrap* req_wrap, T v) { + static_assert(!is_callable::value, + "MakeLibuvRequestCallback missed a callback"); + return v; + } +}; + +// Match the `void callback(uv_req_t*, ...);` signature that all libuv +// callbacks use. +template +struct MakeLibuvRequestCallback { + using F = void(*)(ReqT* req, Args... args); + + static void Wrapper(ReqT* req, Args... args) { + ReqWrap* req_wrap = ContainerOf(&ReqWrap::req_, req); + req_wrap->env()->DecreaseWaitingRequestCounter(); + F original_callback = reinterpret_cast(req_wrap->original_callback_); + original_callback(req, args...); + } + + static F For(ReqWrap* req_wrap, F v) { + CHECK_EQ(req_wrap->original_callback_, nullptr); + req_wrap->original_callback_ = + reinterpret_cast::callback_t>(v); + return Wrapper; + } +}; + +template +template +int ReqWrap::Dispatch(LibuvFunction fn, Args... args) { + Dispatched(); + + // This expands as: + // + // int err = fn(env()->event_loop(), req(), arg1, arg2, Wrapper, arg3, ...) + // ^ ^ ^ + // | | | + // \-- Omitted if `fn` has no | | + // first `uv_loop_t*` argument | | + // | | + // A function callback whose first argument | | + // matches the libuv request type is replaced ---/ | + // by the `Wrapper` method defined above | + // | + // Other (non-function) arguments are passed -----/ + // through verbatim + int err = CallLibuvFunction::Call( + fn, + env()->event_loop(), + req(), + MakeLibuvRequestCallback::For(this, args)...); + if (err >= 0) + env()->IncreaseWaitingRequestCounter(); + return err; +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/req_wrap.h b/src/req_wrap.h index 656be38dcea943..8f8d0cf2885594 100644 --- a/src/req_wrap.h +++ b/src/req_wrap.h @@ -17,16 +17,30 @@ class ReqWrap : public AsyncWrap { v8::Local object, AsyncWrap::ProviderType provider); inline ~ReqWrap() override; - inline void Dispatched(); // Call this after the req has been dispatched. + // Call this after the req has been dispatched, if that did not already + // happen by using Dispatch(). + inline void Dispatched(); + // Call this after a request has finished, if re-using this object is planned. + inline void Reset(); T* req() { return &req_; } + inline void Cancel(); static ReqWrap* from_req(T* req); + template + inline int Dispatch(LibuvFunction fn, Args... args); + private: friend class Environment; friend int GenDebugSymbols(); + template + friend struct MakeLibuvRequestCallback; + ListNode req_wrap_queue_; + typedef void (*callback_t)(); + callback_t original_callback_ = nullptr; + protected: // req_wrap_queue_ needs to be at a fixed offset from the start of the class // because it is used by ContainerOf to calculate the address of the embedding diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index ad708c9ed28def..cdcbe574f9ae5f 100644 --- a/src/stream_wrap.cc +++ b/src/stream_wrap.cc @@ -115,12 +115,14 @@ void LibuvStreamWrap::AddMethods(Environment* env, int LibuvStreamWrap::GetFD() { +#ifdef _WIN32 + return fd_; +#else int fd = -1; -#if !defined(_WIN32) if (stream() != nullptr) uv_fileno(reinterpret_cast(stream()), &fd); -#endif return fd; +#endif } @@ -371,6 +373,11 @@ void LibuvStreamWrap::AfterUvWrite(uv_write_t* req, int status) { req_wrap->Done(status); } +void LibuvStreamWrap::Close(v8::Local close_callback) { + ReadStop(); + HandleWrap::Close(close_callback); +} + } // namespace node NODE_BUILTIN_MODULE_CONTEXT_AWARE(stream_wrap, diff --git a/src/stream_wrap.h b/src/stream_wrap.h index a97e8ba10f91d5..94a161b6d54e07 100644 --- a/src/stream_wrap.h +++ b/src/stream_wrap.h @@ -76,6 +76,8 @@ class LibuvStreamWrap : public HandleWrap, public StreamBase { ShutdownWrap* CreateShutdownWrap(v8::Local object) override; WriteWrap* CreateWriteWrap(v8::Local object) override; + void Close(v8::Local close_callback) override; + protected: LibuvStreamWrap(Environment* env, v8::Local object, @@ -88,6 +90,14 @@ class LibuvStreamWrap : public HandleWrap, public StreamBase { v8::Local target, int flags = StreamBase::kFlagNone); + protected: + inline void set_fd(int fd) { +#ifdef _WIN32 + fd_ = fd; +#endif + } + + private: static void GetWriteQueueSize( const v8::FunctionCallbackInfo& info); @@ -101,6 +111,16 @@ class LibuvStreamWrap : public HandleWrap, public StreamBase { static void AfterUvShutdown(uv_shutdown_t* req, int status); uv_stream_t* const stream_; + +#ifdef _WIN32 + // We don't always have an FD that we could look up on the stream_ + // object itself on Windows. However, for some cases, we open handles + // using FDs; In that case, we can store and provide the value. + // This became necessary because it allows to detect situations + // where multiple handles refer to the same stdio FDs (in particular, + // a possible IPC channel and a regular process.std??? stream). + int fd_ = -1; +#endif }; diff --git a/src/tcp_wrap.cc b/src/tcp_wrap.cc index 3ccd157159c287..8200353b17bf99 100644 --- a/src/tcp_wrap.cc +++ b/src/tcp_wrap.cc @@ -212,6 +212,7 @@ void TCPWrap::Open(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(UV_EBADF)); int fd = static_cast(args[0]->IntegerValue()); uv_tcp_open(&wrap->handle_, fd); + wrap->set_fd(fd); } @@ -287,11 +288,10 @@ void TCPWrap::Connect(const FunctionCallbackInfo& args) { AsyncHooks::DefaultTriggerAsyncIdScope trigger_scope(wrap); ConnectWrap* req_wrap = new ConnectWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_TCPCONNECTWRAP); - err = uv_tcp_connect(req_wrap->req(), - &wrap->handle_, - reinterpret_cast(&addr), - AfterConnect); - req_wrap->Dispatched(); + err = req_wrap->Dispatch(uv_tcp_connect, + &wrap->handle_, + reinterpret_cast(&addr), + AfterConnect); if (err) delete req_wrap; } @@ -323,11 +323,10 @@ void TCPWrap::Connect6(const FunctionCallbackInfo& args) { AsyncHooks::DefaultTriggerAsyncIdScope trigger_scope(wrap); ConnectWrap* req_wrap = new ConnectWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_TCPCONNECTWRAP); - err = uv_tcp_connect(req_wrap->req(), - &wrap->handle_, - reinterpret_cast(&addr), - AfterConnect); - req_wrap->Dispatched(); + err = req_wrap->Dispatch(uv_tcp_connect, + &wrap->handle_, + reinterpret_cast(&addr), + AfterConnect); if (err) delete req_wrap; } diff --git a/src/tty_wrap.cc b/src/tty_wrap.cc index c5abc6bf9b9b91..cd8589cc7fc2e7 100644 --- a/src/tty_wrap.cc +++ b/src/tty_wrap.cc @@ -172,6 +172,9 @@ TTYWrap::TTYWrap(Environment* env, reinterpret_cast(&handle_), AsyncWrap::PROVIDER_TTYWRAP) { *init_err = uv_tty_init(env->event_loop(), &handle_, fd, readable); + set_fd(fd); + if (*init_err != 0) + MarkAsUninitialized(); } } // namespace node diff --git a/src/udp_wrap.cc b/src/udp_wrap.cc index 414fe07eab6da8..1d1ded449bd221 100644 --- a/src/udp_wrap.cc +++ b/src/udp_wrap.cc @@ -380,15 +380,14 @@ void UDPWrap::DoSend(const FunctionCallbackInfo& args, int family) { } if (err == 0) { - err = uv_udp_send(req_wrap->req(), - &wrap->handle_, - *bufs, - count, - reinterpret_cast(&addr), - OnSend); + err = req_wrap->Dispatch(uv_udp_send, + &wrap->handle_, + *bufs, + count, + reinterpret_cast(&addr), + OnSend); } - req_wrap->Dispatched(); if (err) delete req_wrap; diff --git a/src/util.h b/src/util.h index 7a1c6c109fdbff..2c66104e9d24ac 100644 --- a/src/util.h +++ b/src/util.h @@ -447,8 +447,16 @@ struct MallocedBuffer { MallocedBuffer& operator=(const MallocedBuffer&) = delete; }; -} // namespace node +// Test whether some value can be called with (). +template +struct is_callable : std::is_function { }; + +template +struct is_callable::value + >::type> : std::true_type { }; +} // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/test/addons-napi/test_cleanup_hook/binding.cc b/test/addons-napi/test_cleanup_hook/binding.cc new file mode 100644 index 00000000000000..66d53508c69f13 --- /dev/null +++ b/test/addons-napi/test_cleanup_hook/binding.cc @@ -0,0 +1,24 @@ +#include "node_api.h" +#include "uv.h" +#include "../common.h" + +namespace { + +void cleanup(void* arg) { + printf("cleanup(%d)\n", *static_cast(arg)); +} + +int secret = 42; +int wrong_secret = 17; + +napi_value Init(napi_env env, napi_value exports) { + napi_add_env_cleanup_hook(env, cleanup, &wrong_secret); + napi_add_env_cleanup_hook(env, cleanup, &secret); + napi_remove_env_cleanup_hook(env, cleanup, &wrong_secret); + + return nullptr; +} + +} // anonymous namespace + +NAPI_MODULE(NODE_GYP_MODULE_NAME, Init) diff --git a/test/addons-napi/test_cleanup_hook/binding.gyp b/test/addons-napi/test_cleanup_hook/binding.gyp new file mode 100644 index 00000000000000..7ede63d94a0d77 --- /dev/null +++ b/test/addons-napi/test_cleanup_hook/binding.gyp @@ -0,0 +1,9 @@ +{ + 'targets': [ + { + 'target_name': 'binding', + 'defines': [ 'V8_DEPRECATION_WARNINGS=1' ], + 'sources': [ 'binding.cc' ] + } + ] +} diff --git a/test/addons-napi/test_cleanup_hook/test.js b/test/addons-napi/test_cleanup_hook/test.js new file mode 100644 index 00000000000000..354f4449045c17 --- /dev/null +++ b/test/addons-napi/test_cleanup_hook/test.js @@ -0,0 +1,12 @@ +'use strict'; +const common = require('../../common'); +const assert = require('assert'); +const child_process = require('child_process'); + +if (process.argv[2] === 'child') { + require(`./build/${common.buildType}/binding`); +} else { + const { stdout } = + child_process.spawnSync(process.execPath, [__filename, 'child']); + assert.strictEqual(stdout.toString().trim(), 'cleanup(42)'); +} diff --git a/tools/run-valgrind.py b/tools/run-valgrind.py index ae1a3194cac4a6..cad3e7ec6954d2 100755 --- a/tools/run-valgrind.py +++ b/tools/run-valgrind.py @@ -37,9 +37,6 @@ 'valgrind', '--error-exitcode=1', '--smc-check=all', - # Node.js does not clean up on exit so don't complain about - # memory leaks but do complain about invalid memory access. - '--quiet', ] if len(sys.argv) < 2: