-
Notifications
You must be signed in to change notification settings - Fork 228
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PollWatcher may deadlock when user try change watch path in event_handler #410
Comments
Is this fixed by #409 ? |
This bug not fixed yet. #409 only include refactoring, almost all behavior are the same. Recently I have no time to deal this one. If you want to fix it just take it :) |
On which system did you get this to deadlock ? |
I'm tested within debian bullseye. But pretty sure this bug affect all platform. The deadlock will happen when user try to change ( |
- Simple merging logic for `Config` - Load config from system-wide locations (fixes #30) - Dynamically pick up changes from newly created config files (fixes #29) - Use `directories-rs` to find user level config directory path A new `ConfigWatcher` is introduced, which opportunistically watches the closest existing parent of target paths and rewatches dynamically accordingly. This way we can get notifications for new file creations in addition to modifications. `ConfigWatcher` also does debounce (100ms by default) to only trigger one config reload for multiple events in a short time period. The core idea of the watcher is as follow: Given a watching target path `/base/sub/config.toml`, a) readd watches if any segment in the path is created/removed; b) reload config only if the event is about the full target path. The actual implementation extends this idea to handle multiple targets. Please refer to `config_watcher.rs` for details and extensive test cases. A few design points: - Notify event handling is done in a separate thread, rather than in notify's internal event handler thread, because the code is more readable and calling unwatch/watch in that thread may lead to deadlock in some cases. (notify-rs/notify#410, notify-rs/notify#463) - For simplicity, `ConfigWatcher` doesn't provide infomation about which exact config file was changed, assuming a reload would need to read all config files anyway.
- Simple merging logic for `Config` - Load config from system-wide locations (fixes #30) - Dynamically pick up changes from newly created config files (fixes #29) - Use `directories-rs` to find user level config directory path A new `ConfigWatcher` is introduced, which opportunistically watches the closest existing parent of target paths and rewatches dynamically accordingly. This way we can get notifications for new file creations in addition to modifications. `ConfigWatcher` also does debounce (100ms by default) to only trigger one config reload for multiple events in a short time period. The core idea of the watcher is as follow: Given a watching target path `/base/sub/config.toml`, a) readd watches if any segment in the path is created/removed; b) reload config only if the event is about the full target path. The actual implementation extends this idea to handle multiple targets. Please refer to `config_watcher.rs` for details and extensive test cases. A few design points: - Notify event handling is done in a separate thread, rather than in notify's internal event handler thread, because the code is more readable and calling unwatch/watch in that thread may lead to deadlock in some cases. (notify-rs/notify#410, notify-rs/notify#463) - For simplicity, `ConfigWatcher` doesn't provide infomation about which exact config file was changed, assuming a reload would need to read all config files anyway.
- Simple merging logic for `Config` - Load config from system-wide locations (fixes #30) - Dynamically pick up changes from newly created config files (fixes #29) - Use `directories-rs` to find user level config directory path A new `ConfigWatcher` is introduced, which opportunistically watches the closest existing parent of target paths and rewatches dynamically accordingly. This way we can get notifications for new file creations in addition to modifications. `ConfigWatcher` also does debounce (100ms by default) to only trigger one config reload for multiple events in a short time period. The core idea of the watcher is as follow: Given a watching target path `/base/sub/config.toml`, a) readd watches if any segment in the path is created/removed; b) reload config only if the event is about the full target path. The actual implementation extends this idea to handle multiple targets. Please refer to `config_watcher.rs` for details and extensive test cases. A few design points: - Notify event handling is done in a separate thread, rather than in notify's internal event handler thread, because the code is more readable and calling unwatch/watch in that thread may lead to deadlock in some cases. (notify-rs/notify#410, notify-rs/notify#463) - For simplicity, `ConfigWatcher` doesn't provide infomation about which exact config file was changed, assuming a reload would need to read all config files anyway.
- Simple merging logic for `Config` - Load config from system-wide locations (fixes #30) - Dynamically pick up changes from newly created config files (fixes #29) - Use `directories-rs` to find user level config directory path A new `ConfigWatcher` is introduced, which opportunistically watches the closest existing parent of target paths and rewatches dynamically accordingly. This way we can get notifications for new file creations in addition to modifications. `ConfigWatcher` also does debounce (100ms by default) to only trigger one config reload for multiple events in a short time period. The core idea of the watcher is as follow: Given a watching target path `/base/sub/config.toml`, a) readd watches if any segment in the path is created/removed; b) reload config only if the event is about the full target path. The actual implementation extends this idea to handle multiple targets. Please refer to `config_watcher.rs` for details and extensive test cases. A few design points: - Notify event handling is done in a separate thread, rather than in notify's internal event handler thread, because the code is more readable and calling unwatch/watch in that thread may lead to deadlock in some cases. (notify-rs/notify#410, notify-rs/notify#463) - For simplicity, `ConfigWatcher` doesn't provide infomation about which exact config file was changed, assuming a reload would need to read all config files anyway.
- Simple merging logic for `Config` - Load config from system-wide locations (fixes #30) - Dynamically pick up changes from newly created config files (fixes #29) - Use `directories-rs` to find user level config directory path A new `ConfigWatcher` is introduced, which opportunistically watches the closest existing parent of target paths and rewatches dynamically accordingly. This way we can get notifications for new file creations in addition to modifications. `ConfigWatcher` also does debounce (100ms by default) to only trigger one config reload for multiple events in a short time period. The core idea of the watcher is as follow: Given a watching target path `/base/sub/config.toml`, a) readd watches if any segment in the path is created/removed; b) reload config only if the event is about the full target path. The actual implementation extends this idea to handle multiple targets. Please refer to `config_watcher.rs` for details and extensive test cases. A few design points: - Notify event handling is done in a separate thread, rather than in notify's internal event handler thread, because the code is more readable and calling unwatch/watch in that thread may lead to deadlock in some cases. (notify-rs/notify#410, notify-rs/notify#463) - For simplicity, `ConfigWatcher` doesn't provide infomation about which exact config file was changed, assuming a reload would need to read all config files anyway.
- Simple merging logic for `Config` - Load config from system-wide locations (fixes #30) - Dynamically pick up changes from newly created config files (fixes #29) - Use `directories-rs` to find user level config directory path A new `ConfigWatcher` is introduced, which opportunistically watches the closest existing parent of target paths and rewatches dynamically accordingly. This way we can get notifications for new file creations in addition to modifications. `ConfigWatcher` also does debounce (100ms by default) to only trigger one config reload for multiple events in a short time period. The core idea of the watcher is as follow: Given a watching target path `/base/sub/config.toml`, a) readd watches if any segment in the path is created/removed; b) reload config only if the event is about the full target path. The actual implementation extends this idea to handle multiple targets. Please refer to `config_watcher.rs` for details and extensive test cases. A few design points: - Notify event handling is done in a separate thread, rather than in notify's internal event handler thread, because the code is more readable and calling unwatch/watch in that thread may lead to deadlock in some cases. (notify-rs/notify#410, notify-rs/notify#463) - For simplicity, `ConfigWatcher` doesn't provide infomation about which exact config file was changed, assuming a reload would need to read all config files anyway.
* Gracefully handle missing XDG_RUNTIME_DIR env var in tests * Configuration loading overhaul - Simple merging logic for `Config` - Load config from system-wide locations (fixes #30) - Dynamically pick up changes from newly created config files (fixes #29) - Use `directories-rs` to find user level config directory path A new `ConfigWatcher` is introduced, which opportunistically watches the closest existing parent of target paths and rewatches dynamically accordingly. This way we can get notifications for new file creations in addition to modifications. `ConfigWatcher` also does debounce (100ms by default) to only trigger one config reload for multiple events in a short time period. The core idea of the watcher is as follow: Given a watching target path `/base/sub/config.toml`, a) readd watches if any segment in the path is created/removed; b) reload config only if the event is about the full target path. The actual implementation extends this idea to handle multiple targets. Please refer to `config_watcher.rs` for details and extensive test cases. A few design points: - Notify event handling is done in a separate thread, rather than in notify's internal event handler thread, because the code is more readable and calling unwatch/watch in that thread may lead to deadlock in some cases. (notify-rs/notify#410, notify-rs/notify#463) - For simplicity, `ConfigWatcher` doesn't provide infomation about which exact config file was changed, assuming a reload would need to read all config files anyway.
System details
I believe this one affect all environment.
What you did
What you expected
No deadlock.
What happened
Deadlocked.
Investigate
When user try to change current watching paths, the watcher will take the lock from internal data, See:
notify/src/poll.rs
Line 314 in 0245625
notify/src/poll.rs
Line 377 in 0245625
But when event_handler be called, this lock are already hold by internal loop, so we call
lock()
twice at single thread, deadlock.I may fix it (wait until #409 merged) by add a dedicated thread as event consumer, but after this fix, we will have two thread for each
PollWatcher
, just little heavy :(The text was updated successfully, but these errors were encountered: