From 292b53a051be0b3f4d86c69701d2d0e0893d1057 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 9 Sep 2017 22:28:02 +0200 Subject: [PATCH 01/21] src: add environment cleanup hooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This adds pairs of methods to the `Environment` class and to public APIs which can add and remove cleanup handlers. Unlike `AtExit`, this API targets addon developers rather than embedders, giving them (and Node’s internals) the ability to register per-`Environment` cleanup work. We may want to replace `AtExit` with this API at some point. Many thanks for Stephen Belanger for reviewing the original version of this commit in the Ayo.js project. Refs: https://github.com/ayojs/ayo/pull/82 --- doc/api/n-api.md | 52 +++++++++++++++++++ src/env-inl.h | 23 ++++++++ src/env.cc | 21 ++++++++ src/env.h | 31 +++++++++++ src/node.cc | 20 ++++++- src/node.h | 13 +++++ src/node_api.cc | 22 ++++++++ src/node_api.h | 7 +++ test/addons-napi/test_cleanup_hook/binding.cc | 24 +++++++++ .../addons-napi/test_cleanup_hook/binding.gyp | 9 ++++ test/addons-napi/test_cleanup_hook/test.js | 12 +++++ 11 files changed, 233 insertions(+), 1 deletion(-) create mode 100644 test/addons-napi/test_cleanup_hook/binding.cc create mode 100644 test/addons-napi/test_cleanup_hook/binding.gyp create mode 100644 test/addons-napi/test_cleanup_hook/test.js 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/src/env-inl.h b/src/env-inl.h index 6202e50548a3ce..d3c0c211d97328 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -629,6 +629,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..d0faeaa1609953 100644 --- a/src/env.cc +++ b/src/env.cc @@ -305,6 +305,27 @@ void Environment::PrintSyncTrace() const { fflush(stderr); } +void Environment::RunCleanup() { + 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()); + cleanup_hooks_.clear(); + + 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) { + cb.fn_(cb.arg_); + 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..3acb27c9545525 100644 --- a/src/env.h +++ b/src/env.h @@ -42,6 +42,7 @@ #include #include #include +#include struct nghttp2_rcbuf; @@ -775,6 +776,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, @@ -863,6 +868,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/node.cc b/src/node.cc index d71303570b1a56..91ffb8c3f5592e 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) @@ -4434,7 +4450,7 @@ Environment* CreateEnvironment(IsolateData* isolate_data, void FreeEnvironment(Environment* env) { - env->CleanupHandles(); + env->RunCleanup(); delete env; } @@ -4532,6 +4548,8 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, env.set_trace_sync_io(false); const int exit_code = EmitExit(&env); + + env.RunCleanup(); RunAtExit(&env); v8_platform.DrainVMTasks(isolate); 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..d5437d70d933ed 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, 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/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)'); +} From ca372b3a599b1cd33effc8e47300d678c8e55f29 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 27 Apr 2018 19:28:49 +0200 Subject: [PATCH 02/21] fixup! src: add environment cleanup hooks --- src/env.cc | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/env.cc b/src/env.cc index d0faeaa1609953..aadb81092e507c 100644 --- a/src/env.cc +++ b/src/env.cc @@ -310,7 +310,8 @@ void Environment::RunCleanup() { // Copy into a vector, since we can't sort an unordered_set in-place. std::vector callbacks( cleanup_hooks_.begin(), cleanup_hooks_.end()); - cleanup_hooks_.clear(); + // 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) { @@ -320,7 +321,14 @@ void Environment::RunCleanup() { }); 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(); } } From 16e7a35cea3115dd0ce09f303fbe6ce33fbb02f8 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 4 Sep 2017 22:02:55 +0200 Subject: [PATCH 03/21] src: make CleanupHandles() tear down handles/reqs Previously, handles would not be closed when the current `Environment` stopped, which is acceptable in a single-`Environment`-per-process situation, but would otherwise create memory and file descriptor leaks. Also, introduce a generic way to close handles via the `Environment::CloseHandle()` function, which automatically keeps track of whether a close callback has been called yet or not. Many thanks for Stephen Belanger for reviewing the original version of this commit in the Ayo.js project. Refs: https://github.com/ayojs/ayo/pull/85 --- src/cares_wrap.cc | 18 ++++++------------ src/connection_wrap.h | 2 -- src/env-inl.h | 22 ++++++++++++++++++++-- src/env.cc | 18 +++++++++++------- src/env.h | 6 +++++- src/fs_event_wrap.cc | 6 ++++-- src/handle_wrap.cc | 37 +++++++++++++++++++++++++------------ src/handle_wrap.h | 7 +++++++ src/node_stat_watcher.cc | 7 +------ src/process_wrap.cc | 2 ++ src/req_wrap-inl.h | 6 ++++++ src/req_wrap.h | 1 + src/tty_wrap.cc | 2 ++ 13 files changed, 90 insertions(+), 44 deletions(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 4208c02d4fe445..ae253d40ca94b2 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, 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 d3c0c211d97328..917ddd1b6bcb75 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -349,8 +349,26 @@ 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)); + }); } inline uv_loop_t* Environment::event_loop() const { diff --git a/src/env.cc b/src/env.cc index aadb81092e507c..a6ae204f54a227 100644 --- a/src/env.cc +++ b/src/env.cc @@ -209,9 +209,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,13 +231,17 @@ 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 || !handle_wrap_queue_.IsEmpty()) uv_run(event_loop(), UV_RUN_ONCE); } @@ -306,6 +308,8 @@ void Environment::PrintSyncTrace() const { } 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( diff --git a/src/env.h b/src/env.h index 3acb27c9545525..79351666c1182e 100644 --- a/src/env.h +++ b/src/env.h @@ -577,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); 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..e45e00a280b4d8 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_; } + void Close(v8::Local close_callback = v8::Local()); + protected: HandleWrap(Environment* env, v8::Local object, uv_handle_t* handle, AsyncWrap::ProviderType provider); + ~HandleWrap() {} + virtual void OnClose() {} + + void MarkAsInitialized(); + void MarkAsUninitialized(); private: friend class Environment; 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/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..ab5051e41d8e89 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 { @@ -37,6 +38,11 @@ ReqWrap* ReqWrap::from_req(T* req) { return ContainerOf(&ReqWrap::req_, req); } +template +void ReqWrap::Cancel() { + uv_cancel(reinterpret_cast(&req_)); +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/req_wrap.h b/src/req_wrap.h index 656be38dcea943..4d6a89d743c591 100644 --- a/src/req_wrap.h +++ b/src/req_wrap.h @@ -19,6 +19,7 @@ class ReqWrap : public AsyncWrap { inline ~ReqWrap() override; inline void Dispatched(); // Call this after the req has been dispatched. T* req() { return &req_; } + inline void Cancel(); static ReqWrap* from_req(T* req); diff --git a/src/tty_wrap.cc b/src/tty_wrap.cc index c5abc6bf9b9b91..d01caba4a558f2 100644 --- a/src/tty_wrap.cc +++ b/src/tty_wrap.cc @@ -172,6 +172,8 @@ TTYWrap::TTYWrap(Environment* env, reinterpret_cast(&handle_), AsyncWrap::PROVIDER_TTYWRAP) { *init_err = uv_tty_init(env->event_loop(), &handle_, fd, readable); + if (*init_err != 0) + MarkAsUninitialized(); } } // namespace node From 9aa3c8fc4a745bc53fbaa4ad912cfd177d5c3fc1 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 7 May 2018 22:59:20 +0200 Subject: [PATCH 04/21] [squash] move CleanupHandle() up one scope --- src/env.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/env.cc b/src/env.cc index a6ae204f54a227..6526c680ac1792 100644 --- a/src/env.cc +++ b/src/env.cc @@ -333,8 +333,8 @@ void Environment::RunCleanup() { cb.fn_(cb.arg_); cleanup_hooks_.erase(cb); - CleanupHandles(); } + CleanupHandles(); } } From 9f0e5b14693523f2e9dde34a7de71128c4ae5c7a Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 7 May 2018 22:59:40 +0200 Subject: [PATCH 05/21] [squash] remove empty HandleWrap destructor --- src/handle_wrap.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/handle_wrap.h b/src/handle_wrap.h index e45e00a280b4d8..fd2d002dce0338 100644 --- a/src/handle_wrap.h +++ b/src/handle_wrap.h @@ -77,7 +77,6 @@ class HandleWrap : public AsyncWrap { v8::Local object, uv_handle_t* handle, AsyncWrap::ProviderType provider); - ~HandleWrap() {} virtual void OnClose() {} void MarkAsInitialized(); From ef4779e855b87c5ef6523f42c8bed2320e12c6a6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 25 Sep 2017 21:16:02 +0200 Subject: [PATCH 06/21] src: unify ReqWrap libuv calling This allows easier tracking of whether there are active `ReqWrap`s. Many thanks for Stephen Belanger for reviewing the original version of this commit in the Ayo.js project. Refs: https://github.com/ayojs/ayo/pull/85 --- src/cares_wrap.cc | 24 +++++------ src/node_file.cc | 40 ++++++++--------- src/pipe_wrap.cc | 9 ++-- src/req_wrap-inl.h | 104 +++++++++++++++++++++++++++++++++++++++++++++ src/req_wrap.h | 13 +++++- src/tcp_wrap.cc | 18 ++++---- src/udp_wrap.cc | 13 +++--- 7 files changed, 165 insertions(+), 56 deletions(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index ae253d40ca94b2..f829eb2b01d5c9 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -515,7 +515,7 @@ ChannelWrap::~ChannelWrap() { void ChannelWrap::CleanupTimer() { if (timer_handle_ == nullptr) return; - env()->CloseHandle(timer_handle_, [](uv_timer_t* handle){ delete handle; }); + env()->CloseHandle(timer_handle_, [](uv_timer_t* handle) { delete handle; }); timer_handle_ = nullptr; } @@ -1927,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; @@ -1957,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/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/pipe_wrap.cc b/src/pipe_wrap.cc index da7cb9e3ab55ba..7ec5bdf15be9cc 100644 --- a/src/pipe_wrap.cc +++ b/src/pipe_wrap.cc @@ -224,11 +224,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/req_wrap-inl.h b/src/req_wrap-inl.h index ab5051e41d8e89..a9501848d5bebf 100644 --- a/src/req_wrap-inl.h +++ b/src/req_wrap-inl.h @@ -43,6 +43,110 @@ void ReqWrap::Cancel() { 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 { + typedef int(*T)(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 { + typedef int(*T)(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 { + typedef void(*T)(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(!std::is_function::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); + 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: + // + // return 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 + return CallLibuvFunction::Call( + fn, + env()->event_loop(), + req(), + MakeLibuvRequestCallback::For(this, args)...); +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/req_wrap.h b/src/req_wrap.h index 4d6a89d743c591..d181817218227b 100644 --- a/src/req_wrap.h +++ b/src/req_wrap.h @@ -17,17 +17,28 @@ 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(); 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/tcp_wrap.cc b/src/tcp_wrap.cc index 3ccd157159c287..70c60fa47cc68f 100644 --- a/src/tcp_wrap.cc +++ b/src/tcp_wrap.cc @@ -287,11 +287,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 +322,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/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; From 89697e735f54b3f890a6b0722b42ddff0598267d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 27 Apr 2018 19:43:34 +0200 Subject: [PATCH 07/21] [squash] bnoordhuis comment --- src/req_wrap-inl.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/req_wrap-inl.h b/src/req_wrap-inl.h index a9501848d5bebf..03b735bf710369 100644 --- a/src/req_wrap-inl.h +++ b/src/req_wrap-inl.h @@ -59,7 +59,7 @@ struct CallLibuvFunction; // Detect `int uv_foo(uv_loop_t* loop, uv_req_t* request, ...);`. template struct CallLibuvFunction { - typedef int(*T)(uv_loop_t*, ReqT*, Args...); + 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...); @@ -69,7 +69,7 @@ struct CallLibuvFunction { // Detect `int uv_foo(uv_req_t* request, ...);`. template struct CallLibuvFunction { - typedef int(*T)(ReqT*, Args...); + using T = int(*)(ReqT*, Args...); template static int Call(T fn, uv_loop_t* loop, ReqT* req, PassedArgs... args) { return fn(req, args...); @@ -79,7 +79,7 @@ struct CallLibuvFunction { // Detect `void uv_foo(uv_req_t* request, ...);`. template struct CallLibuvFunction { - typedef void(*T)(ReqT*, Args...); + using T= void(*)(ReqT*, Args...); template static int Call(T fn, uv_loop_t* loop, ReqT* req, PassedArgs... args) { fn(req, args...); From 2738964dfe82b81199300fe62ac0c7d6fe90a16f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 27 Apr 2018 20:07:49 +0200 Subject: [PATCH 08/21] [squash] fixup for linter --- src/req_wrap-inl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/req_wrap-inl.h b/src/req_wrap-inl.h index 03b735bf710369..54abf74430f6af 100644 --- a/src/req_wrap-inl.h +++ b/src/req_wrap-inl.h @@ -79,7 +79,7 @@ struct CallLibuvFunction { // Detect `void uv_foo(uv_req_t* request, ...);`. template struct CallLibuvFunction { - using T= void(*)(ReqT*, Args...); + using T = void(*)(ReqT*, Args...); template static int Call(T fn, uv_loop_t* loop, ReqT* req, PassedArgs... args) { fn(req, args...); From 7e4114d67b712671d1b48e2f0e63fd395abe5a39 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 25 Sep 2017 22:53:17 +0200 Subject: [PATCH 09/21] src: keep track of open requests Workers cannot shut down while requests are open, so keep a counter that is increased whenever libuv requests are made and decreased whenever their callback is called. This also applies to other embedders, who may want to shut down an `Environment` instance early. Many thanks for Stephen Belanger for reviewing the original version of this commit in the Ayo.js project. Fixes: https://github.com/nodejs/node/issues/20517 Refs: https://github.com/ayojs/ayo/pull/85 --- src/env-inl.h | 9 +++++++++ src/env.cc | 6 ++++-- src/env.h | 6 +++++- src/node_api.cc | 5 +++++ src/node_crypto.cc | 13 +++++++++++-- src/node_zlib.cc | 13 ++++++++++--- src/req_wrap-inl.h | 43 ++++++++++++++++++++++++++++--------------- src/req_wrap.h | 2 ++ src/util.h | 10 +++++++++- 9 files changed, 83 insertions(+), 24 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 917ddd1b6bcb75..f115656353cff3 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -371,6 +371,15 @@ inline void Environment::CloseHandle(T* handle, OnCloseCallback callback) { }); } +void Environment::IncreaseWaitingRequestCounter() { + request_waiting_++; +} + +void Environment::DecreaseWaitingRequestCounter() { + request_waiting_--; + CHECK_GE(request_waiting_, 0); +} + inline uv_loop_t* Environment::event_loop() const { return isolate_data()->event_loop(); } diff --git a/src/env.cc b/src/env.cc index 6526c680ac1792..e5b9c0fd6aad4e 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) { @@ -241,8 +240,11 @@ void Environment::CleanupHandles() { hc.cb_(this, hc.handle_, hc.arg_); handle_cleanup_queue_.clear(); - while (handle_cleanup_waiting_ != 0 || !handle_wrap_queue_.IsEmpty()) + while (handle_cleanup_waiting_ != 0 || + request_waiting_ != 0 || + !handle_wrap_queue_.IsEmpty()) { uv_run(event_loop(), UV_RUN_ONCE); + } } void Environment::StartProfilerIdleNotifier() { diff --git a/src/env.h b/src/env.h index 79351666c1182e..de3014249ea852 100644 --- a/src/env.h +++ b/src/env.h @@ -601,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(); @@ -833,7 +836,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; diff --git a/src/node_api.cc b/src/node_api.cc index d5437d70d933ed..91a47a12d92751 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -3388,6 +3388,9 @@ class Work : public node::AsyncResource { // 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); + node::Environment* env_ = node::Environment::GetCurrent(env->isolate); + env_->DecreaseWaitingRequestCounter(); + CallbackScope callback_scope(work); NAPI_CALL_INTO_MODULE(env, @@ -3488,6 +3491,8 @@ napi_status napi_queue_async_work(napi_env env, napi_async_work work) { uvimpl::Work* w = reinterpret_cast(work); + node::Environment* env_ = node::Environment::GetCurrent(env->isolate); + env_->IncreaseWaitingRequestCounter(); CALL_UV(env, uv_queue_work(event_loop, w->Request(), uvimpl::Work::ExecuteCallback, diff --git a/src/node_crypto.cc b/src/node_crypto.cc index f611f81f16a819..10e4f593914ae8 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -4639,9 +4639,12 @@ 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->env()->DecreaseWaitingRequestCounter(); + if (status == UV_ECANCELED) + return; + CHECK_EQ(status, 0); req->After(); } @@ -4692,6 +4695,7 @@ void PBKDF2(const FunctionCallbackInfo& args) { if (args[5]->IsFunction()) { obj->Set(env->context(), env->ondone_string(), args[5]).FromJust(); + env->IncreaseWaitingRequestCounter(); uv_queue_work(env->event_loop(), req.release()->work_req(), PBKDF2Request::Work, @@ -4831,10 +4835,13 @@ void RandomBytesCheck(RandomBytesRequest* req, Local (*argv)[2]) { void RandomBytesAfter(uv_work_t* work_req, int status) { - CHECK_EQ(status, 0); std::unique_ptr req( ContainerOf(&RandomBytesRequest::work_req_, work_req)); Environment* env = req->env(); + env->DecreaseWaitingRequestCounter(); + if (status == UV_ECANCELED) + return; + CHECK_EQ(status, 0); HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); Local argv[2]; @@ -4874,6 +4881,7 @@ void RandomBytes(const FunctionCallbackInfo& args) { if (args[1]->IsFunction()) { obj->Set(env->context(), env->ondone_string(), args[1]).FromJust(); + env->IncreaseWaitingRequestCounter(); uv_queue_work(env->event_loop(), req.release()->work_req(), RandomBytesWork, @@ -4913,6 +4921,7 @@ void RandomBytesBuffer(const FunctionCallbackInfo& args) { if (args[3]->IsFunction()) { obj->Set(env->context(), env->ondone_string(), args[3]).FromJust(); + env->IncreaseWaitingRequestCounter(); uv_queue_work(env->event_loop(), req.release()->work_req(), RandomBytesWork, diff --git a/src/node_zlib.cc b/src/node_zlib.cc index ec447638e2ae62..3249905dfbfaf9 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -214,6 +214,7 @@ class ZCtx : public AsyncWrap { } // async version + env->IncreaseWaitingRequestCounter(); uv_queue_work(env->event_loop(), work_req, ZCtx::Process, ZCtx::After); } @@ -361,10 +362,17 @@ 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); Environment* env = ctx->env(); + ctx->write_in_progress_ = false; + + env->DecreaseWaitingRequestCounter(); + if (status == UV_ECANCELED) { + ctx->Close(); + return; + } + + CHECK_EQ(status, 0); HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); @@ -374,7 +382,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(), diff --git a/src/req_wrap-inl.h b/src/req_wrap-inl.h index 54abf74430f6af..e3b26c1f5c6030 100644 --- a/src/req_wrap-inl.h +++ b/src/req_wrap-inl.h @@ -20,6 +20,8 @@ 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 @@ -33,6 +35,12 @@ 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); @@ -40,7 +48,8 @@ ReqWrap* ReqWrap::from_req(T* req) { template void ReqWrap::Cancel() { - uv_cancel(reinterpret_cast(&req_)); + 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 @@ -95,7 +104,7 @@ struct CallLibuvFunction { template struct MakeLibuvRequestCallback { static T For(ReqWrap* req_wrap, T v) { - static_assert(!std::is_function::value, + static_assert(!is_callable::value, "MakeLibuvRequestCallback missed a callback"); return v; } @@ -109,6 +118,7 @@ struct MakeLibuvRequestCallback { 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...); } @@ -128,23 +138,26 @@ int ReqWrap::Dispatch(LibuvFunction fn, Args... args) { // This expands as: // - // return 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 - return CallLibuvFunction::Call( + // 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 diff --git a/src/req_wrap.h b/src/req_wrap.h index d181817218227b..8f8d0cf2885594 100644 --- a/src/req_wrap.h +++ b/src/req_wrap.h @@ -20,6 +20,8 @@ class ReqWrap : public AsyncWrap { // 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(); 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 From 291d97c7aa3f78b6bdc9b6be3ce608d8f21eae67 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 9 Sep 2017 22:29:08 +0200 Subject: [PATCH 10/21] src: use cleanup hooks to tear down BaseObjects Clean up after `BaseObject` instances when the `Environment` is being shut down. This takes care of closing non-libuv resources like `zlib` instances, which do not require asynchronous shutdown. Many thanks for Stephen Belanger, Timothy Gu and Alexey Orlenko for reviewing the original version of this commit in the Ayo.js project. Refs: https://github.com/ayojs/ayo/pull/88 --- src/base_object-inl.h | 9 +++++++++ src/base_object.h | 2 ++ src/env.cc | 6 ++++++ src/inspector_agent.cc | 2 ++ src/node.cc | 3 ++- src/req_wrap-inl.h | 1 - 6 files changed, 21 insertions(+), 2 deletions(-) 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/env.cc b/src/env.cc index e5b9c0fd6aad4e..ab5de3e2ee1b1d 100644 --- a/src/env.cc +++ b/src/env.cc @@ -133,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 @@ -245,6 +249,8 @@ void Environment::CleanupHandles() { !handle_wrap_queue_.IsEmpty()) { uv_run(event_loop(), UV_RUN_ONCE); } + + file_handle_read_wrap_freelist_.clear(); } void Environment::StartProfilerIdleNotifier() { 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 91ffb8c3f5592e..794888c80f2147 100644 --- a/src/node.cc +++ b/src/node.cc @@ -4549,12 +4549,13 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, const int exit_code = EmitExit(&env); + WaitForInspectorDisconnect(&env); + 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/req_wrap-inl.h b/src/req_wrap-inl.h index e3b26c1f5c6030..7e9e2d9fbbf912 100644 --- a/src/req_wrap-inl.h +++ b/src/req_wrap-inl.h @@ -26,7 +26,6 @@ ReqWrap::ReqWrap(Environment* env, template ReqWrap::~ReqWrap() { - CHECK_EQ(req_.data, this); // Assert that someone has called Dispatched(). CHECK_EQ(false, persistent().IsEmpty()); } From 20468519b090160ef1b1a132d037a1799bfd84b4 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 20 Sep 2017 14:43:19 +0200 Subject: [PATCH 11/21] src: add can_call_into_js flag This prevents calls back into JS from the shutdown phase. Many thanks for Stephen Belanger for reviewing the original version of this commit in the Ayo.js project. Refs: https://github.com/ayojs/ayo/pull/82 --- src/async_wrap.cc | 7 +++++-- src/env-inl.h | 8 ++++++++ src/env.h | 7 +++++++ src/node.cc | 9 +++++++++ src/node_contextify.cc | 2 ++ 5 files changed, 31 insertions(+), 2 deletions(-) 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/env-inl.h b/src/env-inl.h index f115656353cff3..0268879f5c7823 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -559,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(); } diff --git a/src/env.h b/src/env.h index de3014249ea852..15d417ba606860 100644 --- a/src/env.h +++ b/src/env.h @@ -679,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); @@ -821,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_; diff --git a/src/node.cc b/src/node.cc index 794888c80f2147..0a1b35bfc591ca 100644 --- a/src/node.cc +++ b/src/node.cc @@ -953,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); @@ -996,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(); } @@ -1013,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; @@ -4551,6 +4559,7 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, WaitForInspectorDisconnect(&env); + env.set_can_call_into_js(false); env.RunCleanup(); RunAtExit(&env); 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."); From 18ee5b993002bda238e21a4696b60682288ecbac Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 15 Mar 2018 21:39:57 +0100 Subject: [PATCH 12/21] tools: remove `--quiet` from run-valgrind.py This should no longer be an issue, now that we clean up resources when exiting. --- tools/run-valgrind.py | 3 --- 1 file changed, 3 deletions(-) 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: From 8db8cc872bc8752aa5c411e6ca7ad25bb7be447d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 17 Mar 2018 22:14:51 +0100 Subject: [PATCH 13/21] process: create stdin with `manualStart: true` Otherwise Node.js will try to read data from the handle. This causes issues when Node.js is already reading from the same handle, but a different associated stream (e.g. a possible IPC channel). --- lib/internal/process/stdio.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/internal/process/stdio.js b/lib/internal/process/stdio.js index 75ede6a8e7e157..9a16a5ab15f166 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 From 32e3770e2bac42670dc896ffa97de996f837c347 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 17 Mar 2018 22:34:38 +0100 Subject: [PATCH 14/21] src: store fd for libuv streams on Windows On Windows, we can't just look up a FD for libuv streams and return it in `GetFD()`. However, we do sometimes construct streams from their FDs; in those cases, it should be okay to store the value on a class field. --- src/pipe_wrap.cc | 1 + src/stream_wrap.cc | 6 ++++-- src/stream_wrap.h | 18 ++++++++++++++++++ src/tcp_wrap.cc | 1 + src/tty_wrap.cc | 1 + 5 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/pipe_wrap.cc b/src/pipe_wrap.cc index 7ec5bdf15be9cc..db29e1eb463620 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->store_fd(fd); if (err != 0) env->isolate()->ThrowException(UVException(err, "uv_pipe_open")); diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index ad708c9ed28def..0e700ba39a6b95 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 } diff --git a/src/stream_wrap.h b/src/stream_wrap.h index a97e8ba10f91d5..a5b537ec89ad0e 100644 --- a/src/stream_wrap.h +++ b/src/stream_wrap.h @@ -88,6 +88,14 @@ class LibuvStreamWrap : public HandleWrap, public StreamBase { v8::Local target, int flags = StreamBase::kFlagNone); + protected: + inline void store_fd(int fd) { +#ifdef _WIN32 + fd_ = fd; +#endif + } + + private: static void GetWriteQueueSize( const v8::FunctionCallbackInfo& info); @@ -101,6 +109,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 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 70c60fa47cc68f..2eb8d8038518a0 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->store_fd(fd); } diff --git a/src/tty_wrap.cc b/src/tty_wrap.cc index d01caba4a558f2..cbcfbd8e091360 100644 --- a/src/tty_wrap.cc +++ b/src/tty_wrap.cc @@ -172,6 +172,7 @@ TTYWrap::TTYWrap(Environment* env, reinterpret_cast(&handle_), AsyncWrap::PROVIDER_TTYWRAP) { *init_err = uv_tty_init(env->event_loop(), &handle_, fd, readable); + store_fd(fd); if (*init_err != 0) MarkAsUninitialized(); } From 8bed0afacfb27af0dc2a567abe9e3b6c17af817c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 7 May 2018 23:01:54 +0200 Subject: [PATCH 15/21] =?UTF-8?q?[squash]=20store=5Ffd=20=E2=86=92=20set?= =?UTF-8?q?=5Ffd?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/pipe_wrap.cc | 2 +- src/stream_wrap.h | 2 +- src/tcp_wrap.cc | 2 +- src/tty_wrap.cc | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/pipe_wrap.cc b/src/pipe_wrap.cc index db29e1eb463620..f3a88a2ecc80c3 100644 --- a/src/pipe_wrap.cc +++ b/src/pipe_wrap.cc @@ -204,7 +204,7 @@ void PipeWrap::Open(const FunctionCallbackInfo& args) { int fd = args[0]->Int32Value(); int err = uv_pipe_open(&wrap->handle_, fd); - wrap->store_fd(fd); + wrap->set_fd(fd); if (err != 0) env->isolate()->ThrowException(UVException(err, "uv_pipe_open")); diff --git a/src/stream_wrap.h b/src/stream_wrap.h index a5b537ec89ad0e..0869c06d53fa34 100644 --- a/src/stream_wrap.h +++ b/src/stream_wrap.h @@ -89,7 +89,7 @@ class LibuvStreamWrap : public HandleWrap, public StreamBase { int flags = StreamBase::kFlagNone); protected: - inline void store_fd(int fd) { + inline void set_fd(int fd) { #ifdef _WIN32 fd_ = fd; #endif diff --git a/src/tcp_wrap.cc b/src/tcp_wrap.cc index 2eb8d8038518a0..8200353b17bf99 100644 --- a/src/tcp_wrap.cc +++ b/src/tcp_wrap.cc @@ -212,7 +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->store_fd(fd); + wrap->set_fd(fd); } diff --git a/src/tty_wrap.cc b/src/tty_wrap.cc index cbcfbd8e091360..cd8589cc7fc2e7 100644 --- a/src/tty_wrap.cc +++ b/src/tty_wrap.cc @@ -172,7 +172,7 @@ TTYWrap::TTYWrap(Environment* env, reinterpret_cast(&handle_), AsyncWrap::PROVIDER_TTYWRAP) { *init_err = uv_tty_init(env->event_loop(), &handle_, fd, readable); - store_fd(fd); + set_fd(fd); if (*init_err != 0) MarkAsUninitialized(); } From 2aab562a6e9adf7d6965f012e3bb9e1a8cacac21 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 7 May 2018 23:02:40 +0200 Subject: [PATCH 16/21] [squash] add missing "have" --- src/stream_wrap.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/stream_wrap.h b/src/stream_wrap.h index 0869c06d53fa34..7847ebe754614a 100644 --- a/src/stream_wrap.h +++ b/src/stream_wrap.h @@ -111,7 +111,7 @@ class LibuvStreamWrap : public HandleWrap, public StreamBase { uv_stream_t* const stream_; #ifdef _WIN32 - // We don't always an FD that we could look up on the stream_ + // 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 From b1110a0a09adb92d786ce18be9060d46ae9084c7 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 27 Apr 2018 00:16:09 +0200 Subject: [PATCH 17/21] src: remove NodeCategorySet destructor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This currently crashes during environment cleanup because the object would be torn down while there are enabled categories. I’m not sure about the exact semantics here, but since the object cannot be garbage collected at this point anyway because it’s `Persistent` handle is strong, removing the destructor at least doesn’t make anything worse than it is right now (i.e. the destructor would never have been called before anyway). --- src/node_trace_events.cc | 5 ----- 1 file changed, 5 deletions(-) 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); From 204792fd1cbc2f5748be2c079b3479484208b1e3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 24 Mar 2018 23:11:10 +0100 Subject: [PATCH 18/21] lib: defer pausing stdin to the next tick This is done to match the stream implementation, which also only actually stops reading in the next tick after the `'pause'` event is emitted. --- lib/internal/process/stdio.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/internal/process/stdio.js b/lib/internal/process/stdio.js index 9a16a5ab15f166..3fc299777873e9 100644 --- a/lib/internal/process/stdio.js +++ b/lib/internal/process/stdio.js @@ -112,12 +112,18 @@ function setupStdio() { // if the user calls stdin.pause(), then we need to stop reading // immediately, 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; } From fae778b264fd4db991aefd0bcf4e6876d13f3630 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 7 May 2018 22:58:38 +0200 Subject: [PATCH 19/21] [squash] adjust comment for stdin onpause --- lib/internal/process/stdio.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/internal/process/stdio.js b/lib/internal/process/stdio.js index 3fc299777873e9..ce84142938f066 100644 --- a/lib/internal/process/stdio.js +++ b/lib/internal/process/stdio.js @@ -109,8 +109,9 @@ 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); }); From e355d1157324ac7e1a93ed9030684dd82eceefab Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 24 Mar 2018 23:12:22 +0100 Subject: [PATCH 20/21] src: always call ReadStop() before Close() For libuv-backed streams, always explicitly stop reading before closing the handle. --- src/handle_wrap.h | 3 ++- src/stream_wrap.cc | 5 +++++ src/stream_wrap.h | 2 ++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/handle_wrap.h b/src/handle_wrap.h index fd2d002dce0338..b2b09f5010d1f7 100644 --- a/src/handle_wrap.h +++ b/src/handle_wrap.h @@ -70,7 +70,8 @@ class HandleWrap : public AsyncWrap { inline uv_handle_t* GetHandle() const { return handle_; } - void Close(v8::Local close_callback = v8::Local()); + virtual void Close( + v8::Local close_callback = v8::Local()); protected: HandleWrap(Environment* env, diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index 0e700ba39a6b95..cdcbe574f9ae5f 100644 --- a/src/stream_wrap.cc +++ b/src/stream_wrap.cc @@ -373,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 7847ebe754614a..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, From 29e0584f0469d7c4d12a1fdca9c49804c7967749 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 9 May 2018 17:40:24 +0200 Subject: [PATCH 21/21] src: unify thread pool work Instead of using the libuv mechanism directly, provide an internal `ThreadPoolWork` wrapper that takes care of increasing/decreasing the waiting request counter. --- src/node_api.cc | 73 +++++++++++++------------------- src/node_crypto.cc | 99 ++++++++++++++------------------------------ src/node_internals.h | 35 ++++++++++++++++ src/node_zlib.cc | 29 ++++++------- 4 files changed, 108 insertions(+), 128 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index 91a47a12d92751..b456ade2d4ca74 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -3338,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, @@ -3349,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, @@ -3374,47 +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); - node::Environment* env_ = node::Environment::GetCurrent(env->isolate); - env_->DecreaseWaitingRequestCounter(); + CallbackScope callback_scope(this); - CallbackScope callback_scope(work); + 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); + }); - 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. - } - } - - 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; }; @@ -3491,12 +3479,7 @@ napi_status napi_queue_async_work(napi_env env, napi_async_work work) { uvimpl::Work* w = reinterpret_cast(work); - node::Environment* env_ = node::Environment::GetCurrent(env->isolate); - env_->IncreaseWaitingRequestCounter(); - CALL_UV(env, uv_queue_work(event_loop, - w->Request(), - uvimpl::Work::ExecuteCallback, - uvimpl::Work::CompleteCallback)); + w->ScheduleWork(); return napi_clear_last_error(env); } @@ -3507,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_crypto.cc b/src/node_crypto.cc index 10e4f593914ae8..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,17 +4631,6 @@ void PBKDF2Request::After() { } -void PBKDF2Request::After(uv_work_t* work_req, int status) { - std::unique_ptr req( - ContainerOf(&PBKDF2Request::work_req_, work_req)); - req->env()->DecreaseWaitingRequestCounter(); - if (status == UV_ECANCELED) - return; - CHECK_EQ(status, 0); - req->After(); -} - - void PBKDF2(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -4695,14 +4677,10 @@ void PBKDF2(const FunctionCallbackInfo& args) { if (args[5]->IsFunction()) { obj->Set(env->context(), env->ondone_string(), args[5]).FromJust(); - env->IncreaseWaitingRequestCounter(); - 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); @@ -4715,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 }; @@ -4725,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_; } @@ -4772,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) @@ -4782,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) } } @@ -4834,19 +4806,16 @@ void RandomBytesCheck(RandomBytesRequest* req, Local (*argv)[2]) { } -void RandomBytesAfter(uv_work_t* work_req, int status) { - std::unique_ptr req( - ContainerOf(&RandomBytesRequest::work_req_, work_req)); - Environment* env = req->env(); - env->DecreaseWaitingRequestCounter(); +void RandomBytesRequest::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()); + 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); } @@ -4854,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()) @@ -4881,11 +4850,7 @@ void RandomBytes(const FunctionCallbackInfo& args) { if (args[1]->IsFunction()) { obj->Set(env->context(), env->ondone_string(), args[1]).FromJust(); - env->IncreaseWaitingRequestCounter(); - uv_queue_work(env->event_loop(), - req.release()->work_req(), - RandomBytesWork, - RandomBytesAfter); + req.release()->ScheduleWork(); args.GetReturnValue().Set(obj); } else { Local argv[2]; @@ -4921,11 +4886,7 @@ void RandomBytesBuffer(const FunctionCallbackInfo& args) { if (args[3]->IsFunction()) { obj->Set(env->context(), env->ondone_string(), args[3]).FromJust(); - env->IncreaseWaitingRequestCounter(); - 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_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_zlib.cc b/src/node_zlib.cc index 3249905dfbfaf9..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,18 +212,24 @@ class ZCtx : public AsyncWrap { } // async version - env->IncreaseWaitingRequestCounter(); - 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 @@ -361,12 +365,10 @@ class ZCtx : public AsyncWrap { // v8 land! - static void After(uv_work_t* work_req, int status) { - ZCtx* ctx = ContainerOf(&ZCtx::work_req_, work_req); + static void After(ZCtx* ctx, int status) { Environment* env = ctx->env(); ctx->write_in_progress_ = false; - env->DecreaseWaitingRequestCounter(); if (status == UV_ECANCELED) { ctx->Close(); return; @@ -685,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_;