StdSchedulerFactory and derived factories are not threadsafe

[jcracknell]

GetScheduler(CancellationToken) uses SchedulerRepository.Instance to check if the the scheduler already exists. If it does not, then then Instantiate() later adds the scheduler to the repository singleton:

quartznet/src/Quartz/Impl/StdSchedulerFactory.cs

Lines 1099 to 1113 in 7d0dc1b

	IScheduler? sched = await schedRep.Lookup(SchedulerName, cancellationToken).ConfigureAwait(false);
	if (sched != null)
	{
	if (sched.IsShutdown)
	{
	schedRep.Remove(SchedulerName);
	}
	else
	{
	return sched;
	}
	}
	sched = await Instantiate().ConfigureAwait(false);

In the intervening time another thread may call GetScheduler, resulting in an exception from SchedulerRepository.Bind:

quartznet/src/Quartz/Impl/SchedulerRepository.cs

Lines 53 to 64 in 7d0dc1b

	public virtual void Bind(IScheduler sched)
	{
	lock (syncRoot)
	{
	if (schedulers.ContainsKey(sched.SchedulerName))
	{
	throw new SchedulerException($"Scheduler with name '{sched.SchedulerName}' already exists.");
	}
	schedulers[sched.SchedulerName] = sched;
	}
	}

This is problematic under dependency injection where the ISchedulerFactory seems to be the point of access for the scheduler. I see #1265, however this does not address the issue as the race occurs within StdSchedulerFactory prior to acquiring the semaphore.

Also related: #1453.

[lahma]

Would you like to offer a PR against the 3.x branch which would improve things?

[jcracknell]

Done. In the interim if anyone is waiting for a release you could also e.g.

public class ThreadsafeSchedulerFactory : ISchedulerFactory {
    private readonly ISchedulerFactory _schedulerFactory;
    private readonly SemaphoreSlim _semaphore;
    private bool _initialized;

    public ThreadsafeSchedulerFactory(IServiceProvider serviceProvider) {
        // ServiceCollectionSchedulerFactory is internal, so we need to instantiate it via reflection
        const string implementationTypeName = "Quartz.ServiceCollectionSchedulerFactory";
        var implementationType = typeof(Quartz.ServiceCollectionExtensions).Assembly.GetType(implementationTypeName);
        if(implementationType is null)
            throw new Exception($"Unable to resolve type {implementationTypeName}.");

        _schedulerFactory = (ISchedulerFactory)ActivatorUtilities.CreateInstance(serviceProvider, implementationType);
        _semaphore = new SemaphoreSlim(1, 1);
        _initialized = false;
    }

    public async Task<IScheduler> GetScheduler(CancellationToken cancellationToken = default) {
        if(_initialized)
            return await _schedulerFactory.GetScheduler(cancellationToken);

        await _semaphore.WaitAsync(cancellationToken);
        try {
            var scheduler = await _schedulerFactory.GetScheduler(cancellationToken);
            _initialized = true;
            return scheduler;
        } finally {
            _semaphore.Release();
        }
    }

    // These methods just forward to SchedulerRepostiory.Instance and probably should not be used

    public Task<IReadOnlyList<IScheduler>> GetAllSchedulers(CancellationToken cancellationToken = default) =>
        _schedulerFactory.GetAllSchedulers(cancellationToken);

    public Task<IScheduler> GetScheduler(string schedName, CancellationToken cancellationToken = default) =>
        _schedulerFactory.GetScheduler(schedName, cancellationToken);
}

and then:

services.Replace(ServiceDescriptor.Singleton<ISchedulerFactory, ThreadsafeSchedulerFactory>());