Skip to content

Commit

Permalink
fix race condition in run once at start logic
Browse files Browse the repository at this point in the history
  • Loading branch information
jamesmh committed Jan 17, 2025
1 parent 04df9a6 commit 88ea3e8
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 4 deletions.
18 changes: 16 additions & 2 deletions Src/Coravel/Scheduling/Schedule/Scheduler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public class Scheduler : IScheduler, ISchedulerConfiguration
private string _currentWorkerName;
private CancellationTokenSource _cancellationTokenSource;
private bool _isFirstTick = true;
private object _isFirstTickLockObj = new object();

public Scheduler(IMutex mutex, IServiceScopeFactory scopeFactory, IDispatcher dispatcher)
{
Expand Down Expand Up @@ -102,8 +103,21 @@ public IScheduler OnWorker(string workerName)
public async Task RunAtAsync(DateTime utcDate)
{
Interlocked.Increment(ref this._schedulerIterationsActiveCount);
bool isFirstTick = this._isFirstTick;
this._isFirstTick = false;

// Possibility for race condition here.
// Note: This was a bug fixed by issue 353.
bool isFirstTick = false;
if(this._isFirstTick)
{
lock(this._isFirstTickLockObj)
{
// Set to `this._isFirstTick` not `true` because there might be multiple threads coming into
// this critical section - but only one should count as "the first run".
isFirstTick = this._isFirstTick;
this._isFirstTick = false;
}
}

await RunWorkersAt(utcDate, isFirstTick);
Interlocked.Decrement(ref this._schedulerIterationsActiveCount);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
using System;
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;
using Coravel.Scheduling.Schedule;
using Coravel.Scheduling.Schedule.Mutex;
Expand Down Expand Up @@ -38,8 +41,8 @@ public async Task TestRunsOnlyAtStart(int atHour, int atMinute, int day, int hou
Assert.True(taskRan);

taskRan = false;

await RunScheduledTasksFromDayHourMinutes(scheduler, day, hour, minute);

Assert.False(taskRan);
}

Expand Down Expand Up @@ -67,9 +70,51 @@ public async Task TestRunsAtStartAndWhenDue(int atHour, int atMinute, int day, i
Assert.True(taskRan);

taskRan = false;
await RunScheduledTasksFromDayHourMinutes(scheduler, day, hour, minute);

await RunScheduledTasksFromDayHourMinutes(scheduler, day, hour, minute);
Assert.True(taskRan);
}

/// <summary>
/// This test runs for a long time - like 40 seconds on a fast codespaces VM. But, that's how many iterations of running this test takes to
/// catch a really rare race condition. This was causing the `RunOnceAtStart` method to have an bug where intermittently those tasks weren't running
/// when the application started up.
/// </summary>
[Fact]
public async Task TestConcurrentThreadsDoesNotSkipForcedRun()
{
for(int i = 0; i < 10000; i++)
{
var scheduler = new Scheduler(new InMemoryMutex(), new ServiceScopeFactoryStub(), new DispatcherStub());
var taskRan = false;
var taskRunCount = 0;

scheduler
.Schedule(() =>
{
Interlocked.Increment(ref taskRunCount);
taskRan = true;
})
.Monthly()
.RunOnceAtStart();


Func<Task> runTask = async () =>
{
await Task.Delay(1);
await scheduler.RunAtAsync(new DateTime(2000, 1, 15));
};

var tasks = new List<Task>()
{
runTask(), runTask(), runTask()
};

await Task.WhenAll(tasks);

Assert.True(taskRan);
Assert.Equal(1, taskRunCount);
}
}
}
}

0 comments on commit 88ea3e8

Please sign in to comment.