From c605d6cf6387c61abff55fad79ffe912d02bd31f Mon Sep 17 00:00:00 2001 From: Ajay Bhargav Baaskaran Date: Wed, 14 Aug 2019 12:40:44 -0700 Subject: [PATCH] Don't release SemaphoreSlim when it is canceled (#12818) * Don't release SemaphoreSlim when it is cancelled * fixed tests * Rebased * Updated ref * mark test as flaky --- ...oft.AspNetCore.Components.netcoreapp3.0.cs | 1 + ...ft.AspNetCore.Components.netstandard2.0.cs | 1 + src/Components/Components/src/Dispatcher.cs | 14 ++ .../Components/src/Rendering/Renderer.cs | 21 +-- .../Server/src/Circuits/CircuitHost.cs | 139 +++++++----------- .../test/Circuits/CircuitRegistryTest.cs | 72 --------- .../InteropReliabilityTests.cs | 1 + .../ServerComponentRenderingTest.cs | 2 +- 8 files changed, 75 insertions(+), 176 deletions(-) diff --git a/src/Components/Components/ref/Microsoft.AspNetCore.Components.netcoreapp3.0.cs b/src/Components/Components/ref/Microsoft.AspNetCore.Components.netcoreapp3.0.cs index 8d1cf57be2..e8054887c5 100644 --- a/src/Components/Components/ref/Microsoft.AspNetCore.Components.netcoreapp3.0.cs +++ b/src/Components/Components/ref/Microsoft.AspNetCore.Components.netcoreapp3.0.cs @@ -97,6 +97,7 @@ namespace Microsoft.AspNetCore.Components public abstract partial class Dispatcher { protected Dispatcher() { } + public void AssertAccess() { } public abstract bool CheckAccess(); public static Microsoft.AspNetCore.Components.Dispatcher CreateDefault() { throw null; } public abstract System.Threading.Tasks.Task InvokeAsync(System.Action workItem); diff --git a/src/Components/Components/ref/Microsoft.AspNetCore.Components.netstandard2.0.cs b/src/Components/Components/ref/Microsoft.AspNetCore.Components.netstandard2.0.cs index 8d1cf57be2..e8054887c5 100644 --- a/src/Components/Components/ref/Microsoft.AspNetCore.Components.netstandard2.0.cs +++ b/src/Components/Components/ref/Microsoft.AspNetCore.Components.netstandard2.0.cs @@ -97,6 +97,7 @@ namespace Microsoft.AspNetCore.Components public abstract partial class Dispatcher { protected Dispatcher() { } + public void AssertAccess() { } public abstract bool CheckAccess(); public static Microsoft.AspNetCore.Components.Dispatcher CreateDefault() { throw null; } public abstract System.Threading.Tasks.Task InvokeAsync(System.Action workItem); diff --git a/src/Components/Components/src/Dispatcher.cs b/src/Components/Components/src/Dispatcher.cs index c08275af7d..908daba4cc 100644 --- a/src/Components/Components/src/Dispatcher.cs +++ b/src/Components/Components/src/Dispatcher.cs @@ -23,6 +23,20 @@ namespace Microsoft.AspNetCore.Components /// internal event UnhandledExceptionEventHandler UnhandledException; + /// + /// Validates that the currently executing code is running inside the dispatcher. + /// + public void AssertAccess() + { + if (!CheckAccess()) + { + throw new InvalidOperationException( + "The current thread is not associated with the Dispatcher. " + + "Use InvokeAsync() to switch execution to the Dispatcher when " + + "triggering rendering or component state."); + } + } + /// /// Returns a value that determines whether using the dispatcher to invoke a work item is required /// from the current context. diff --git a/src/Components/Components/src/Rendering/Renderer.cs b/src/Components/Components/src/Rendering/Renderer.cs index 208630f141..3acaccd2ed 100644 --- a/src/Components/Components/src/Rendering/Renderer.cs +++ b/src/Components/Components/src/Rendering/Renderer.cs @@ -209,7 +209,7 @@ namespace Microsoft.AspNetCore.Components.Rendering /// public virtual Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo fieldInfo, EventArgs eventArgs) { - EnsureSynchronizationContext(); + Dispatcher.AssertAccess(); if (!_eventBindings.TryGetValue(eventHandlerId, out var callback)) { @@ -337,7 +337,7 @@ namespace Microsoft.AspNetCore.Components.Rendering /// A that will supply the updated UI contents. internal void AddToRenderQueue(int componentId, RenderFragment renderFragment) { - EnsureSynchronizationContext(); + Dispatcher.AssertAccess(); var componentState = GetOptionalComponentState(componentId); if (componentState == null) @@ -374,21 +374,6 @@ namespace Microsoft.AspNetCore.Components.Rendering return eventHandlerId; } - private void EnsureSynchronizationContext() - { - // Render operations are not thread-safe, so they need to be serialized by the dispatcher. - // Plus, any other logic that mutates state accessed during rendering also - // needs not to run concurrently with rendering so should be dispatched to - // the renderer's sync context. - if (!Dispatcher.CheckAccess()) - { - throw new InvalidOperationException( - "The current thread is not associated with the Dispatcher. " + - "Use Invoke() or InvokeAsync() to switch execution to the Dispatcher when " + - "triggering rendering or modifying any state accessed during rendering."); - } - } - private ComponentState GetRequiredComponentState(int componentId) => _componentStateById.TryGetValue(componentId, out var componentState) ? componentState @@ -414,7 +399,7 @@ namespace Microsoft.AspNetCore.Components.Rendering private void ProcessRenderQueue() { - EnsureSynchronizationContext(); + Dispatcher.AssertAccess(); if (_isBatchInProgress) { diff --git a/src/Components/Server/src/Circuits/CircuitHost.cs b/src/Components/Server/src/Circuits/CircuitHost.cs index 97a2e3f3c7..8010f1174d 100644 --- a/src/Components/Server/src/Circuits/CircuitHost.cs +++ b/src/Components/Server/src/Circuits/CircuitHost.cs @@ -18,7 +18,6 @@ namespace Microsoft.AspNetCore.Components.Server.Circuits { internal class CircuitHost : IAsyncDisposable { - private readonly SemaphoreSlim HandlerLock = new SemaphoreSlim(1); private readonly IServiceScope _scope; private readonly CircuitOptions _options; private readonly CircuitHandler[] _circuitHandlers; @@ -187,35 +186,28 @@ namespace Microsoft.AspNetCore.Components.Server.Circuits { Log.CircuitOpened(_logger, Circuit.Id); - await HandlerLock.WaitAsync(cancellationToken); + Renderer.Dispatcher.AssertAccess(); - try + List exceptions = null; + + for (var i = 0; i < _circuitHandlers.Length; i++) { - List exceptions = null; - - for (var i = 0; i < _circuitHandlers.Length; i++) + var circuitHandler = _circuitHandlers[i]; + try { - var circuitHandler = _circuitHandlers[i]; - try - { - await circuitHandler.OnCircuitOpenedAsync(Circuit, cancellationToken); - } - catch (Exception ex) - { - Log.CircuitHandlerFailed(_logger, circuitHandler, nameof(CircuitHandler.OnCircuitOpenedAsync), ex); - exceptions ??= new List(); - exceptions.Add(ex); - } + await circuitHandler.OnCircuitOpenedAsync(Circuit, cancellationToken); } - - if (exceptions != null) + catch (Exception ex) { - throw new AggregateException("Encountered exceptions while executing circuit handlers.", exceptions); + Log.CircuitHandlerFailed(_logger, circuitHandler, nameof(CircuitHandler.OnCircuitOpenedAsync), ex); + exceptions ??= new List(); + exceptions.Add(ex); } } - finally + + if (exceptions != null) { - HandlerLock.Release(); + throw new AggregateException("Encountered exceptions while executing circuit handlers.", exceptions); } } @@ -223,35 +215,28 @@ namespace Microsoft.AspNetCore.Components.Server.Circuits { Log.ConnectionUp(_logger, Circuit.Id, Client.ConnectionId); - await HandlerLock.WaitAsync(cancellationToken); + Renderer.Dispatcher.AssertAccess(); + + List exceptions = null; - try + for (var i = 0; i < _circuitHandlers.Length; i++) { - List exceptions = null; - - for (var i = 0; i < _circuitHandlers.Length; i++) + var circuitHandler = _circuitHandlers[i]; + try { - var circuitHandler = _circuitHandlers[i]; - try - { - await circuitHandler.OnConnectionUpAsync(Circuit, cancellationToken); - } - catch (Exception ex) - { - Log.CircuitHandlerFailed(_logger, circuitHandler, nameof(CircuitHandler.OnConnectionUpAsync), ex); - exceptions ??= new List(); - exceptions.Add(ex); - } + await circuitHandler.OnConnectionUpAsync(Circuit, cancellationToken); } - - if (exceptions != null) + catch (Exception ex) { - throw new AggregateException("Encountered exceptions while executing circuit handlers.", exceptions); + Log.CircuitHandlerFailed(_logger, circuitHandler, nameof(CircuitHandler.OnConnectionUpAsync), ex); + exceptions ??= new List(); + exceptions.Add(ex); } } - finally + + if (exceptions != null) { - HandlerLock.Release(); + throw new AggregateException("Encountered exceptions while executing circuit handlers.", exceptions); } } @@ -259,35 +244,28 @@ namespace Microsoft.AspNetCore.Components.Server.Circuits { Log.ConnectionDown(_logger, Circuit.Id, Client.ConnectionId); - await HandlerLock.WaitAsync(cancellationToken); + Renderer.Dispatcher.AssertAccess(); + + List exceptions = null; - try + for (var i = 0; i < _circuitHandlers.Length; i++) { - List exceptions = null; - - for (var i = 0; i < _circuitHandlers.Length; i++) + var circuitHandler = _circuitHandlers[i]; + try { - var circuitHandler = _circuitHandlers[i]; - try - { - await circuitHandler.OnConnectionDownAsync(Circuit, cancellationToken); - } - catch (Exception ex) - { - Log.CircuitHandlerFailed(_logger, circuitHandler, nameof(CircuitHandler.OnConnectionDownAsync), ex); - exceptions ??= new List(); - exceptions.Add(ex); - } + await circuitHandler.OnConnectionDownAsync(Circuit, cancellationToken); } - - if (exceptions != null) + catch (Exception ex) { - throw new AggregateException("Encountered exceptions while executing circuit handlers.", exceptions); + Log.CircuitHandlerFailed(_logger, circuitHandler, nameof(CircuitHandler.OnConnectionDownAsync), ex); + exceptions ??= new List(); + exceptions.Add(ex); } } - finally + + if (exceptions != null) { - HandlerLock.Release(); + throw new AggregateException("Encountered exceptions while executing circuit handlers.", exceptions); } } @@ -295,35 +273,26 @@ namespace Microsoft.AspNetCore.Components.Server.Circuits { Log.CircuitClosed(_logger, Circuit.Id); - await HandlerLock.WaitAsync(cancellationToken); + List exceptions = null; - try + for (var i = 0; i < _circuitHandlers.Length; i++) { - List exceptions = null; - - for (var i = 0; i < _circuitHandlers.Length; i++) + var circuitHandler = _circuitHandlers[i]; + try { - var circuitHandler = _circuitHandlers[i]; - try - { - await circuitHandler.OnCircuitClosedAsync(Circuit, cancellationToken); - } - catch (Exception ex) - { - Log.CircuitHandlerFailed(_logger, circuitHandler, nameof(CircuitHandler.OnCircuitClosedAsync), ex); - exceptions ??= new List(); - exceptions.Add(ex); - } + await circuitHandler.OnCircuitClosedAsync(Circuit, cancellationToken); } - - if (exceptions != null) + catch (Exception ex) { - throw new AggregateException("Encountered exceptions while executing circuit handlers.", exceptions); + Log.CircuitHandlerFailed(_logger, circuitHandler, nameof(CircuitHandler.OnCircuitClosedAsync), ex); + exceptions ??= new List(); + exceptions.Add(ex); } } - finally + + if (exceptions != null) { - HandlerLock.Release(); + throw new AggregateException("Encountered exceptions while executing circuit handlers.", exceptions); } } diff --git a/src/Components/Server/test/Circuits/CircuitRegistryTest.cs b/src/Components/Server/test/Circuits/CircuitRegistryTest.cs index 6efdf212a0..f57c27dbfa 100644 --- a/src/Components/Server/test/Circuits/CircuitRegistryTest.cs +++ b/src/Components/Server/test/Circuits/CircuitRegistryTest.cs @@ -270,46 +270,6 @@ namespace Microsoft.AspNetCore.Components.Server.Circuits Assert.False(registry.DisconnectedCircuits.TryGetValue(circuitHost.CircuitId, out _)); } - [Fact] - public async Task Connect_WhileDisconnectIsInProgress_SeriallyExecutesCircuitHandlers() - { - // Arrange - var circuitIdFactory = TestCircuitIdFactory.CreateTestFactory(); - - var registry = new TestCircuitRegistry(circuitIdFactory); - registry.BeforeDisconnect = new ManualResetEventSlim(); - // This verifies that connection up \ down events on a circuit handler are always invoked serially. - var circuitHandler = new SerialCircuitHandler(); - var tcs = new TaskCompletionSource(); - - var circuitHost = TestCircuitHost.Create(circuitIdFactory.CreateCircuitId(), handlers: new[] { circuitHandler }); - registry.Register(circuitHost); - var client = Mock.Of(); - var newId = "new-connection"; - - // Act - var disconnect = Task.Run(() => - { - var task = registry.DisconnectAsync(circuitHost, circuitHost.Client.ConnectionId); - tcs.SetResult(0); - return task; - }); - var connect = Task.Run(async () => - { - registry.BeforeDisconnect.Set(); - await tcs.Task; - await registry.ConnectAsync(circuitHost.CircuitId, client, newId, default); - }); - await Task.WhenAll(disconnect, connect); - - // Assert - Assert.Single(registry.ConnectedCircuits.Values); - Assert.False(registry.DisconnectedCircuits.TryGetValue(circuitHost.CircuitId, out _)); - - Assert.True(circuitHandler.OnConnectionDownExecuted, "OnConnectionDownAsync should have been executed."); - Assert.True(circuitHandler.OnConnectionUpExecuted, "OnConnectionUpAsync should have been executed."); - } - [Fact] public async Task DisconnectWhenAConnectIsInProgress() { @@ -444,37 +404,5 @@ namespace Microsoft.AspNetCore.Components.Server.Circuits NullLogger.Instance, factory ?? TestCircuitIdFactory.CreateTestFactory()); } - - private class SerialCircuitHandler : CircuitHandler - { - private readonly SemaphoreSlim _sempahore = new SemaphoreSlim(1); - - public bool OnConnectionUpExecuted { get; private set; } - public bool OnConnectionDownExecuted { get; private set; } - - public override async Task OnConnectionUpAsync(Circuit circuit, CancellationToken cancellationToken) - { - Assert.True(await _sempahore.WaitAsync(0), "This should be serialized and consequently without contention"); - await Task.Delay(10); - - Assert.False(OnConnectionUpExecuted); - Assert.True(OnConnectionDownExecuted); - OnConnectionUpExecuted = true; - - _sempahore.Release(); - } - - public override async Task OnConnectionDownAsync(Circuit circuit, CancellationToken cancellationToken) - { - Assert.True(await _sempahore.WaitAsync(0), "This should be serialized and consequently without contention"); - await Task.Delay(10); - - Assert.False(OnConnectionUpExecuted); - Assert.False(OnConnectionDownExecuted); - OnConnectionDownExecuted = true; - - _sempahore.Release(); - } - } } } diff --git a/src/Components/test/E2ETest/ServerExecutionTests/InteropReliabilityTests.cs b/src/Components/test/E2ETest/ServerExecutionTests/InteropReliabilityTests.cs index b1d275bc3a..0e9b6568ef 100644 --- a/src/Components/test/E2ETest/ServerExecutionTests/InteropReliabilityTests.cs +++ b/src/Components/test/E2ETest/ServerExecutionTests/InteropReliabilityTests.cs @@ -264,6 +264,7 @@ namespace Microsoft.AspNetCore.Components.E2ETest.ServerExecutionTests } [Fact] + [Flaky("https://github.com/aspnet/AspNetCore/issues/13086", FlakyOn.AzP.Windows)] public async Task ContinuesWorkingAfterInvalidAsyncReturnCallback() { // Arrange diff --git a/src/Components/test/E2ETest/ServerExecutionTests/ServerComponentRenderingTest.cs b/src/Components/test/E2ETest/ServerExecutionTests/ServerComponentRenderingTest.cs index abf1ef5a15..6315ee1b51 100644 --- a/src/Components/test/E2ETest/ServerExecutionTests/ServerComponentRenderingTest.cs +++ b/src/Components/test/E2ETest/ServerExecutionTests/ServerComponentRenderingTest.cs @@ -33,7 +33,7 @@ namespace Microsoft.AspNetCore.Components.E2ETest.ServerExecutionTests appElement.FindElement(By.Id("run-without-dispatch")).Click(); Browser.Contains( - $"{typeof(InvalidOperationException).FullName}: The current thread is not associated with the Dispatcher. Use Invoke() or InvokeAsync() to switch execution to the Dispatcher when triggering rendering or modifying any state accessed during rendering.", + $"{typeof(InvalidOperationException).FullName}: The current thread is not associated with the Dispatcher. Use InvokeAsync() to switch execution to the Dispatcher when triggering rendering or component state.", () => result.Text); } }