Fix potential deadlock in SyncedEnforcer with Redis Watcher #409
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
A potential deadlock existed in
SyncedEnforcerwhen using Redis watcher (or similar watchers) for policy synchronization between multiple nodes. The deadlock occurred due to circular lock acquisition between the enforcer's write lock and the watcher's mutex.Deadlock Scenario
Consider two threads running concurrently:
Thread A (watcher subscribe thread):
subscribe()methodload_policy())_wl)Thread B (policy modification thread):
add_policy()which acquires the enforcer's write lock (_wl)watcher.update()while holding the lockwatcher.update()attempts to acquire the watcher's mutexResult: Circular dependency → Deadlock
Solution
This PR fixes the deadlock by ensuring watcher notifications happen after releasing the enforcer's locks, eliminating the circular wait condition.
Key Changes
Added
_notify_watcher()helper method: Calls watcher notification methods outside of any lock context, preventing lock ordering issues.Modified all policy modification methods: Updated 26 methods (including
add_policy,remove_policy,add_policies,add_grouping_policy, etc.) to:auto_notify_watcherbefore acquiring the lockEnhanced
set_watcher()method: Now properly sets the watcher's update callback to useself.load_policy, ensuring the callback acquires locks correctly when invoked by the watcher.Added
enable_auto_notify_watcher()method: Exposed this method inSyncedEnforcerfor consistency with the baseEnforcerclass.Example Pattern Applied
Testing
Added comprehensive tests in
test_synced_enforcer_deadlock.py:auto_notify_watchercan be disabledAll 304 tests pass, including both the new tests and all existing tests, confirming no regressions.
Impact
This fix makes
SyncedEnforcersafe to use with Redis watcher in production multi-threaded environments where policy updates and synchronization happen concurrently.Fixes #XXX
Original prompt
Fixes #408
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.