madelson/DistributedLock

WaitHandles: Possible race condition in InternalWaitOneAsync()

madelson opened this issue · 1 comments

Referring to https://github.com/madelson/DistributedLock/blob/master/DistributedLock.WaitHandles/WaitHandleExtensions.cs#L43

Consider the following case:

  • CancellationToken fires and sets the task to canceled
  • WaitForSingleObject callback runs and fails to set the task

In that case, I think we lose a signal on the handle; ideally if the wait callback fails to set the task then it should re-signal the event.

Reproduction:

[Test]
        public async Task TestCancellationDoesNotLeadToLostSignal([Values] bool async)
        {
            var semaphore = new WaitHandleDistributedSemaphore(nameof(this.TestCancellationDoesNotLeadToLostSignal), 2);
            await using var _ = await semaphore.AcquireAsync(TimeSpan.FromSeconds(1));

            for (var i = 0; i < 50; ++i)
            {
                using var barrier = new Barrier(2);
                using var source = new CancellationTokenSource();
                var acquireTask = Task.Run(async () =>
                {
                    barrier.SignalAndWait();
                    try
                    { 
                        if (async) { await using var _ = await semaphore.AcquireAsync(cancellationToken: source.Token); }
                        else { using var _ = semaphore.Acquire(cancellationToken: source.Token); }
                    }
                    catch when (source.Token.IsCancellationRequested) { }
                });
                var cancelTask = Task.Run(() =>
                {
                    barrier.SignalAndWait();
                    source.Cancel();
                });
                await Task.WhenAll(acquireTask, cancelTask);
            }

            await using var handle = await semaphore.TryAcquireAsync();
            Assert.IsNotNull(handle); // if we lost even a single signal due to cancellation in the loop above, this will fail
        }