Deadlock between Watcher and Debounce threads
#187
Comments
|
Seeing how it was working in PR: #189 |
bpasero
added a commit
to bpasero/watcher
that referenced
this issue
Sep 11, 2024
There are two threads involved in `Watcher.cc` and `Debounce.cc` each calling into respective methods of the other and thus each potentially holding onto locks of the other. This can lead to a deadlock in the following scenario: While the `Debounce.cc` thread is processing callbacks in the `Debounce::notify()` method, it holds its own lock. The method loops over callbacks to process in `Watcher.cc` which itself requires a lock in `Watcher.cc`. If an event gets reported while the debouncer is in `Watcher.triggerCallbacks()`, a deadlock is present, because: - assume the event thread is thread A - assume the debouncer thread is thread B - A holds its own lock in `Watcher::notify()` and calls into `Debounce.trigger()` which requires the debouncer lock - B at this time is inside `Debounce::notify()` holding its own lock processing callbacks and about to call into `Watcher.triggerCallbacks()` - A deadlocks waiting for B to release the debouncer lock - B deadlocks waiting for A to release the watcher lock
|
I updated #189 and now have more understanding when exactly this issue happens. The fix is relatively simple. |
bpasero
added a commit
to bpasero/watcher
that referenced
this issue
Sep 12, 2024
There are two threads involved in `Watcher.cc` and `Debounce.cc` each calling into respective methods of the other and thus each potentially holding onto locks of the other. This can lead to a deadlock in the following scenario: While the `Debounce.cc` thread is processing callbacks in the `Debounce::notify()` method, it holds its own lock. The method loops over callbacks to process in `Watcher.cc` which itself requires a lock in `Watcher.cc`. If an event gets reported while the debouncer is in `Watcher.triggerCallbacks()`, a deadlock is present, because: - assume the event thread is thread A - assume the debouncer thread is thread B - A holds its own lock in `Watcher::notify()` and calls into `Debounce.trigger()` which requires the debouncer lock - B at this time is inside `Debounce::notify()` holding its own lock processing callbacks and about to call into `Watcher.triggerCallbacks()` - A deadlocks waiting for B to release the debouncer lock - B deadlocks waiting for A to release the watcher lock
bpasero
added a commit
to bpasero/watcher
that referenced
this issue
Sep 12, 2024
the `@parcel/watcher` in VSCode: - Deadlock between Watcher and Debounce threads (fix parcel-bundler#187) - Allow to compile the native components from sources - Prefer `stat` first before calling `pathExists` Deadlock between Watcher and Debounce threads (fix parcel-bundler#187) There are two threads involved in `Watcher.cc` and `Debounce.cc` each calling into respective methods of the other and thus each potentially holding onto locks of the other. This can lead to a deadlock in the following scenario: While the `Debounce.cc` thread is processing callbacks in the `Debounce::notify()` method, it holds its own lock. The method loops over callbacks to process in `Watcher.cc` which itself requires a lock in `Watcher.cc`. If an event gets reported while the debouncer is in `Watcher.triggerCallbacks()`, a deadlock is present, because: - assume the event thread is thread A - assume the debouncer thread is thread B - A holds its own lock in `Watcher::notify()` and calls into `Debounce.trigger()` which requires the debouncer lock - B at this time is inside `Debounce::notify()` holding its own lock processing callbacks and about to call into `Watcher.triggerCallbacks()` - A deadlocks waiting for B to release the debouncer lock - B deadlocks waiting for A to release the watcher lock
bpasero
added a commit
to bpasero/watcher
that referenced
this issue
Sep 13, 2024
the `@parcel/watcher` in VSCode: - Deadlock between Watcher and Debounce threads (fix parcel-bundler#187) - Allow to compile the native components from sources - macOS: Prefer `stat` first before calling `pathExists` - Windows: disable WER UI in case of crashes Notes on the deadlock: There are two threads involved in `Watcher.cc` and `Debounce.cc` each calling into respective methods of the other and thus each potentially holding onto locks of the other. This can lead to a deadlock in the following scenario: While the `Debounce.cc` thread is processing callbacks in the `Debounce::notify()` method, it holds its own lock. The method loops over callbacks to process in `Watcher.cc` which itself requires a lock in `Watcher.cc`. If an event gets reported while the debouncer is in `Watcher.triggerCallbacks()`, a deadlock is present, because: - assume the event thread is thread A - assume the debouncer thread is thread B - A holds its own lock in `Watcher::notify()` and calls into `Debounce.trigger()` which requires the debouncer lock - B at this time is inside `Debounce::notify()` holding its own lock processing callbacks and about to call into `Watcher.triggerCallbacks()` - A deadlocks waiting for B to release the debouncer lock - B deadlocks waiting for A to release the watcher lock
This was referenced Jun 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Ever since 9b7c657 landed, we have tried to update to newer versions but in VS Code events would stop at random and not recover. Now I had some time to diagnose this further and add logging into the various places where
std::unique_lock<std::mutex>are used because I was suspecting a deadlock between threads.VS Code is probably making it easier to uncover this issue though I would think it could happen otherwise too: in VS Code it is possible for the same folder being watched with different set of
ignorerules, so you can end up withNevents firing fromfseventsfor the same path.In master...bpasero:watcher:ben/fork-2, logging is added through out
Watcher.ccandDebounce.cc, in this pattern:...LOCK 1: try to acquire a lock...LOCK 2: lock acquired...LOCK 3: lock releasedSteps to Reproduce:
Run above with correct paths to any folder (does not have to be
vscode). Open a file from that folder and simply keepCmd+Spressed in the editor to trigger a ton of saves (e.g. using VS Code).=> 🐛 at one point, no more events are printed
Deadlock
With the logging in my fork, I get the following output:
Log Output
You can see that 2 threads are involved here: the debounce thread and the watcher thread. Both have the chance of locking the mutex in
Watcher.ccas well asDebounce.cc. If the one thread was successful in locking the one mutex while the other was successful in locking the other and then they both need the others lock to proceed, the watcher deadlocks.Lets focus on the last log messages, separating threads with newlines:
Threads:
0x16f867000: debouncer0x16f8f3000: watcherThe deadlock is:
Debouncer.cclock successfullyWatcher.cclock successfullyWatcher.ccthat requires the lock inWatcher.ccthat the other thread holdsDebouncer.ccthat the other thread holdsNote: I have seen other variations of this deadlock
@devongovett I would need your advice here and would appreciate if you could have a look. I have a fix in mind that seems to help but I am not sure if its addressing all the issues. Specifically before this line:
watcher/src/Watcher.cc
Line 69 in 99cf43b
call
lk.unlock();so that theWatcher.cclock is free when the debouncer thread needs it.The text was updated successfully, but these errors were encountered: