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.

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

Jump to bottom

Try resolving jobs from service provider before resorting to ActivatorUtilities #1159

Merged
merged 6 commits into from
Apr 9, 2021
Merged

Try resolving jobs from service provider before resorting to ActivatorUtilities #1159

merged 6 commits into from
Apr 9, 2021

Conversation

lahma
Copy link
Member

@lahma lahma commented Apr 9, 2021 •
edited

fixes #1157

@lahma lahma mentioned this pull request Apr 9, 2021
Closed
@kevinchalet
Copy link
Contributor

Should (InnerJob as IDisposable)?.Dispose(); be behind a if to prevent double-disposal if the service comes from DI? (when it's created using ActivatorUtilities, it must be disposed of manually)

@lahma
Copy link
Member Author

lahma commented Apr 9, 2021 •
edited

Should (InnerJob as IDisposable)?.Dispose(); be behind a if to prevent double-disposal if the service comes from DI? (when it's created using ActivatorUtilities, it must be disposed of manually)

Good catch, added check for job's origin and only disposing if it came from ActivatorUtilities.

@ArturDorochowicz
Copy link
Contributor

Looks good.

ArturDorochowicz
ArturDorochowicz reviewed Apr 9, 2021
View reviewed changes
src/Quartz.Extensions.DependencyInjection/ServiceCollectionQuartzConfigurator.cs Outdated
@@ -61,15 +61,10 @@ public void UseMicrosoftDependencyInjectionJobFactory(Action<JobFactoryOptions>?
UseJobFactory<MicrosoftDependencyInjectionJobFactory>(configure);
}

[Obsolete("Jobs are always created with some")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean ... with scope?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, working in parallel with two bug fixes seems to freeze the brain. Pushed fix commit.

@lahma
Copy link
Member Author

lahma commented Apr 9, 2021

I think we have the pieces together to patch the DI resolution support.. @kevinchalet and @ArturDorochowicz are you happy with this so I can merge? Thank you both for helping out and spotting problems.

@ArturDorochowicz
Copy link
Contributor

@lahma I think it addresses all concerns. Ship it! :)
Thank you.

@lahma lahma merged commit d26d41e into main Apr 9, 2021
@lahma lahma deleted the job-registration branch April 9, 2021 15:54
@kevinchalet
Copy link
Contributor

@lahma looks awesome, thanks for fixing it! 👏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ArturDorochowicz ArturDorochowicz ArturDorochowicz left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Can no longer register jobs via DI
3 participants
@lahma @kevinchalet @ArturDorochowicz