Skip to content

Commit c605d6c

Browse files
authored
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
1 parent 1f7d59d commit c605d6c

File tree

8 files changed

+75
-176
lines changed

8 files changed

+75
-176
lines changed

src/Components/Components/ref/Microsoft.AspNetCore.Components.netcoreapp3.0.cs

+1
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ protected void StateHasChanged() { }
9797
public abstract partial class Dispatcher
9898
{
9999
protected Dispatcher() { }
100+
public void AssertAccess() { }
100101
public abstract bool CheckAccess();
101102
public static Microsoft.AspNetCore.Components.Dispatcher CreateDefault() { throw null; }
102103
public abstract System.Threading.Tasks.Task InvokeAsync(System.Action workItem);

src/Components/Components/ref/Microsoft.AspNetCore.Components.netstandard2.0.cs

+1
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ protected void StateHasChanged() { }
9797
public abstract partial class Dispatcher
9898
{
9999
protected Dispatcher() { }
100+
public void AssertAccess() { }
100101
public abstract bool CheckAccess();
101102
public static Microsoft.AspNetCore.Components.Dispatcher CreateDefault() { throw null; }
102103
public abstract System.Threading.Tasks.Task InvokeAsync(System.Action workItem);

src/Components/Components/src/Dispatcher.cs

+14
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,20 @@ public abstract class Dispatcher
2323
/// </summary>
2424
internal event UnhandledExceptionEventHandler UnhandledException;
2525

26+
/// <summary>
27+
/// Validates that the currently executing code is running inside the dispatcher.
28+
/// </summary>
29+
public void AssertAccess()
30+
{
31+
if (!CheckAccess())
32+
{
33+
throw new InvalidOperationException(
34+
"The current thread is not associated with the Dispatcher. " +
35+
"Use InvokeAsync() to switch execution to the Dispatcher when " +
36+
"triggering rendering or component state.");
37+
}
38+
}
39+
2640
/// <summary>
2741
/// Returns a value that determines whether using the dispatcher to invoke a work item is required
2842
/// from the current context.

src/Components/Components/src/Rendering/Renderer.cs

+3-18
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,7 @@ private ComponentState AttachAndInitComponent(IComponent component, int parentCo
209209
/// </returns>
210210
public virtual Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo fieldInfo, EventArgs eventArgs)
211211
{
212-
EnsureSynchronizationContext();
212+
Dispatcher.AssertAccess();
213213

214214
if (!_eventBindings.TryGetValue(eventHandlerId, out var callback))
215215
{
@@ -337,7 +337,7 @@ internal void AssignEventHandlerId(ref RenderTreeFrame frame)
337337
/// <param name="renderFragment">A <see cref="RenderFragment"/> that will supply the updated UI contents.</param>
338338
internal void AddToRenderQueue(int componentId, RenderFragment renderFragment)
339339
{
340-
EnsureSynchronizationContext();
340+
Dispatcher.AssertAccess();
341341

342342
var componentState = GetOptionalComponentState(componentId);
343343
if (componentState == null)
@@ -374,21 +374,6 @@ private ulong FindLatestEventHandlerIdInChain(ulong eventHandlerId)
374374
return eventHandlerId;
375375
}
376376

377-
private void EnsureSynchronizationContext()
378-
{
379-
// Render operations are not thread-safe, so they need to be serialized by the dispatcher.
380-
// Plus, any other logic that mutates state accessed during rendering also
381-
// needs not to run concurrently with rendering so should be dispatched to
382-
// the renderer's sync context.
383-
if (!Dispatcher.CheckAccess())
384-
{
385-
throw new InvalidOperationException(
386-
"The current thread is not associated with the Dispatcher. " +
387-
"Use Invoke() or InvokeAsync() to switch execution to the Dispatcher when " +
388-
"triggering rendering or modifying any state accessed during rendering.");
389-
}
390-
}
391-
392377
private ComponentState GetRequiredComponentState(int componentId)
393378
=> _componentStateById.TryGetValue(componentId, out var componentState)
394379
? componentState
@@ -414,7 +399,7 @@ protected virtual void ProcessPendingRender()
414399

415400
private void ProcessRenderQueue()
416401
{
417-
EnsureSynchronizationContext();
402+
Dispatcher.AssertAccess();
418403

419404
if (_isBatchInProgress)
420405
{

src/Components/Server/src/Circuits/CircuitHost.cs

+54-85
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ namespace Microsoft.AspNetCore.Components.Server.Circuits
1818
{
1919
internal class CircuitHost : IAsyncDisposable
2020
{
21-
private readonly SemaphoreSlim HandlerLock = new SemaphoreSlim(1);
2221
private readonly IServiceScope _scope;
2322
private readonly CircuitOptions _options;
2423
private readonly CircuitHandler[] _circuitHandlers;
@@ -187,143 +186,113 @@ private async Task OnCircuitOpenedAsync(CancellationToken cancellationToken)
187186
{
188187
Log.CircuitOpened(_logger, Circuit.Id);
189188

190-
await HandlerLock.WaitAsync(cancellationToken);
189+
Renderer.Dispatcher.AssertAccess();
191190

192-
try
193-
{
194-
List<Exception> exceptions = null;
191+
List<Exception> exceptions = null;
195192

196-
for (var i = 0; i < _circuitHandlers.Length; i++)
193+
for (var i = 0; i < _circuitHandlers.Length; i++)
194+
{
195+
var circuitHandler = _circuitHandlers[i];
196+
try
197197
{
198-
var circuitHandler = _circuitHandlers[i];
199-
try
200-
{
201-
await circuitHandler.OnCircuitOpenedAsync(Circuit, cancellationToken);
202-
}
203-
catch (Exception ex)
204-
{
205-
Log.CircuitHandlerFailed(_logger, circuitHandler, nameof(CircuitHandler.OnCircuitOpenedAsync), ex);
206-
exceptions ??= new List<Exception>();
207-
exceptions.Add(ex);
208-
}
198+
await circuitHandler.OnCircuitOpenedAsync(Circuit, cancellationToken);
209199
}
210-
211-
if (exceptions != null)
200+
catch (Exception ex)
212201
{
213-
throw new AggregateException("Encountered exceptions while executing circuit handlers.", exceptions);
202+
Log.CircuitHandlerFailed(_logger, circuitHandler, nameof(CircuitHandler.OnCircuitOpenedAsync), ex);
203+
exceptions ??= new List<Exception>();
204+
exceptions.Add(ex);
214205
}
215206
}
216-
finally
207+
208+
if (exceptions != null)
217209
{
218-
HandlerLock.Release();
210+
throw new AggregateException("Encountered exceptions while executing circuit handlers.", exceptions);
219211
}
220212
}
221213

222214
public async Task OnConnectionUpAsync(CancellationToken cancellationToken)
223215
{
224216
Log.ConnectionUp(_logger, Circuit.Id, Client.ConnectionId);
225217

226-
await HandlerLock.WaitAsync(cancellationToken);
218+
Renderer.Dispatcher.AssertAccess();
219+
220+
List<Exception> exceptions = null;
227221

228-
try
222+
for (var i = 0; i < _circuitHandlers.Length; i++)
229223
{
230-
List<Exception> exceptions = null;
231-
232-
for (var i = 0; i < _circuitHandlers.Length; i++)
224+
var circuitHandler = _circuitHandlers[i];
225+
try
233226
{
234-
var circuitHandler = _circuitHandlers[i];
235-
try
236-
{
237-
await circuitHandler.OnConnectionUpAsync(Circuit, cancellationToken);
238-
}
239-
catch (Exception ex)
240-
{
241-
Log.CircuitHandlerFailed(_logger, circuitHandler, nameof(CircuitHandler.OnConnectionUpAsync), ex);
242-
exceptions ??= new List<Exception>();
243-
exceptions.Add(ex);
244-
}
227+
await circuitHandler.OnConnectionUpAsync(Circuit, cancellationToken);
245228
}
246-
247-
if (exceptions != null)
229+
catch (Exception ex)
248230
{
249-
throw new AggregateException("Encountered exceptions while executing circuit handlers.", exceptions);
231+
Log.CircuitHandlerFailed(_logger, circuitHandler, nameof(CircuitHandler.OnConnectionUpAsync), ex);
232+
exceptions ??= new List<Exception>();
233+
exceptions.Add(ex);
250234
}
251235
}
252-
finally
236+
237+
if (exceptions != null)
253238
{
254-
HandlerLock.Release();
239+
throw new AggregateException("Encountered exceptions while executing circuit handlers.", exceptions);
255240
}
256241
}
257242

258243
public async Task OnConnectionDownAsync(CancellationToken cancellationToken)
259244
{
260245
Log.ConnectionDown(_logger, Circuit.Id, Client.ConnectionId);
261246

262-
await HandlerLock.WaitAsync(cancellationToken);
247+
Renderer.Dispatcher.AssertAccess();
248+
249+
List<Exception> exceptions = null;
263250

264-
try
251+
for (var i = 0; i < _circuitHandlers.Length; i++)
265252
{
266-
List<Exception> exceptions = null;
267-
268-
for (var i = 0; i < _circuitHandlers.Length; i++)
253+
var circuitHandler = _circuitHandlers[i];
254+
try
269255
{
270-
var circuitHandler = _circuitHandlers[i];
271-
try
272-
{
273-
await circuitHandler.OnConnectionDownAsync(Circuit, cancellationToken);
274-
}
275-
catch (Exception ex)
276-
{
277-
Log.CircuitHandlerFailed(_logger, circuitHandler, nameof(CircuitHandler.OnConnectionDownAsync), ex);
278-
exceptions ??= new List<Exception>();
279-
exceptions.Add(ex);
280-
}
256+
await circuitHandler.OnConnectionDownAsync(Circuit, cancellationToken);
281257
}
282-
283-
if (exceptions != null)
258+
catch (Exception ex)
284259
{
285-
throw new AggregateException("Encountered exceptions while executing circuit handlers.", exceptions);
260+
Log.CircuitHandlerFailed(_logger, circuitHandler, nameof(CircuitHandler.OnConnectionDownAsync), ex);
261+
exceptions ??= new List<Exception>();
262+
exceptions.Add(ex);
286263
}
287264
}
288-
finally
265+
266+
if (exceptions != null)
289267
{
290-
HandlerLock.Release();
268+
throw new AggregateException("Encountered exceptions while executing circuit handlers.", exceptions);
291269
}
292270
}
293271

294272
private async Task OnCircuitDownAsync(CancellationToken cancellationToken)
295273
{
296274
Log.CircuitClosed(_logger, Circuit.Id);
297275

298-
await HandlerLock.WaitAsync(cancellationToken);
276+
List<Exception> exceptions = null;
299277

300-
try
278+
for (var i = 0; i < _circuitHandlers.Length; i++)
301279
{
302-
List<Exception> exceptions = null;
303-
304-
for (var i = 0; i < _circuitHandlers.Length; i++)
280+
var circuitHandler = _circuitHandlers[i];
281+
try
305282
{
306-
var circuitHandler = _circuitHandlers[i];
307-
try
308-
{
309-
await circuitHandler.OnCircuitClosedAsync(Circuit, cancellationToken);
310-
}
311-
catch (Exception ex)
312-
{
313-
Log.CircuitHandlerFailed(_logger, circuitHandler, nameof(CircuitHandler.OnCircuitClosedAsync), ex);
314-
exceptions ??= new List<Exception>();
315-
exceptions.Add(ex);
316-
}
283+
await circuitHandler.OnCircuitClosedAsync(Circuit, cancellationToken);
317284
}
318-
319-
if (exceptions != null)
285+
catch (Exception ex)
320286
{
321-
throw new AggregateException("Encountered exceptions while executing circuit handlers.", exceptions);
287+
Log.CircuitHandlerFailed(_logger, circuitHandler, nameof(CircuitHandler.OnCircuitClosedAsync), ex);
288+
exceptions ??= new List<Exception>();
289+
exceptions.Add(ex);
322290
}
323291
}
324-
finally
292+
293+
if (exceptions != null)
325294
{
326-
HandlerLock.Release();
295+
throw new AggregateException("Encountered exceptions while executing circuit handlers.", exceptions);
327296
}
328297
}
329298

src/Components/Server/test/Circuits/CircuitRegistryTest.cs

-72
Original file line numberDiff line numberDiff line change
@@ -270,46 +270,6 @@ public async Task Connect_WhileDisconnectIsInProgress()
270270
Assert.False(registry.DisconnectedCircuits.TryGetValue(circuitHost.CircuitId, out _));
271271
}
272272

273-
[Fact]
274-
public async Task Connect_WhileDisconnectIsInProgress_SeriallyExecutesCircuitHandlers()
275-
{
276-
// Arrange
277-
var circuitIdFactory = TestCircuitIdFactory.CreateTestFactory();
278-
279-
var registry = new TestCircuitRegistry(circuitIdFactory);
280-
registry.BeforeDisconnect = new ManualResetEventSlim();
281-
// This verifies that connection up \ down events on a circuit handler are always invoked serially.
282-
var circuitHandler = new SerialCircuitHandler();
283-
var tcs = new TaskCompletionSource<int>();
284-
285-
var circuitHost = TestCircuitHost.Create(circuitIdFactory.CreateCircuitId(), handlers: new[] { circuitHandler });
286-
registry.Register(circuitHost);
287-
var client = Mock.Of<IClientProxy>();
288-
var newId = "new-connection";
289-
290-
// Act
291-
var disconnect = Task.Run(() =>
292-
{
293-
var task = registry.DisconnectAsync(circuitHost, circuitHost.Client.ConnectionId);
294-
tcs.SetResult(0);
295-
return task;
296-
});
297-
var connect = Task.Run(async () =>
298-
{
299-
registry.BeforeDisconnect.Set();
300-
await tcs.Task;
301-
await registry.ConnectAsync(circuitHost.CircuitId, client, newId, default);
302-
});
303-
await Task.WhenAll(disconnect, connect);
304-
305-
// Assert
306-
Assert.Single(registry.ConnectedCircuits.Values);
307-
Assert.False(registry.DisconnectedCircuits.TryGetValue(circuitHost.CircuitId, out _));
308-
309-
Assert.True(circuitHandler.OnConnectionDownExecuted, "OnConnectionDownAsync should have been executed.");
310-
Assert.True(circuitHandler.OnConnectionUpExecuted, "OnConnectionUpAsync should have been executed.");
311-
}
312-
313273
[Fact]
314274
public async Task DisconnectWhenAConnectIsInProgress()
315275
{
@@ -444,37 +404,5 @@ private static CircuitRegistry CreateRegistry(CircuitIdFactory factory = null)
444404
NullLogger<CircuitRegistry>.Instance,
445405
factory ?? TestCircuitIdFactory.CreateTestFactory());
446406
}
447-
448-
private class SerialCircuitHandler : CircuitHandler
449-
{
450-
private readonly SemaphoreSlim _sempahore = new SemaphoreSlim(1);
451-
452-
public bool OnConnectionUpExecuted { get; private set; }
453-
public bool OnConnectionDownExecuted { get; private set; }
454-
455-
public override async Task OnConnectionUpAsync(Circuit circuit, CancellationToken cancellationToken)
456-
{
457-
Assert.True(await _sempahore.WaitAsync(0), "This should be serialized and consequently without contention");
458-
await Task.Delay(10);
459-
460-
Assert.False(OnConnectionUpExecuted);
461-
Assert.True(OnConnectionDownExecuted);
462-
OnConnectionUpExecuted = true;
463-
464-
_sempahore.Release();
465-
}
466-
467-
public override async Task OnConnectionDownAsync(Circuit circuit, CancellationToken cancellationToken)
468-
{
469-
Assert.True(await _sempahore.WaitAsync(0), "This should be serialized and consequently without contention");
470-
await Task.Delay(10);
471-
472-
Assert.False(OnConnectionUpExecuted);
473-
Assert.False(OnConnectionDownExecuted);
474-
OnConnectionDownExecuted = true;
475-
476-
_sempahore.Release();
477-
}
478-
}
479407
}
480408
}

src/Components/test/E2ETest/ServerExecutionTests/InteropReliabilityTests.cs

+1
Original file line numberDiff line numberDiff line change
@@ -264,6 +264,7 @@ await Client.InvokeDotNetMethod(
264264
}
265265

266266
[Fact]
267+
[Flaky("https://github.com/aspnet/AspNetCore/issues/13086", FlakyOn.AzP.Windows)]
267268
public async Task ContinuesWorkingAfterInvalidAsyncReturnCallback()
268269
{
269270
// Arrange

src/Components/test/E2ETest/ServerExecutionTests/ServerComponentRenderingTest.cs

+1-1
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ public void ThrowsIfRenderIsRequestedOutsideSyncContext()
3333
appElement.FindElement(By.Id("run-without-dispatch")).Click();
3434

3535
Browser.Contains(
36-
$"{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.",
36+
$"{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.",
3737
() => result.Text);
3838
}
3939
}

0 commit comments

Comments
 (0)