Skip to content

Commit 69202c8

Browse files
committed
Fix disposal race condition in AsyncBatchHandler
`DisposeAsync` now awaits both background tasks (`_timerTask` and `_flushSignalTask`) before attempting the final flush. Previously, cancellation and resource disposal happened without waiting for in-flight flushes to complete, so the final `FlushBatchesAsync` could be silently skipped when `_flushing` was still held at 1. Fixes #146
1 parent e95b57c commit 69202c8

2 files changed

Lines changed: 53 additions & 6 deletions

File tree

src/PostHog/Library/AsyncBatchHandler.cs

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,8 @@ internal sealed class AsyncBatchHandler<TItem, TBatchContext> : IDisposable, IAs
4343
readonly PeriodicTimer _timer;
4444
readonly CancellationTokenSource _cancellationTokenSource = new();
4545
readonly SemaphoreSlim _flushSignal = new(0); // Used to signal when a flush is needed
46+
readonly Task _timerTask;
47+
readonly Task _flushSignalTask;
4648
volatile int _disposed;
4749
volatile int _flushing;
4850

@@ -63,8 +65,8 @@ public AsyncBatchHandler(
6365
FullMode = BoundedChannelFullMode.DropOldest
6466
});
6567
_timer = new PeriodicTimer(options.Value.FlushInterval, timeProvider);
66-
taskScheduler.Run(() => HandleTimer(_cancellationTokenSource.Token));
67-
taskScheduler.Run(() => HandleFlushSignal(_cancellationTokenSource.Token));
68+
_timerTask = taskScheduler.Run(() => HandleTimer(_cancellationTokenSource.Token));
69+
_flushSignalTask = taskScheduler.Run(() => HandleFlushSignal(_cancellationTokenSource.Token));
6870
}
6971

7072
public AsyncBatchHandler(
@@ -244,17 +246,20 @@ public async ValueTask DisposeAsync()
244246

245247
_logger.LogInfoDisposeAsyncCalled();
246248

247-
// Ensures that both the HandleFlushSignal and HandleTimer throw
248-
// OperationCancelledException which is handled gracefully.
249+
// Cancel the token so both background loops exit, then wait for them to finish.
250+
// This ensures any in-flight flush completes and _flushing returns to 0 before
251+
// we attempt the final flush below.
249252
await _cancellationTokenSource.CancelAsync();
250-
_cancellationTokenSource.Dispose();
253+
await Task.WhenAll(_timerTask, _flushSignalTask);
254+
251255
_timer.Dispose();
252256
_flushSignal.Dispose();
257+
_cancellationTokenSource.Dispose();
253258
_channel.Writer.Complete();
254259
try
255260
{
256261
_logger.LogTraceFlushCalledInDispose(Count);
257-
// Flush the last remaining items.
262+
// Flush any remaining items that weren't picked up by the background tasks.
258263
await FlushBatchesAsync();
259264
}
260265
#pragma warning disable CA1031 // Do not catch general exception types

tests/UnitTests/Library/AsyncBatchHandlerTests.cs

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -330,6 +330,48 @@ public async Task DoesNotDisposeTwice()
330330
Assert.Equal([1, 2], items);
331331
}
332332

333+
[Fact]
334+
public async Task FlushesItemWhenDisposedDuringInFlightFlush()
335+
{
336+
var options = new FakeOptions<PostHogOptions>(new()
337+
{
338+
FlushAt = 1,
339+
FlushInterval = TimeSpan.FromHours(3)
340+
});
341+
var items = new List<int>();
342+
var flushStarted = new TaskCompletionSource();
343+
var flushCanProceed = new TaskCompletionSource();
344+
345+
Func<IEnumerable<int>, Task> handlerFunc = async batch =>
346+
{
347+
flushStarted.SetResult();
348+
await flushCanProceed.Task;
349+
items.AddRange(batch);
350+
};
351+
352+
var batchHandler = new AsyncBatchHandler<int, object>(
353+
handlerFunc, () => new object(), new FakeTimeProvider(), options);
354+
355+
// Enqueue triggers a flush (FlushAt = 1), which will block on flushCanProceed.
356+
batchHandler.Enqueue(Task.FromResult(42));
357+
await flushStarted.Task;
358+
359+
// Start disposal while the flush is still in progress.
360+
var disposeTask = batchHandler.DisposeAsync().AsTask();
361+
362+
// Unblock the flush handler so it can complete.
363+
flushCanProceed.SetResult();
364+
365+
var timeout = TimeSpan.FromSeconds(5);
366+
var completed = await Task.WhenAny(disposeTask, Task.Delay(timeout));
367+
if (completed != disposeTask)
368+
{
369+
throw new TimeoutException("DisposeAsync did not complete within 5 seconds; possible deadlock.");
370+
}
371+
372+
Assert.Equal([42], items);
373+
}
374+
333375
[Fact]
334376
public async Task HandlesExceptionsInFlushBatchAsync()
335377
{

0 commit comments

Comments
 (0)