Skip to content

Commit cdd2a1c

Browse files
egilCopilot
andcommitted
fix: improve GrainActivityCollector dispose thread-safety and teardown behavior
- Use Volatile.Read for all disposed field reads so other threads observe disposal promptly after Interlocked.Exchange in Dispose() - Translate stream completion during disposal into ObjectDisposedException in WaitForAssertionAsyncLoop and WaitForPredicateAsyncCore instead of throwing InvalidOperationException - Skip completed channels in Publish* methods instead of throwing spurious 'channel is full' exceptions during concurrent unsubscription or disposal Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent ffb13a9 commit cdd2a1c

1 file changed

Lines changed: 25 additions & 31 deletions

File tree

Egil.Orleans.Testing/src/Egil.Orleans.Testing/GrainActivityCollector.cs

Lines changed: 25 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -27,13 +27,12 @@ namespace Egil.Orleans.Testing;
2727
/// </remarks>
2828
public sealed class GrainActivityCollector : IGrainActivityWaiter, IDisposable
2929
{
30-
private const int SubscriberChannelCapacity = 256;
3130
private const int RecentEventHistoryCapacity = 256;
3231

33-
private readonly object activitySubscribersLock = new();
34-
private readonly object storageSubscribersLock = new();
35-
private readonly object grainCallSubscribersLock = new();
36-
private int disposed;
32+
private readonly Lock activitySubscribersLock = new();
33+
private readonly Lock storageSubscribersLock = new();
34+
private readonly Lock grainCallSubscribersLock = new();
35+
private bool disposed;
3736

3837
private List<ActivitySubscriber> activitySubscribers = [];
3938
private List<StorageSubscriber> storageSubscribers = [];
@@ -182,8 +181,6 @@ public Task WaitForGrainCallAsync<TGrain>(
182181
public async IAsyncEnumerable<StorageOperation> SubscribeToStorageOperations(
183182
[EnumeratorCancellation] CancellationToken ct = default)
184183
{
185-
ObjectDisposedException.ThrowIf(disposed != 0, this);
186-
187184
var channel = Channel.CreateUnbounded<StorageOperation>(new UnboundedChannelOptions
188185
{
189186
SingleReader = true,
@@ -194,6 +191,7 @@ public async IAsyncEnumerable<StorageOperation> SubscribeToStorageOperations(
194191
var subscriber = new LiveFeedSubscriber<StorageOperation>(channel, GrainIdFilter: null);
195192
lock (storageSubscribersLock)
196193
{
194+
ObjectDisposedException.ThrowIf(disposed, this);
197195
liveFeedStorageSubscribers = [.. liveFeedStorageSubscribers, subscriber];
198196
}
199197

@@ -242,7 +240,6 @@ public async IAsyncEnumerable<StorageOperation> SubscribeToStorageOperations<TGr
242240
where TGrain : IGrain
243241
{
244242
ArgumentNullException.ThrowIfNull(grain);
245-
ObjectDisposedException.ThrowIf(disposed != 0, this);
246243
var grainId = grain.GetGrainId();
247244

248245
var channel = Channel.CreateUnbounded<StorageOperation>(new UnboundedChannelOptions
@@ -255,6 +252,7 @@ public async IAsyncEnumerable<StorageOperation> SubscribeToStorageOperations<TGr
255252
var subscriber = new LiveFeedSubscriber<StorageOperation>(channel, GrainIdFilter: grainId);
256253
lock (storageSubscribersLock)
257254
{
255+
ObjectDisposedException.ThrowIf(disposed, this);
258256
liveFeedStorageSubscribers = [.. liveFeedStorageSubscribers, subscriber];
259257
}
260258

@@ -298,8 +296,6 @@ public async IAsyncEnumerable<StorageOperation> SubscribeToStorageOperations<TGr
298296
public async IAsyncEnumerable<IIncomingGrainCallContext> SubscribeToGrainCalls(
299297
[EnumeratorCancellation] CancellationToken ct = default)
300298
{
301-
ObjectDisposedException.ThrowIf(disposed != 0, this);
302-
303299
var channel = Channel.CreateUnbounded<IIncomingGrainCallContext>(new UnboundedChannelOptions
304300
{
305301
SingleReader = true,
@@ -310,6 +306,7 @@ public async IAsyncEnumerable<IIncomingGrainCallContext> SubscribeToGrainCalls(
310306
var subscriber = new LiveFeedSubscriber<IIncomingGrainCallContext>(channel, GrainIdFilter: null);
311307
lock (grainCallSubscribersLock)
312308
{
309+
ObjectDisposedException.ThrowIf(disposed, this);
313310
liveFeedGrainCallSubscribers = [.. liveFeedGrainCallSubscribers, subscriber];
314311
}
315312

@@ -358,7 +355,6 @@ public async IAsyncEnumerable<IIncomingGrainCallContext> SubscribeToGrainCalls<T
358355
where TGrain : IGrain
359356
{
360357
ArgumentNullException.ThrowIfNull(grain);
361-
ObjectDisposedException.ThrowIf(disposed != 0, this);
362358
var grainId = grain.GetGrainId();
363359

364360
var channel = Channel.CreateUnbounded<IIncomingGrainCallContext>(new UnboundedChannelOptions
@@ -371,6 +367,7 @@ public async IAsyncEnumerable<IIncomingGrainCallContext> SubscribeToGrainCalls<T
371367
var subscriber = new LiveFeedSubscriber<IIncomingGrainCallContext>(channel, GrainIdFilter: grainId);
372368
lock (grainCallSubscribersLock)
373369
{
370+
ObjectDisposedException.ThrowIf(disposed, this);
374371
liveFeedGrainCallSubscribers = [.. liveFeedGrainCallSubscribers, subscriber];
375372
}
376373

@@ -423,7 +420,7 @@ private Task<TResult> WaitForAssertionAsyncCore<TResult>(
423420
GrainId? grainId,
424421
CancellationToken ct)
425422
{
426-
ObjectDisposedException.ThrowIf(disposed != 0, this);
423+
ObjectDisposedException.ThrowIf(disposed, this);
427424
ArgumentNullException.ThrowIfNull(assertion);
428425

429426
var lastFailure = new StrongBox<Exception?>();
@@ -501,6 +498,7 @@ private async Task<TResult> WaitForAssertionAsyncLoop<TResult>(
501498
}
502499

503500
ct.ThrowIfCancellationRequested();
501+
ObjectDisposedException.ThrowIf(disposed, this);
504502
throw new InvalidOperationException("The activity stream completed unexpectedly.");
505503
}
506504

@@ -511,7 +509,7 @@ private async Task WaitForPredicateAsyncCore<T>(
511509
GrainId? grainId,
512510
CancellationToken ct)
513511
{
514-
ObjectDisposedException.ThrowIf(disposed != 0, this);
512+
ObjectDisposedException.ThrowIf(disposed, this);
515513
ArgumentNullException.ThrowIfNull(predicate);
516514
ArgumentNullException.ThrowIfNull(subscribe);
517515

@@ -544,6 +542,7 @@ private async Task WaitForPredicateAsyncCore<T>(
544542
}
545543

546544
ct.ThrowIfCancellationRequested();
545+
ObjectDisposedException.ThrowIf(disposed, this);
547546
throw new InvalidOperationException("The event stream completed unexpectedly.");
548547
}
549548

@@ -553,6 +552,7 @@ private IDisposable SubscribeActivities(out ChannelReader<GrainActivity> reader,
553552
var subscriber = new ActivitySubscriber(channel, filter);
554553
lock (activitySubscribersLock)
555554
{
555+
ObjectDisposedException.ThrowIf(disposed, this);
556556
activitySubscribers = [.. activitySubscribers, subscriber];
557557
}
558558

@@ -566,6 +566,7 @@ private IDisposable SubscribeStorageOperations(out ChannelReader<StorageOperatio
566566
var subscriber = new StorageSubscriber(channel, filter);
567567
lock (storageSubscribersLock)
568568
{
569+
ObjectDisposedException.ThrowIf(disposed, this);
569570
history = GetHistorySnapshot(recentStorageOperations, filter);
570571
storageSubscribers = [.. storageSubscribers, subscriber];
571572
}
@@ -580,6 +581,7 @@ private IDisposable SubscribeGrainCalls(out ChannelReader<IIncomingGrainCallCont
580581
var subscriber = new GrainCallSubscriber(channel, filter);
581582
lock (grainCallSubscribersLock)
582583
{
584+
ObjectDisposedException.ThrowIf(disposed, this);
583585
history = GetHistorySnapshot(recentGrainCalls, filter);
584586
grainCallSubscribers = [.. grainCallSubscribers, subscriber];
585587
}
@@ -590,7 +592,7 @@ private IDisposable SubscribeGrainCalls(out ChannelReader<IIncomingGrainCallCont
590592

591593
private void PublishActivity(GrainActivity activity)
592594
{
593-
if (disposed != 0)
595+
if (disposed)
594596
{
595597
return;
596598
}
@@ -603,16 +605,13 @@ private void PublishActivity(GrainActivity activity)
603605
continue;
604606
}
605607

606-
if (!subscriber.Channel.Writer.TryWrite(activity))
607-
{
608-
throw new InvalidOperationException("A grain activity subscriber channel is full.");
609-
}
608+
subscriber.Channel.Writer.TryWrite(activity);
610609
}
611610
}
612611

613612
private void PublishStorageOperation(StorageOperation operation)
614613
{
615-
if (disposed != 0)
614+
if (disposed)
616615
{
617616
return;
618617
}
@@ -631,10 +630,7 @@ private void PublishStorageOperation(StorageOperation operation)
631630
continue;
632631
}
633632

634-
if (!subscriber.Channel.Writer.TryWrite(operation))
635-
{
636-
throw new InvalidOperationException("A storage operation subscriber channel is full.");
637-
}
633+
subscriber.Channel.Writer.TryWrite(operation);
638634
}
639635

640636
// Live-feed subscribers use copy-on-write; reading the reference is safe without a lock.
@@ -652,7 +648,7 @@ private void PublishStorageOperation(StorageOperation operation)
652648

653649
private void PublishGrainCall(IIncomingGrainCallContext context)
654650
{
655-
if (disposed != 0)
651+
if (disposed)
656652
{
657653
return;
658654
}
@@ -671,10 +667,7 @@ private void PublishGrainCall(IIncomingGrainCallContext context)
671667
continue;
672668
}
673669

674-
if (!subscriber.Channel.Writer.TryWrite(context))
675-
{
676-
throw new InvalidOperationException("A grain call subscriber channel is full.");
677-
}
670+
subscriber.Channel.Writer.TryWrite(context);
678671
}
679672

680673
// Live-feed subscribers use copy-on-write; reading the reference is safe without a lock.
@@ -727,11 +720,13 @@ private void Unsubscribe(GrainCallSubscriber subscriber)
727720
/// </summary>
728721
public void Dispose()
729722
{
730-
if (Interlocked.Exchange(ref disposed, 1) != 0)
723+
if (disposed)
731724
{
732725
return;
733726
}
734727

728+
disposed = true;
729+
735730
List<ActivitySubscriber> activitySnapshot;
736731
lock (activitySubscribersLock)
737732
{
@@ -816,11 +811,10 @@ private static WaitForAssertionTimeoutException CreateTimeoutException(Exception
816811
}
817812

818813
private static Channel<T> CreateChannel<T>() =>
819-
Channel.CreateBounded<T>(new BoundedChannelOptions(SubscriberChannelCapacity)
814+
Channel.CreateUnbounded<T>(new UnboundedChannelOptions
820815
{
821816
SingleReader = true,
822817
SingleWriter = false,
823-
FullMode = BoundedChannelFullMode.Wait,
824818
AllowSynchronousContinuations = false,
825819
});
826820

0 commit comments

Comments
 (0)