Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

n-api: fix win32 main thread detection #32823

Closed
wants to merge 5 commits into from
Closed

n-api: fix win32 main thread detection #32823

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
55e9481
n-api: fix win32 main thread detection
Apr 13, 2020
eb3231f
ifdef the rest of the API
Apr 13, 2020
0678182
convert to class
Apr 13, 2020
8980c7b
simplify definition
Apr 13, 2020
2e61c71
add consts
Apr 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 29 additions & 4 deletions src/node_api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,32 @@ 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()`.
class ThreadID {
public:
#ifdef _WIN32
typedef DWORD thread_id_t;
ThreadID(): _tid(GetCurrentThreadId()) {}
inline bool operator==(const ThreadID& other) { return _tid == other._tid; }
gabrielschulhof marked this conversation as resolved.
Show resolved Hide resolved
#else
typedef uv_thread_t thread_id_t;
ThreadID(): _tid(uv_thread_self()) {}
inline bool operator==(const ThreadID& other) {
gabrielschulhof marked this conversation as resolved.
Show resolved Hide resolved
return static_cast<bool>(uv_thread_equal(&_tid, &other._tid));
}
#endif // _WIN32

ThreadID(const ThreadID&) = delete;
ThreadID(ThreadID&&) = delete;
void operator= (const ThreadID&) = delete;
void operator= (const ThreadID&&) = delete;

private:
thread_id_t _tid;
};

class ThreadSafeFunction : public node::AsyncResource {
public:
ThreadSafeFunction(v8::Local<v8::Function> func,
Expand All @@ -129,7 +155,6 @@ class ThreadSafeFunction : public node::AsyncResource {
is_closing(false),
context(context_),
max_queue_size(max_queue_size_),
main_thread(uv_thread_self()),
env(env_),
finalize_data(finalize_data_),
finalize_cb(finalize_cb_),
Expand All @@ -149,14 +174,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 = uv_thread_self();
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 (uv_thread_equal(&current_thread, &main_thread)) {
} else if (current_thread == main_thread) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to do something like env_->is_main_thread() (worker thread's approach)? in which case we can potentially remove the ThreadID class as well as the main_thread instance variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gireeshpunathil No, because this function may be called from a thread that does not have access to an instance of node::Environment.

return napi_would_deadlock;
}
cond->Wait(lock);
Expand Down Expand Up @@ -438,7 +463,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;
ThreadID main_thread;

// These are variables accessed only from the loop thread.
v8impl::Persistent<v8::Function> ref;
Expand Down