[release/10.2] Backport: Fix deadlock in ServiceEndpointWatcher when disposing change token registration#7259
[release/10.2] Backport: Fix deadlock in ServiceEndpointWatcher when disposing change token registration#7259joperezr wants to merge 3 commits intorelease/10.2from
Conversation
…gistration Move _changeTokenRegistration.Dispose() outside the lock to avoid deadlock. CancellationTokenRegistration.Dispose() blocks waiting for any in-flight callback to complete, but the callback (RefreshAsync) tries to acquire the same lock, causing a deadlock. The fix captures the registration reference while holding the lock, then disposes it after releasing the lock. Applied to both RefreshAsyncInternal and DisposeAsync methods.
|
Closing as dupe for #7258 |
There was a problem hiding this comment.
Pull request overview
This is a backport to the release/10.2 branch that fixes a deadlock issue in ServiceEndpointWatcher when disposing change token registrations and timers. The deadlock occurred when Dispose() was called inside a lock while callbacks were waiting to acquire the same lock, causing Dispose() to wait for the callback to complete, creating a circular wait.
Changes:
- Modified disposal pattern to capture disposable resources inside locks and dispose them outside locks
- Applied this pattern consistently to
RefreshAsyncInternal(),SchedulePollingTimer(), andDisposeAsync()methods - Added explanatory comments documenting the deadlock scenario and why this pattern is necessary
| _pollingTimer?.Dispose(); | ||
| _pollingTimer = null; | ||
| return; | ||
| timerToDispose = Interlocked.Exchange(ref _pollingTimer, null); |
There was a problem hiding this comment.
Using Interlocked.Exchange inside a lock is unnecessary and inconsistent with the pattern used in RefreshAsyncInternal (lines 144, 174) and DisposeAsync (lines 283-286), where simple assignment is used inside the lock. Since this code is already protected by the lock, a simple assignment would be more appropriate and consistent:
timerToDispose = _pollingTimer;
_pollingTimer = null;
The Interlocked.Exchange provides thread-safe atomic operations, but that's already guaranteed by the lock.
| timerToDispose = Interlocked.Exchange(ref _pollingTimer, null); | |
| timerToDispose = _pollingTimer; | |
| _pollingTimer = null; |
2 participants
cc: @ReubenBond
Microsoft Reviewers: Open in CodeFlow