Skip to content
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.

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

Make StdSchedulerFactory and ServiceCollectionSchedulerFactories threadsafe #1588

Merged
merged 4 commits into from
May 29, 2022

Conversation

jcracknell
Copy link

@jcracknell jcracknell commented Apr 22, 2022

fixes #1587

I moved the call to SchedulerRepository.Bind to GetScheduler, as this makes the registration process clearer.

You can't write tests against this as it would pollute the SchedulerRepository singleton as noted in #1453.

… stopped instance

ServiceCollectionSchedulerFactory.GetScheduler initializes new
schedulers created by the base method to replace a previous instance
which was stopped.
@jcracknell
Copy link
Author

jcracknell commented Apr 23, 2022

I thought about this some more last night and there's a second bug here where the ServiceCollectionSchedulerFactory would fail to initialize a new scheduler instance produced by StdSchedulerFactory in the event that the previously initialized scheduler was shut down:

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);
return sched!;

@lahma lahma modified the milestone: vNext May 29, 2022
Copy link
Member

@lahma lahma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you!

@lahma lahma merged commit be87f06 into quartznet:3.x May 29, 2022
@lahma
Copy link
Member

lahma commented Jun 26, 2022

@jcracknell I hate to ask, but would you be willing to create another PR against main as this doesn't merge cleanly due to some structural changes done there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants