n-api: fix win32 main thread detection #32823
Conversation
nodejs-github-bot
added
c++
`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 <gabriel.schulhof@intel.com>
gabrielschulhof
force-pushed
the
tsfn-fix-win32-thread-self
branch
from
1d5c7f8
to
55e9481
Compare
|
I don't think this is a sound change, it turns Alternative solution: use a With a |
|
@bnoordhuis I'll try the |
|
... aaaand you can't make instance variables thread-local (at least AFAICT), so I'll go with the full API separation approach. |
|
@addaleax @bnoordhuis I changed the implementation to not abuse the libuv API. |
|
|
||
| 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 (current_thread == main_thread) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@gireeshpunathil No, because this function may be called from a thread that does not have access to an instance of node::Environment.
Co-Authored-By: Anna Henningsen <github@addaleax.net>
|
@addaleax suggestions applied 👍 |
Yes, I was talking about a static class member because the There's only one main thread in a process; it looks like what is meant here is more accurately called the home thread. Just wondering, didn't the |
|
@bnoordhuis the problem is that I would need one key per instance of a thread-safe function, whereas by my understanding |
|
@bnoordhuis I'll rename |
|
@bnoordhuis I think I grok what you're saying 🙂 Testing locally now. |
|
@bnoordhuis I have created #32860 as an alternative. I'll keep it in draft until I've had a chance to look at the CI results. |
|
Closing in favour of #32860. |
uv_thread_self()works on Windows only for threads created usinguv_thread_start()because libuv does not useGetCurrentThreadId()for threads that were created otherwise.
uv_thread_equal()workscorrectly.
Thus, on Windows we use
GetCurrentThreadId()to compare the mainthread with the current thread.
Signed-off-by: @gabrielschulhof
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes