Repository navigation
napi_call_threadsafe_function should error if called from the main thread #32615
Description
I spent the last few hours tracking down a deadlock one of my users was running into with my library. It was 100% a bug in my code - but I think napi could pretty easily protect module writers against the mistake I was making.
The problem was it turns out that (sometimes) I've been calling napi_call_threadsafe_function(fn, ctx, napi_tsfn_blocking) from nodejs's main thread. The problem is if you call that function with napi_tsfn_blocking when the queue is full, nodejs's main thread will end up deadlocked. (The call to call_threadsafe_function will block until there's room in the queue - but it can't clear room in the queue while its blocking waiting for room.)
Anyway, I think we should add a check in ThreadsafeFunction::Push to return an error if:
- Its called from nodejs's main thread.
- Its called with
napi_tsfn_blocking, and the queue has limited size
This would have saved me some time; and I suspect I'm not the only one who will accidentally misuse napi_call_threadsafe_function and land in trouble.
The compatibility constraints on this are interesting - its always wrong to call call_threadsafe_function like this, but many programs will run for awhile anyway, timebomb and all.
Activity
himself65 commentedon Apr 2, 2020
the reason I think is when
ThreadSafeFunction(tsfn) called by the main thread and the queuealso happened full, so deadlock appears.
for short, wait and notify are called by the same thread
node/src/node_api.cc
Line 158 in d3fb168
josephg commentedon Apr 2, 2020
Yes. Another option would be to call the JS function immediately without putting it in the queue. That would stop the deadlock behaviour but it breaks the (implicit?) contract that order is preserved between calls to napi_call_tsfn and JavaScript. (Although if you’re calling napi_call_tsfn from multiple threads you can’t really preserve order anyway, because there’s no guarantee of thread execution order.)
bnoordhuis commentedon Apr 3, 2020
cc @gabrielschulhof
gabrielschulhof commentedon Apr 3, 2020
@nodejs/n-api we should discuss this during our next meeting.
gabrielschulhof commentedon Apr 3, 2020
@josephg dequeuing one item synchronously to make room for the incoming item might be one solution ...
josephg commentedon Apr 4, 2020
Could be; but that sounds complicated. And the javascript code you call might synchronously queue another item or two in turn.
n-api: detect deadlocks in thread-safe function
n-api: detect deadlocks in thread-safe function
n-api: detect deadlocks in thread-safe function
n-api: detect deadlocks in thread-safe function
n-api: detect deadlocks in thread-safe function
n-api: detect deadlocks in thread-safe function
n-api: detect deadlocks in thread-safe function
n-api: detect deadlocks in thread-safe function
n-api: detect deadlocks in thread-safe function