From ebf208ff8a2e0f09842bb580601a709f393a614f Mon Sep 17 00:00:00 2001 From: pavelsavara Date: Tue, 1 Jul 2025 12:54:11 +0200 Subject: [PATCH 1/3] fix multiple calls to OnConnectionDownAsync --- src/Components/Server/src/Circuits/CircuitHost.cs | 5 ++++- src/Components/Server/src/Circuits/CircuitMetrics.cs | 5 ----- .../Server/test/Circuits/CircuitMetricsTest.cs | 9 ++++----- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/Components/Server/src/Circuits/CircuitHost.cs b/src/Components/Server/src/Circuits/CircuitHost.cs index ba684e1984cd..e665a3877231 100644 --- a/src/Components/Server/src/Circuits/CircuitHost.cs +++ b/src/Components/Server/src/Circuits/CircuitHost.cs @@ -209,7 +209,10 @@ await Renderer.Dispatcher.InvokeAsync(async () => try { - await OnConnectionDownAsync(CancellationToken.None); + if (this.Client.Connected) + { + await OnConnectionDownAsync(CancellationToken.None); + } } catch { diff --git a/src/Components/Server/src/Circuits/CircuitMetrics.cs b/src/Components/Server/src/Circuits/CircuitMetrics.cs index 9432c6d18678..2d7c84dea66a 100644 --- a/src/Components/Server/src/Circuits/CircuitMetrics.cs +++ b/src/Components/Server/src/Circuits/CircuitMetrics.cs @@ -70,11 +70,6 @@ public void OnCircuitDown(long startTimestamp, long currentTimestamp) _circuitActiveCounter.Add(-1); } - if (_circuitConnectedCounter.Enabled) - { - _circuitConnectedCounter.Add(-1); - } - if (_circuitDuration.Enabled) { var duration = Stopwatch.GetElapsedTime(startTimestamp, currentTimestamp); diff --git a/src/Components/Server/test/Circuits/CircuitMetricsTest.cs b/src/Components/Server/test/Circuits/CircuitMetricsTest.cs index 8394b01036dc..b1e3ae3c44a2 100644 --- a/src/Components/Server/test/Circuits/CircuitMetricsTest.cs +++ b/src/Components/Server/test/Circuits/CircuitMetricsTest.cs @@ -108,15 +108,11 @@ public async Task OnCircuitDown_UpdatesCountersAndRecordsDuration() // Assert var activeMeasurements = activeCircuitCounter.GetMeasurementSnapshot(); - var connectedMeasurements = connectedCircuitCounter.GetMeasurementSnapshot(); var durationMeasurements = circuitDurationCollector.GetMeasurementSnapshot(); Assert.Single(activeMeasurements); Assert.Equal(-1, activeMeasurements[0].Value); - Assert.Single(connectedMeasurements); - Assert.Equal(-1, connectedMeasurements[0].Value); - Assert.Single(durationMeasurements); Assert.True(durationMeasurements[0].Value > 0); } @@ -162,7 +158,10 @@ public void FullCircuitLifecycle_RecordsAllMetricsCorrectly() // 4. Connection re-established circuitMetrics.OnConnectionUp(); - // 5. Circuit closes + // 5. Connection drops + circuitMetrics.OnConnectionDown(); + + // 6. Circuit closes Thread.Sleep(10); // Add a small delay to ensure a measurable duration var endTime = Stopwatch.GetTimestamp(); circuitMetrics.OnCircuitDown(startTime, endTime); From 89b709aaf1b3ec9c364fce98d40611f71d762f4a Mon Sep 17 00:00:00 2001 From: pavelsavara Date: Mon, 14 Jul 2025 18:56:35 +0200 Subject: [PATCH 2/3] fix other scenarios where OnConnectionDownAsync needs to me more explicit --- .../Server/src/Circuits/CircuitHost.cs | 10 ++++++++ .../Server/src/Circuits/CircuitRegistry.cs | 25 +++++++++++++------ 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/Components/Server/src/Circuits/CircuitHost.cs b/src/Components/Server/src/Circuits/CircuitHost.cs index e665a3877231..535cd9bcf128 100644 --- a/src/Components/Server/src/Circuits/CircuitHost.cs +++ b/src/Components/Server/src/Circuits/CircuitHost.cs @@ -188,6 +188,16 @@ public Task InitializeAsync(ProtectedPrerenderComponentApplicationStore store, A })); } + public bool SetDisconnected() + { + if (Client.Connected) + { + Client.SetDisconnected(); + return true; + } + return false; + } + // We handle errors in DisposeAsync because there's no real value in letting it propagate. // We run user code here (CircuitHandlers) and it's reasonable to expect some might throw, however, // there isn't anything better to do than log when one of these exceptions happens - because the diff --git a/src/Components/Server/src/Circuits/CircuitRegistry.cs b/src/Components/Server/src/Circuits/CircuitRegistry.cs index 6b068e56eae1..92e8a79ad470 100644 --- a/src/Components/Server/src/Circuits/CircuitRegistry.cs +++ b/src/Components/Server/src/Circuits/CircuitRegistry.cs @@ -135,12 +135,12 @@ protected virtual bool DisconnectCore(CircuitHost circuitHost, string connection var result = ConnectedCircuits.TryRemove(circuitId, out circuitHost); Debug.Assert(result, "This operation operates inside of a lock. We expect the previously inspected value to be still here."); - circuitHost.Client.SetDisconnected(); + var previouslyConnected = circuitHost.SetDisconnected(); RegisterDisconnectedCircuit(circuitHost); Log.CircuitMarkedDisconnected(_logger, circuitId); - return true; + return previouslyConnected; } public void RegisterDisconnectedCircuit(CircuitHost circuitHost) @@ -309,6 +309,11 @@ private Task PauseAndDisposeCircuitEntry(DisconnectedCircuitEntry entry) private async Task PauseAndDisposeCircuitHost(CircuitHost circuitHost, bool saveStateToClient) { + if (circuitHost.SetDisconnected()) + { + await circuitHost.Renderer.Dispatcher.InvokeAsync(() => circuitHost.OnConnectionDownAsync(default)); + } + await _circuitPersistenceManager.PauseCircuitAsync(circuitHost, saveStateToClient); circuitHost.UnhandledException -= CircuitHost_UnhandledException; await circuitHost.DisposeAsync(); @@ -376,10 +381,12 @@ private void DisposeTokenSource(DisconnectedCircuitEntry entry) } // We don't expect this to throw. User code only runs inside DisposeAsync and that does its own error handling. - public ValueTask TerminateAsync(CircuitId circuitId) + public async ValueTask TerminateAsync(CircuitId circuitId) { CircuitHost circuitHost; DisconnectedCircuitEntry entry = default; + var circuitHandlerTask = Task.CompletedTask; + lock (CircuitRegistryLock) { if (ConnectedCircuits.TryGetValue(circuitId, out circuitHost) || DisconnectedCircuits.TryGetValue(circuitId.Secret, out entry)) @@ -388,17 +395,21 @@ public ValueTask TerminateAsync(CircuitId circuitId) DisconnectedCircuits.Remove(circuitId.Secret); ConnectedCircuits.TryRemove(circuitId, out _); Log.CircuitDisconnectedPermanently(_logger, circuitHost.CircuitId); - circuitHost.Client.SetDisconnected(); + + if (circuitHost.SetDisconnected()) + { + circuitHandlerTask = circuitHost.Renderer.Dispatcher.InvokeAsync(() => circuitHost.OnConnectionDownAsync(default)); + } } } + await circuitHandlerTask; + if (circuitHost != null) { circuitHost.UnhandledException -= CircuitHost_UnhandledException; - return circuitHost.DisposeAsync(); + await circuitHost.DisposeAsync(); } - - return default; } // We don't need to do anything with the exception here, logging and sending exceptions to the client From 4f774f6bc4172e1efff3da1b39f77fbea996491e Mon Sep 17 00:00:00 2001 From: pavelsavara Date: Thu, 17 Jul 2025 17:24:25 +0200 Subject: [PATCH 3/3] feedback --- .../Server/src/Circuits/CircuitHost.cs | 23 +++++++--------- .../Server/src/Circuits/CircuitRegistry.cs | 26 ++++++------------- 2 files changed, 17 insertions(+), 32 deletions(-) diff --git a/src/Components/Server/src/Circuits/CircuitHost.cs b/src/Components/Server/src/Circuits/CircuitHost.cs index 535cd9bcf128..11dcdb3e5234 100644 --- a/src/Components/Server/src/Circuits/CircuitHost.cs +++ b/src/Components/Server/src/Circuits/CircuitHost.cs @@ -30,6 +30,7 @@ internal partial class CircuitHost : IAsyncDisposable private CircuitHandler[] _circuitHandlers; private bool _initialized; private bool _isFirstUpdate = true; + private bool _onConnectionDownFired; private bool _disposed; private long _startTime; private PersistedCircuitState _persistedCircuitState; @@ -188,16 +189,6 @@ public Task InitializeAsync(ProtectedPrerenderComponentApplicationStore store, A })); } - public bool SetDisconnected() - { - if (Client.Connected) - { - Client.SetDisconnected(); - return true; - } - return false; - } - // We handle errors in DisposeAsync because there's no real value in letting it propagate. // We run user code here (CircuitHandlers) and it's reasonable to expect some might throw, however, // there isn't anything better to do than log when one of these exceptions happens - because the @@ -219,10 +210,7 @@ await Renderer.Dispatcher.InvokeAsync(async () => try { - if (this.Client.Connected) - { - await OnConnectionDownAsync(CancellationToken.None); - } + await OnConnectionDownAsync(CancellationToken.None); } catch { @@ -292,6 +280,7 @@ private async Task OnCircuitOpenedAsync(CancellationToken cancellationToken) public async Task OnConnectionUpAsync(CancellationToken cancellationToken) { + _onConnectionDownFired = false; Log.ConnectionUp(_logger, CircuitId, Client.ConnectionId); _circuitMetrics?.OnConnectionUp(); @@ -322,6 +311,12 @@ public async Task OnConnectionUpAsync(CancellationToken cancellationToken) public async Task OnConnectionDownAsync(CancellationToken cancellationToken) { + if(_onConnectionDownFired) + { + return; + } + + _onConnectionDownFired = true; Log.ConnectionDown(_logger, CircuitId, Client.ConnectionId); _circuitMetrics?.OnConnectionDown(); diff --git a/src/Components/Server/src/Circuits/CircuitRegistry.cs b/src/Components/Server/src/Circuits/CircuitRegistry.cs index 92e8a79ad470..c26ca6d27160 100644 --- a/src/Components/Server/src/Circuits/CircuitRegistry.cs +++ b/src/Components/Server/src/Circuits/CircuitRegistry.cs @@ -135,12 +135,12 @@ protected virtual bool DisconnectCore(CircuitHost circuitHost, string connection var result = ConnectedCircuits.TryRemove(circuitId, out circuitHost); Debug.Assert(result, "This operation operates inside of a lock. We expect the previously inspected value to be still here."); - var previouslyConnected = circuitHost.SetDisconnected(); + circuitHost.Client.SetDisconnected(); RegisterDisconnectedCircuit(circuitHost); Log.CircuitMarkedDisconnected(_logger, circuitId); - return previouslyConnected; + return true; } public void RegisterDisconnectedCircuit(CircuitHost circuitHost) @@ -309,13 +309,9 @@ private Task PauseAndDisposeCircuitEntry(DisconnectedCircuitEntry entry) private async Task PauseAndDisposeCircuitHost(CircuitHost circuitHost, bool saveStateToClient) { - if (circuitHost.SetDisconnected()) - { - await circuitHost.Renderer.Dispatcher.InvokeAsync(() => circuitHost.OnConnectionDownAsync(default)); - } - await _circuitPersistenceManager.PauseCircuitAsync(circuitHost, saveStateToClient); circuitHost.UnhandledException -= CircuitHost_UnhandledException; + circuitHost.Client.SetDisconnected(); await circuitHost.DisposeAsync(); } @@ -381,12 +377,10 @@ private void DisposeTokenSource(DisconnectedCircuitEntry entry) } // We don't expect this to throw. User code only runs inside DisposeAsync and that does its own error handling. - public async ValueTask TerminateAsync(CircuitId circuitId) + public ValueTask TerminateAsync(CircuitId circuitId) { CircuitHost circuitHost; DisconnectedCircuitEntry entry = default; - var circuitHandlerTask = Task.CompletedTask; - lock (CircuitRegistryLock) { if (ConnectedCircuits.TryGetValue(circuitId, out circuitHost) || DisconnectedCircuits.TryGetValue(circuitId.Secret, out entry)) @@ -395,21 +389,17 @@ public async ValueTask TerminateAsync(CircuitId circuitId) DisconnectedCircuits.Remove(circuitId.Secret); ConnectedCircuits.TryRemove(circuitId, out _); Log.CircuitDisconnectedPermanently(_logger, circuitHost.CircuitId); - - if (circuitHost.SetDisconnected()) - { - circuitHandlerTask = circuitHost.Renderer.Dispatcher.InvokeAsync(() => circuitHost.OnConnectionDownAsync(default)); - } + circuitHost.Client.SetDisconnected(); } } - await circuitHandlerTask; - if (circuitHost != null) { circuitHost.UnhandledException -= CircuitHost_UnhandledException; - await circuitHost.DisposeAsync(); + return circuitHost.DisposeAsync(); } + + return default; } // We don't need to do anything with the exception here, logging and sending exceptions to the client