Skip to content

Merge pull request #63 from jeanlrnt/copilot/refactor-job-registry-locks #64

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jeanlrnt merged 4 commits into from
Jan 2, 2026
Merged

Merge pull request #63 from jeanlrnt/copilot/refactor-job-registry-locks #64

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5847297
Initial plan
Copilot Jan 2, 2026
6ca5682
refactor(JobRegistry): Move event invocations outside WriteLock - Job…
Copilot Jan 2, 2026
e5cf6ca
fix: Address PR review feedback - remove async void handlers, unused …
Copilot Jan 2, 2026
c3f9a4a
Merge pull request #63 from jeanlrnt/copilot/refactor-job-registry-locks
jeanlrnt Jan 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 100 additions & 43 deletions src/MiniCron.Core/Services/JobRegistry.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@ public class JobRegistry : IDisposable
/// Occurs when a job is added to the registry.
/// </summary>
/// <remarks>
/// This event is raised inside the write lock, ensuring event handlers observe consistent registry state.
/// Event handlers should be lightweight to avoid blocking other registry operations.
/// This event is raised after the write lock is released, ensuring event handlers do not block registry operations.
/// Event handlers observe the job state at the time of addition.
/// <para>
/// <strong>Note:</strong> Event handlers may call back into the JobRegistry (e.g., RemoveJob, UpdateSchedule, ScheduleJob)
/// as the lock supports recursion. However, this should be done judiciously to avoid performance degradation.
/// <strong>Note:</strong> Event handlers can safely call back into the JobRegistry (e.g., RemoveJob, UpdateSchedule, ScheduleJob)
/// without risk of deadlock, as the event is invoked outside the lock.
/// </para>
/// </remarks>
public event EventHandler<JobEventArgs>? JobAdded;
Expand All @@ -27,11 +27,11 @@ public class JobRegistry : IDisposable
/// Occurs when a job is removed from the registry.
/// </summary>
/// <remarks>
/// This event is raised inside the write lock, ensuring event handlers observe consistent registry state.
/// Event handlers should be lightweight to avoid blocking other registry operations.
/// This event is raised after the write lock is released, ensuring event handlers do not block registry operations.
/// Event handlers observe the job state at the time of removal.
/// <para>
/// <strong>Note:</strong> Event handlers may call back into the JobRegistry (e.g., RemoveJob, UpdateSchedule, ScheduleJob)
/// as the lock supports recursion. However, this should be done judiciously to avoid performance degradation.
/// <strong>Note:</strong> Event handlers can safely call back into the JobRegistry (e.g., RemoveJob, UpdateSchedule, ScheduleJob)
/// without risk of deadlock, as the event is invoked outside the lock.
/// </para>
/// </remarks>
public event EventHandler<JobEventArgs>? JobRemoved;
Expand All @@ -40,11 +40,11 @@ public class JobRegistry : IDisposable
/// Occurs when a job's schedule is updated in the registry.
/// </summary>
/// <remarks>
/// This event is raised inside the write lock, ensuring event handlers observe consistent registry state.
/// Event handlers should be lightweight to avoid blocking other registry operations.
/// This event is raised after the write lock is released, ensuring event handlers do not block registry operations.
/// Event handlers observe the job state at the time of update.
/// <para>
/// <strong>Note:</strong> Event handlers may call back into the JobRegistry (e.g., RemoveJob, UpdateSchedule, ScheduleJob)
/// as the lock supports recursion. However, this should be done judiciously to avoid performance degradation.
/// <strong>Note:</strong> Event handlers can safely call back into the JobRegistry (e.g., RemoveJob, UpdateSchedule, ScheduleJob)
/// without risk of deadlock, as the event is invoked outside the lock.
/// </para>
/// </remarks>
public event EventHandler<JobEventArgs>? JobUpdated;
Expand All @@ -65,26 +65,39 @@ public Guid ScheduleJob(string cronExpression, Func<IServiceProvider, Cancellati
CronHelper.ValidateCronExpression(cronExpression);

var job = new CronJob(cronExpression, action);
JobEventArgs? eventArgs = null;

_lock.EnterWriteLock();
try
{
_jobs.Add(job.Id, job);
_logger?.LogInformation("Job added: {JobId} {Cron}", job.Id, job.CronExpression);

// Prepare event args if there are subscribers, but don't invoke yet
if (JobAdded != null)
{
eventArgs = new JobEventArgs(job);
}
}
finally
{
_lock.ExitWriteLock();
}

// Invoke event outside the lock
if (eventArgs != null)
{
try
{
JobAdded?.Invoke(this, new JobEventArgs(job));
JobAdded?.Invoke(this, eventArgs);
}
catch (Exception ex)
{
_logger?.LogError(ex, "Unhandled exception in JobAdded event handler for job {JobId}", job.Id);
}
return job.Id;
}
finally
{
_lock.ExitWriteLock();
}

return job.Id;
}

