napi_call_threadsafe_function should error if called from the main thread

[josephg]

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.

[himself65]

the reason I think is when ThreadSafeFunction (tsfn) called by the main thread and the queue
also 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

cond->Wait(lock);

[josephg]

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]

cc @gabrielschulhof

[gabrielschulhof]

@nodejs/n-api we should discuss this during our next meeting.

[gabrielschulhof]

@josephg dequeuing one item synchronously to make room for the incoming item might be one solution ...

[josephg]

Could be; but that sounds complicated. And the javascript code you call might synchronously queue another item or two in turn.