From 55e948100971f9c384b8ce3f4f4891f4868ddc14 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Mon, 13 Apr 2020 11:08:13 -0700 Subject: [PATCH 1/5] n-api: fix win32 main thread detection `uv_thread_self()` works on Windows only for threads created using `uv_thread_start()` because libuv does not use `GetCurrentThreadId()` for threads that were created otherwise. `uv_thread_equal()` works correctly. Thus, on Windows we use `GetCurrentThreadId()` to compare the main thread with the current thread. Signed-off-by: Gabriel Schulhof --- src/node_api.cc | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index 552538c6f05fcf..987fd6bc3a43c9 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -110,6 +110,15 @@ static inline void trigger_fatal_exception( node::errors::TriggerUncaughtException(env->isolate, local_err, local_msg); } +// `uv_thread_self()` returns 0 on Windows for threads that were not created +// using `uv_thread_start()`. Thus, for correct comparison, we need to use +// `GetCurrentThreadId()`. +#ifdef _WIN32 +#define THREAD_SELF_API reinterpret_cast(GetCurrentThreadId()) +#else +#define THREAD_SELF_API uv_thread_self() +#endif // _WIN32 + class ThreadSafeFunction : public node::AsyncResource { public: ThreadSafeFunction(v8::Local func, @@ -129,7 +138,7 @@ class ThreadSafeFunction : public node::AsyncResource { is_closing(false), context(context_), max_queue_size(max_queue_size_), - main_thread(uv_thread_self()), + main_thread(THREAD_SELF_API), env(env_), finalize_data(finalize_data_), finalize_cb(finalize_cb_), @@ -149,7 +158,7 @@ class ThreadSafeFunction : public node::AsyncResource { napi_status Push(void* data, napi_threadsafe_function_call_mode mode) { node::Mutex::ScopedLock lock(this->mutex); - uv_thread_t current_thread = uv_thread_self(); + uv_thread_t current_thread = THREAD_SELF_API; while (queue.size() >= max_queue_size && max_queue_size > 0 && From eb3231ff653401d658430d4cee52d3e063f6aebd Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Mon, 13 Apr 2020 15:46:48 -0700 Subject: [PATCH 2/5] ifdef the rest of the API --- src/node_api.cc | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index 987fd6bc3a43c9..0c777a8d6c4a2a 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -114,9 +114,13 @@ static inline void trigger_fatal_exception( // using `uv_thread_start()`. Thus, for correct comparison, we need to use // `GetCurrentThreadId()`. #ifdef _WIN32 -#define THREAD_SELF_API reinterpret_cast(GetCurrentThreadId()) +typedef DWORD thread_id_t; +#define TSFN_SELF_TID GetCurrentThreadId() +#define TSFN_COMPARE_TIDS(id1, id2) ((id1) == (id2)) #else -#define THREAD_SELF_API uv_thread_self() +typedef uv_thread_t thread_id_t; +#define TSFN_SELF_TID uv_thread_self() +#define TSFN_COMPARE_TIDS(id1, id2) uv_thread_equal((&(id1)), (&(id2))) #endif // _WIN32 class ThreadSafeFunction : public node::AsyncResource { @@ -138,7 +142,7 @@ class ThreadSafeFunction : public node::AsyncResource { is_closing(false), context(context_), max_queue_size(max_queue_size_), - main_thread(THREAD_SELF_API), + main_thread(TSFN_SELF_TID), env(env_), finalize_data(finalize_data_), finalize_cb(finalize_cb_), @@ -158,14 +162,14 @@ class ThreadSafeFunction : public node::AsyncResource { napi_status Push(void* data, napi_threadsafe_function_call_mode mode) { node::Mutex::ScopedLock lock(this->mutex); - uv_thread_t current_thread = THREAD_SELF_API; + thread_id_t current_thread = TSFN_SELF_TID; while (queue.size() >= max_queue_size && max_queue_size > 0 && !is_closing) { if (mode == napi_tsfn_nonblocking) { return napi_queue_full; - } else if (uv_thread_equal(¤t_thread, &main_thread)) { + } else if (TSFN_COMPARE_TIDS(¤t_thread, &main_thread)) { return napi_would_deadlock; } cond->Wait(lock); @@ -447,7 +451,7 @@ class ThreadSafeFunction : public node::AsyncResource { // means we don't need the mutex to read them. void* context; size_t max_queue_size; - uv_thread_t main_thread; + thread_id_t main_thread; // These are variables accessed only from the loop thread. v8impl::Persistent ref; From 06781824baf1e970ef660962ed4ff4d8d1353dc8 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Mon, 13 Apr 2020 16:05:33 -0700 Subject: [PATCH 3/5] convert to class --- src/node_api.cc | 38 ++++++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index 0c777a8d6c4a2a..d01eed9aa14d99 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -113,15 +113,34 @@ static inline void trigger_fatal_exception( // `uv_thread_self()` returns 0 on Windows for threads that were not created // using `uv_thread_start()`. Thus, for correct comparison, we need to use // `GetCurrentThreadId()`. +class ThreadID { + public: +#ifdef _WIN32 + ThreadID(): _tid(GetCurrentThreadId()) {} +#else + ThreadID(): _tid(uv_thread_self()) {} +#endif // _WIN32 + + ThreadID(const ThreadID&) = delete; + ThreadID(ThreadID&&) = delete; + void operator= (const ThreadID&) = delete; + void operator= (const ThreadID&&) = delete; + #ifdef _WIN32 -typedef DWORD thread_id_t; -#define TSFN_SELF_TID GetCurrentThreadId() -#define TSFN_COMPARE_TIDS(id1, id2) ((id1) == (id2)) + inline bool operator==(const ThreadID& other) { return _tid == other._tid; } #else -typedef uv_thread_t thread_id_t; -#define TSFN_SELF_TID uv_thread_self() -#define TSFN_COMPARE_TIDS(id1, id2) uv_thread_equal((&(id1)), (&(id2))) + inline bool operator==(const ThreadID& other) { + return static_cast(uv_thread_equal(&_tid, &other._tid)); + } +#endif + + private: +#ifdef _WIN32 + DWORD _tid = 0; +#else + uv_thread_t _tid; #endif // _WIN32 +}; class ThreadSafeFunction : public node::AsyncResource { public: @@ -142,7 +161,6 @@ class ThreadSafeFunction : public node::AsyncResource { is_closing(false), context(context_), max_queue_size(max_queue_size_), - main_thread(TSFN_SELF_TID), env(env_), finalize_data(finalize_data_), finalize_cb(finalize_cb_), @@ -162,14 +180,14 @@ class ThreadSafeFunction : public node::AsyncResource { napi_status Push(void* data, napi_threadsafe_function_call_mode mode) { node::Mutex::ScopedLock lock(this->mutex); - thread_id_t current_thread = TSFN_SELF_TID; + ThreadID current_thread; while (queue.size() >= max_queue_size && max_queue_size > 0 && !is_closing) { if (mode == napi_tsfn_nonblocking) { return napi_queue_full; - } else if (TSFN_COMPARE_TIDS(¤t_thread, &main_thread)) { + } else if (current_thread == main_thread) { return napi_would_deadlock; } cond->Wait(lock); @@ -451,7 +469,7 @@ class ThreadSafeFunction : public node::AsyncResource { // means we don't need the mutex to read them. void* context; size_t max_queue_size; - thread_id_t main_thread; + ThreadID main_thread; // These are variables accessed only from the loop thread. v8impl::Persistent ref; From 8980c7b27213aa0f76e83e9cacc70dbdea8f75f3 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Mon, 13 Apr 2020 16:24:27 -0700 Subject: [PATCH 4/5] simplify definition --- src/node_api.cc | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index d01eed9aa14d99..8a56d68d07b33c 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -116,9 +116,15 @@ static inline void trigger_fatal_exception( class ThreadID { public: #ifdef _WIN32 + typedef DWORD thread_id_t; ThreadID(): _tid(GetCurrentThreadId()) {} + inline bool operator==(const ThreadID& other) { return _tid == other._tid; } #else + typedef uv_thread_t thread_id_t; ThreadID(): _tid(uv_thread_self()) {} + inline bool operator==(const ThreadID& other) { + return static_cast(uv_thread_equal(&_tid, &other._tid)); + } #endif // _WIN32 ThreadID(const ThreadID&) = delete; @@ -126,20 +132,8 @@ class ThreadID { void operator= (const ThreadID&) = delete; void operator= (const ThreadID&&) = delete; -#ifdef _WIN32 - inline bool operator==(const ThreadID& other) { return _tid == other._tid; } -#else - inline bool operator==(const ThreadID& other) { - return static_cast(uv_thread_equal(&_tid, &other._tid)); - } -#endif - private: -#ifdef _WIN32 - DWORD _tid = 0; -#else - uv_thread_t _tid; -#endif // _WIN32 + thread_id_t _tid; }; class ThreadSafeFunction : public node::AsyncResource { From 2e61c71f66e3737f6f9c5a9543fa3da04a9ba15c Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Tue, 14 Apr 2020 07:47:54 -0700 Subject: [PATCH 5/5] add consts Co-Authored-By: Anna Henningsen --- src/node_api.cc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index 8a56d68d07b33c..18adb1aadb245b 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -117,12 +117,14 @@ class ThreadID { public: #ifdef _WIN32 typedef DWORD thread_id_t; - ThreadID(): _tid(GetCurrentThreadId()) {} - inline bool operator==(const ThreadID& other) { return _tid == other._tid; } + ThreadID() : _tid(GetCurrentThreadId()) {} + inline bool operator==(const ThreadID& other) const { + return _tid == other._tid; + } #else typedef uv_thread_t thread_id_t; - ThreadID(): _tid(uv_thread_self()) {} - inline bool operator==(const ThreadID& other) { + ThreadID() : _tid(uv_thread_self()) {} + inline bool operator==(const ThreadID& other) const { return static_cast(uv_thread_equal(&_tid, &other._tid)); } #endif // _WIN32