/// <summary>
Expand Down Expand Up @@ -119,25 +132,38 @@ public Guid ScheduleJob(string cronExpression, Func<IServiceProvider, Cancellati
CronHelper.ValidateCronExpression(cronExpression);

var job = new CronJob(cronExpression, action, timeout);
JobEventArgs? eventArgs = null;

_lock.EnterWriteLock();
try
{
_jobs.Add(job.Id, job);
_logger?.LogInformation("Job added: {JobId} {Cron} Timeout: {Timeout}", job.Id, job.CronExpression, timeout);

// Prepare event args if there are subscribers, but don't invoke yet
if (JobAdded != null)
{
eventArgs = new JobEventArgs(job);
}
}
finally
{
_lock.ExitWriteLock();
}

// Invoke event outside the lock
if (eventArgs != null)
{
try
{
JobAdded?.Invoke(this, new JobEventArgs(job));
JobAdded?.Invoke(this, eventArgs);
}
catch (Exception ex)
{
_logger?.LogError(ex, "Unhandled exception in JobAdded event handler for job {JobId}", job.Id);
}
}
finally
{
_lock.ExitWriteLock();
}

return job.Id;
}

Expand Down Expand Up @@ -168,6 +194,9 @@ public Guid ScheduleJob(string cronExpression, Action action, TimeSpan? timeout)
/// <returns>True if the job was found and removed; otherwise, false.</returns>
public bool RemoveJob(Guid jobId)
{
CronJob? removedJob = null;
JobEventArgs? eventArgs = null;

_lock.EnterWriteLock();
try
{
Expand All @@ -178,22 +207,34 @@ public bool RemoveJob(Guid jobId)
}

// At this point, job is guaranteed to be non-null because Remove returned true
_logger?.LogInformation("Job removed: {JobId} {Cron}", jobId, job!.CronExpression);
try
{
JobRemoved?.Invoke(this, new JobEventArgs(job!));
}
catch (Exception ex)
removedJob = job!;
_logger?.LogInformation("Job removed: {JobId} {Cron}", jobId, removedJob.CronExpression);

// Prepare event args if there are subscribers, but don't invoke yet
if (JobRemoved != null)
{
_logger?.LogError(ex, "Unhandled exception in JobRemoved event handler for job {JobId} {Cron}", jobId, job!.CronExpression);
eventArgs = new JobEventArgs(removedJob);
}

return true;
}
finally
{
_lock.ExitWriteLock();
}

// Invoke event outside the lock
if (eventArgs != null)
{
try
{
JobRemoved?.Invoke(this, eventArgs);
}
catch (Exception ex)
{
_logger?.LogError(ex, "Unhandled exception in JobRemoved event handler for job {JobId} {Cron}", jobId, removedJob!.CronExpression);
}
}

return removedJob != null;
}

/// <summary>
Expand All @@ -206,38 +247,54 @@ public bool UpdateSchedule(Guid jobId, string newCronExpression)
{
CronHelper.ValidateCronExpression(newCronExpression);

CronJob? updatedJob = null;
CronJob? existingJob = null;
JobEventArgs? eventArgs = null;

_lock.EnterWriteLock();
try
{
if (!_jobs.TryGetValue(jobId, out var existingJob))
if (!_jobs.TryGetValue(jobId, out var oldJob))
{
return false;
}

var updatedJob = existingJob with { CronExpression = newCronExpression };
existingJob = oldJob;
updatedJob = existingJob with { CronExpression = newCronExpression };
_jobs[jobId] = updatedJob;

_logger?.LogInformation("Job updated: {JobId} {OldCron} -> {NewCron}", jobId, existingJob.CronExpression, newCronExpression);

// Prepare event args if there are subscribers, but don't invoke yet
if (JobUpdated != null)
{
eventArgs = new JobEventArgs(updatedJob, existingJob);
}
}
finally
{
_lock.ExitWriteLock();
}

// Invoke event outside the lock
if (eventArgs != null)
{
try
{
JobUpdated?.Invoke(this, new JobEventArgs(updatedJob, existingJob));
JobUpdated?.Invoke(this, eventArgs);
}
catch (Exception ex)
{
_logger?.LogError(
ex,
"Unhandled exception in JobUpdated event handler for job {JobId} with cron change {OldCron} -> {NewCron}",
jobId,
existingJob.CronExpression,
updatedJob.CronExpression);
existingJob!.CronExpression,
updatedJob!.CronExpression);
}

return true;
}
finally
{
_lock.ExitWriteLock();
}

return updatedJob != null;
}

/// <summary>
Expand Down
Loading