Skip to content

[release/10.2] Backport: Fix deadlock in ServiceEndpointWatcher when disposing change token registration #7259

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.

Sign up for GitHub

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

Closed
joperezr wants to merge 3 commits into from
Closed

[release/10.2] Backport: Fix deadlock in ServiceEndpointWatcher when disposing change token registration #7259

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a466577
Always dispose outside of lock. Use Interlocked.Exchange to clear
ReubenBond Feb 3, 2026
46c63cd
Fix deadlock in ServiceEndpointWatcher when disposing change token re…
ReubenBond Feb 2, 2026
a726db3
Always dispose outside of lock. Use Interlocked.Exchange to clear
ReubenBond Feb 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 30 additions & 12 deletions src/Libraries/Microsoft.Extensions.ServiceDiscovery/ServiceEndpointWatcher.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,13 +135,19 @@ private async Task RefreshAsyncInternal()
CacheStatus newCacheState;
try
{
// Capture the existing change token registration, if any, so we can dispose it.
// Disposing inside the lock can cause a deadlock since Dispose waits for any in-flight callback to complete,
// but the callback may be waiting to acquire _lock.
IDisposable? changeTokenRegistration;
lock (_lock)
{
// Dispose the existing change token registration, if any.
_changeTokenRegistration?.Dispose();
changeTokenRegistration = _changeTokenRegistration;
_changeTokenRegistration = null;
}

// Dispose the existing change token registration, if any.
changeTokenRegistration?.Dispose();

Log.ResolvingEndpoints(_logger, ServiceName);
var builder = new ServiceEndpointBuilder();
foreach (var provider in _providers)
Expand All @@ -153,6 +159,10 @@ private async Task RefreshAsyncInternal()
var endpoints = builder.Build();
newCacheState = CacheStatus.Valid;

// Capture the existing timer, if any, so we can dispose it outside the lock.
// Disposing inside the lock can cause a deadlock.
ITimer? pollingTimer = null;

lock (_lock)
{
// Check if we need to poll for updates or if we can register for change notification callbacks.
Expand All @@ -161,8 +171,7 @@ private async Task RefreshAsyncInternal()
// Initiate a background refresh when the change token fires.
_changeTokenRegistration = endpoints.ChangeToken.RegisterChangeCallback(static state => _ = ((ServiceEndpointWatcher)state!).RefreshAsync(force: false), this);

// Dispose the existing timer, if any, since we are reliant on change tokens for updates.
_pollingTimer?.Dispose();
pollingTimer = _pollingTimer;
_pollingTimer = null;
}
else
Expand All @@ -174,6 +183,8 @@ private async Task RefreshAsyncInternal()
newEndpoints = endpoints;
newCacheState = CacheStatus.Valid;
}

pollingTimer?.Dispose();
}
catch (Exception exception)
{
Expand Down Expand Up @@ -230,16 +241,14 @@ private async Task RefreshAsyncInternal()

private void SchedulePollingTimer()
{
ITimer? timerToDispose = null;
lock (_lock)
{
if (_disposalCancellation.IsCancellationRequested)
{
_pollingTimer?.Dispose();
_pollingTimer = null;
return;
timerToDispose = Interlocked.Exchange(ref _pollingTimer, null);
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
timerToDispose = Interlocked.Exchange(ref _pollingTimer, null);
timerToDispose = _pollingTimer;
_pollingTimer = null;

Copilot uses AI. Check for mistakes.
}

if (_pollingTimer is null)
else if (_pollingTimer is null)
{
_pollingTimer = _timeProvider.CreateTimer(s_pollingAction, this, _options.RefreshPeriod, TimeSpan.Zero);
}
Expand All @@ -248,6 +257,8 @@ private void SchedulePollingTimer()
_pollingTimer.Change(_options.RefreshPeriod, TimeSpan.Zero);
}
}

timerToDispose?.Dispose();
}

/// <inheritdoc/>
Expand All @@ -262,15 +273,22 @@ public async ValueTask DisposeAsync()
_logger.LogError(exception, "Error cancelling disposal cancellation token.");
}

// Capture the existing change token registration and polling timer so we can dispose them.
// Disposing _changeTokenRegistration inside the lock can cause a deadlock since Dispose waits for any in-flight
// callback to complete, but the callback may be waiting to acquire _lock.
IDisposable? changeTokenRegistration;
ITimer? pollingTimer;
lock (_lock)
{
_changeTokenRegistration?.Dispose();
changeTokenRegistration = _changeTokenRegistration;
_changeTokenRegistration = null;

_pollingTimer?.Dispose();
pollingTimer = _pollingTimer;
_pollingTimer = null;
}

changeTokenRegistration?.Dispose();
pollingTimer?.Dispose();

if (_refreshTask is { } task)
{
await task.ConfigureAwait(ConfigureAwaitOptions.SuppressThrowing);
Expand Down
Loading