From a46657759ef6c13effc2f93a3ec806c905b53ac4 Mon Sep 17 00:00:00 2001 From: Reuben Bond Date: Tue, 3 Feb 2026 10:24:19 -0800 Subject: [PATCH 1/3] Always dispose outside of lock. Use Interlocked.Exchange to clear --- .../ServiceEndpointWatcher.cs | 42 +++++++++++++------ 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/src/Libraries/Microsoft.Extensions.ServiceDiscovery/ServiceEndpointWatcher.cs b/src/Libraries/Microsoft.Extensions.ServiceDiscovery/ServiceEndpointWatcher.cs index a94b7b7a3c1..4bf47b04411 100644 --- a/src/Libraries/Microsoft.Extensions.ServiceDiscovery/ServiceEndpointWatcher.cs +++ b/src/Libraries/Microsoft.Extensions.ServiceDiscovery/ServiceEndpointWatcher.cs @@ -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) @@ -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. @@ -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 @@ -174,6 +183,8 @@ private async Task RefreshAsyncInternal() newEndpoints = endpoints; newCacheState = CacheStatus.Valid; } + + pollingTimer?.Dispose(); } catch (Exception exception) { @@ -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); } - - if (_pollingTimer is null) + else if (_pollingTimer is null) { _pollingTimer = _timeProvider.CreateTimer(s_pollingAction, this, _options.RefreshPeriod, TimeSpan.Zero); } @@ -248,6 +257,8 @@ private void SchedulePollingTimer() _pollingTimer.Change(_options.RefreshPeriod, TimeSpan.Zero); } } + + timerToDispose?.Dispose(); } /// @@ -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); From 46c63cd86756512c7a37f39834a210abeccec0cc Mon Sep 17 00:00:00 2001 From: Reuben Bond Date: Mon, 2 Feb 2026 13:04:26 -0800 Subject: [PATCH 2/3] Fix deadlock in ServiceEndpointWatcher when disposing change token registration 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. --- .../ServiceEndpointWatcher.cs | 34 ++++++++----------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/src/Libraries/Microsoft.Extensions.ServiceDiscovery/ServiceEndpointWatcher.cs b/src/Libraries/Microsoft.Extensions.ServiceDiscovery/ServiceEndpointWatcher.cs index 4bf47b04411..b09295f23ab 100644 --- a/src/Libraries/Microsoft.Extensions.ServiceDiscovery/ServiceEndpointWatcher.cs +++ b/src/Libraries/Microsoft.Extensions.ServiceDiscovery/ServiceEndpointWatcher.cs @@ -135,17 +135,17 @@ 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) { + // Capture the existing change token registration, if any, so we can dispose it outside the lock. + // 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. changeTokenRegistration = _changeTokenRegistration; _changeTokenRegistration = null; } - // Dispose the existing change token registration, if any. + // Dispose the existing change token registration, if any, outside the lock to avoid deadlock. changeTokenRegistration?.Dispose(); Log.ResolvingEndpoints(_logger, ServiceName); @@ -159,10 +159,6 @@ 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. @@ -171,7 +167,8 @@ 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); - pollingTimer = _pollingTimer; + // Dispose the existing timer, if any, since we are reliant on change tokens for updates. + _pollingTimer?.Dispose(); _pollingTimer = null; } else @@ -183,8 +180,6 @@ private async Task RefreshAsyncInternal() newEndpoints = endpoints; newCacheState = CacheStatus.Valid; } - - pollingTimer?.Dispose(); } catch (Exception exception) { @@ -241,14 +236,16 @@ private async Task RefreshAsyncInternal() private void SchedulePollingTimer() { - ITimer? timerToDispose = null; lock (_lock) { if (_disposalCancellation.IsCancellationRequested) { - timerToDispose = Interlocked.Exchange(ref _pollingTimer, null); + _pollingTimer?.Dispose(); + _pollingTimer = null; + return; } - else if (_pollingTimer is null) + + if (_pollingTimer is null) { _pollingTimer = _timeProvider.CreateTimer(s_pollingAction, this, _options.RefreshPeriod, TimeSpan.Zero); } @@ -257,8 +254,6 @@ private void SchedulePollingTimer() _pollingTimer.Change(_options.RefreshPeriod, TimeSpan.Zero); } } - - timerToDispose?.Dispose(); } /// @@ -273,15 +268,16 @@ 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) { + // Capture the existing change token registration and polling timer so we can dispose them outside the lock. + // 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. changeTokenRegistration = _changeTokenRegistration; _changeTokenRegistration = null; + pollingTimer = _pollingTimer; _pollingTimer = null; } From a726db34915e20bc76c0ced79babf5fa58ebf003 Mon Sep 17 00:00:00 2001 From: Reuben Bond Date: Tue, 3 Feb 2026 10:24:19 -0800 Subject: [PATCH 3/3] Always dispose outside of lock. Use Interlocked.Exchange to clear --- .../ServiceEndpointWatcher.cs | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/src/Libraries/Microsoft.Extensions.ServiceDiscovery/ServiceEndpointWatcher.cs b/src/Libraries/Microsoft.Extensions.ServiceDiscovery/ServiceEndpointWatcher.cs index b09295f23ab..4bf47b04411 100644 --- a/src/Libraries/Microsoft.Extensions.ServiceDiscovery/ServiceEndpointWatcher.cs +++ b/src/Libraries/Microsoft.Extensions.ServiceDiscovery/ServiceEndpointWatcher.cs @@ -135,17 +135,17 @@ 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) { - // Capture the existing change token registration, if any, so we can dispose it outside the lock. - // 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. changeTokenRegistration = _changeTokenRegistration; _changeTokenRegistration = null; } - // Dispose the existing change token registration, if any, outside the lock to avoid deadlock. + // Dispose the existing change token registration, if any. changeTokenRegistration?.Dispose(); Log.ResolvingEndpoints(_logger, ServiceName); @@ -159,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. @@ -167,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 @@ -180,6 +183,8 @@ private async Task RefreshAsyncInternal() newEndpoints = endpoints; newCacheState = CacheStatus.Valid; } + + pollingTimer?.Dispose(); } catch (Exception exception) { @@ -236,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); } - - if (_pollingTimer is null) + else if (_pollingTimer is null) { _pollingTimer = _timeProvider.CreateTimer(s_pollingAction, this, _options.RefreshPeriod, TimeSpan.Zero); } @@ -254,6 +257,8 @@ private void SchedulePollingTimer() _pollingTimer.Change(_options.RefreshPeriod, TimeSpan.Zero); } } + + timerToDispose?.Dispose(); } /// @@ -268,16 +273,15 @@ 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) { - // Capture the existing change token registration and polling timer so we can dispose them outside the lock. - // 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. changeTokenRegistration = _changeTokenRegistration; _changeTokenRegistration = null; - pollingTimer = _pollingTimer; _pollingTimer = null; }