n-api: fix dead lock if multi push events #58724
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
@milkpotatoes would it be possible to add a test case? That would help use see the problem more easily as well as make sure its not regressed in the future. |
- add waiting_task_size to record waiting push events - compare max_queue_size before queue.size which would be faster Signed-off-by: milkpotatoes <chenshulin5@huawei.com>
milkpotatoes
force-pushed
the
fix-ntf-tsfn-full
branch
from
3547016 to
e2014e6
Compare
| @@ -518,6 +521,7 @@ class ThreadSafeFunction : public node::AsyncResource { | |||
| // means we don't need the mutex to read them. | |||
| void* context; | |||
| size_t max_queue_size; | |||
| size_t waiting_task_size; | |||
There was a problem hiding this comment.
This seems like it adds an extra variable when we could also just call Signal() unconditionally with the same effect? This class already has a very large state space (one of the worst examples I've seen), I'd try to avoid another piece of state even if its usage is localized to managing cond
There was a problem hiding this comment.
Yes, this approach seems acceptable. However, it may introduce unnecessary signal calls, which could incur additional performance overhead.
fix #58723