Uh oh!
There was an error while loading. Please reload this page.
src: fix platform shutdown deadlock - #56827
Conversation
Each worker is signalling its own completion of tasks independently and so should only be signalling for one corresponding drain otherwise the count of outstanding tasks goes out of sync and the process will never stop waiting for tasks when it should be exiting. It just needs to be calling Signal rather than Broadcast.
nodejs-github-bot
commented
Jan 30, 2025
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@## main #56827 +/- ##
=======================================
Coverage 89.20% 89.20% =======================================
Files 663 663 Lines 192012 192012 Branches 36929 36933 +4 =======================================
+ Hits 171286 171293 +7 - Misses 13582 13590 +8 + Partials 7144 7129 -15
🚀 New features to boost your workflow:
|
targos
commented
Jan 30, 2025
This seems to bring new flakyness to |
Qard
commented
Jan 30, 2025
I think it may also have not actually fixed the issue, just made it more rare. I've been doing a very long run locally here. Before this I encountered the issue at 9.9k iterations, with this it eventually popped up at 117k iterations. |
Yes, unfortunately it does not seem to fix the issue: |
A |
Qard
commented
Jan 31, 2025
I suspect it's a spurious wakeup that's not handled safely and so the |
santigimeno
commented
Jan 31, 2025
By looking at the traces, it seems to me that the problem is that one of the tasks in the Platform workers never ends thus DrainTasks never ends. In the traces from #56827 (comment), the |
According to the V8 team, the issue is in Node.js, see https://issues.chromium.org/issues/374285493. |
santigimeno
commented
Feb 1, 2025
Thanks for the reference. Yes, I think the solution is not calling |
Qard
commented
Feb 1, 2025
That doesn't seem to be the case though as all the workers and the dispatcher are stuck in uv_cond_wait at the same time, indicating none of them are working, they're all waiting for each other. I think the issue might be with how |
Aren't they waiting in different cond variables though. Maybe I'm wrong, but they way I see it is:
So, |
Qard
commented
Feb 2, 2025
The main thread and the workers are all in uv_cond_wait at the same time though. If the V8 worker enters uv_cond_wait while main is already waiting then it will just continue to wait until main is ready before it can unlock and apply its own operation. Meanwhile though, the workers are all waiting as they seem to think there are zero tasks to process. If main was in uv_cond_wait at the time then it must have believed otherwise. For it to have reached It may be possible for the main thread and GC thread to mutually lock each other, but the workers would still pick up the values from BlockingPop, continue operating, and then signal the drain condvar. It doesn't make sense that they would get stuck in uv_cond_wait too unless the counts went out of sync and the main thread was waiting on a task that had not been delivered. |
santigimeno
commented
Feb 3, 2025
I mostly agree. Indeed all tasks have been sent and picked up by the workers, but there's only one left to complete as explained above ( |
But all workers are in uv_cond_wait here. Thread 1 is main, Thread 2 is the While It would be the case that after processing everything in the queue the worker threads would then all be in uv_cond_wait due to having nothing to operate on, but in that case |
targos
commented
Feb 4, 2025
It seems that #56842 has more test timeouts than usual. Maybe it will be easier for you to debug/reproduce with it? |
Qard
commented
Feb 6, 2025
@targos Thanks for the heads up! I'll see what I can do with that. I hope I can figure out a fix that doesn't involve a total rewrite of the platform subsystem. 🙈 |
mcollina
commented
Feb 10, 2025
@lpinca maybe we could always do that before shutting down? Could we trigger the gc manually from c++? |
I thought about that but I don't have the required knowledge to add anything substantial. It seems hacky. |
joyeecheung
commented
Feb 12, 2025
From the TSC meeting @mcollina asked whether there is a method to trigger GC from the C++ side, there is |
Fixes#54918
Each worker is signalling its own completion of tasks independently and so should only be signalling for one corresponding drain otherwise the count of outstanding tasks goes out of sync and the process will never stop waiting for tasks when it should be exiting.
It just needs to be calling Signal rather than Broadcast.
Not sure if there was a reason for it to be a broadcast in the first place, but if so then the
outstanding_tasks_count adjustment needs to factor that in properly.