Quartz exception after upgrade

NoahStahl commented 3 years ago

After upgrading to latest OpenIddict (3.0.3) and Quartz (3.3.1), seeing below runtime exception on startup.

This doesn't occur with 3.0.2 and Quartz 3.2.4. Rolling back is viable workaround. Happy to provide further details.

Configuration:

// Add Quartz.NET for recurring datastore pruning
services.AddQuartz(options =>
{
    options.UseMicrosoftDependencyInjectionJobFactory();
    options.UseSimpleTypeLoader();
    options.UseInMemoryStore();
});

// Register the Quartz.NET service and configure it to block shutdown until jobs are complete
services.AddQuartzHostedService(options => options.WaitForJobsToComplete = true);

...

services.AddOpenIddict()
    .AddCore(options =>
    {
        // Enable Quartz.NET integration
        options.UseQuartz(options =>
        {
            options.SetMinimumTokenLifespan(environment.IsDevelopment() ? TimeSpan.FromHours(1) : TimeSpan.FromHours(24));
        });
    })
...

Exception:

An error occurred instantiating job to be executed. job= 'OpenIddict.Quartz.OpenIddictQuartzJob, message=Multiple constructors accepting all given argument types have been found in type 'OpenIddict.Quartz.OpenIddictQuartzJob'. There should only be one applicable constructor.'
Quartz.SchedulerException: Problem instantiating type 'OpenIddict.Quartz.OpenIddictQuartzJob: Multiple constructors accepting all given argument types have been found in type 'OpenIddict.Quartz.OpenIddictQuartzJob'. There should only be one applicable constructor.'
 ---> System.InvalidOperationException: Multiple constructors accepting all given argument types have been found in type 'OpenIddict.Quartz.OpenIddictQuartzJob'. There should only be one applicable constructor.
   at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.TryFindMatchingConstructor(Type instanceType, Type[] argumentTypes, ConstructorInfo& matchingConstructor, Nullable`1[]& parameterMap)
   at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.FindApplicableConstructor(Type instanceType, Type[] argumentTypes, ConstructorInfo& matchingConstructor, Nullable`1[]& matchingParameterMap)
   at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.CreateFactory(Type instanceType, Type[] argumentTypes)
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
   at Quartz.Simpl.JobActivatorCache.CreateInstance(IServiceProvider serviceProvider, Type jobType)
   at Quartz.Simpl.PropertySettingJobFactory.NewJob(TriggerFiredBundle bundle, IScheduler scheduler)
   at Quartz.Core.JobRunShell.Initialize(QuartzScheduler sched, CancellationToken cancellationToken)
   --- End of inner exception stack trace --- [See nested exception: System.InvalidOperationException: Multiple constructors accepting all given argument types have been found in type 'OpenIddict.Quartz.OpenIddictQuartzJob'. There should only be one applicable constructor.
   at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.TryFindMatchingConstructor(Type instanceType, Type[] argumentTypes, ConstructorInfo& matchingConstructor, Nullable`1[]& parameterMap)
   at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.FindApplicableConstructor(Type instanceType, Type[] argumentTypes, ConstructorInfo& matchingConstructor, Nullable`1[]& matchingParameterMap)
   at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.CreateFactory(Type instanceType, Type[] argumentTypes)
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
   at Quartz.Simpl.JobActivatorCache.CreateInstance(IServiceProvider serviceProvider, Type jobType)
   at Quartz.Simpl.PropertySettingJobFactory.NewJob(TriggerFiredBundle bundle, IScheduler scheduler)
   at Quartz.Core.JobRunShell.Initialize(QuartzScheduler sched, CancellationToken cancellationToken)]
[10:58:38 INF] All triggers of Job OpenIddict.Quartz.OpenIddictQuartzJob set to ERROR state.

Further technical details

ASP.NET Core version: 5.0.5
Visual Studio 16.9

kevinchalet commented 3 years ago

@lahma hey Marko! Are you aware of a breaking change between Quartz.NET 3.2.x and 3.3.x that could explain this issue?

lahma commented 3 years ago

Hmm.. probably caused by this change . My gut feeling is that removing the default constructor should work here. The AllowDefaultConstructor was removed as it's now redundant. So I think that your extra check against using it by having the default constructor is now redundant, maybe?

kevinchalet commented 3 years ago

Hum, abandoning native DI support and moving to internal type factories is a quite massive change for a non-major version 😅 This also means that one will no longer be able to register the jobs in the DI container and control how they are instantiated.

lahma commented 3 years ago

Yes I was hoping this not being major as as described in the PR it's the thing that ASP.NET Core does for one. Couldn't see this impact that you are now facing, sorry for that 😞 The DI integration is a work in progress, but can't really hide behind that excuse.

If you have ideas how to circumvent the problem/allow the needed specialization please let me know, I can tweak Quartz.

kevinchalet commented 3 years ago

Couldn't see this impact that you are now facing, sorry for that 😞 The DI integration is a work in progress, but can't really hide behind that excuse.

Haha, no worries!

Yes I was hoping this not being major as as described in the PR it's the thing that ASP.NET Core does for one.

Well, I suspect things are a bit less consistent than the PR author thinks:

MVC controllers are indeed instantiated instead of being directly resolved from the DI, but MVC offers a controllers-as-services option to change that. I suspect this is mostly motivated by the fact the MVC folks didn't want to add a "controllers discovery" story to detect and register controllers.
When the authentication stack was redesigned in 2.0, we all concluded that registering the authentication handlers as services (so they can be directly resolved from the DI) was the best thing to do. ActivatorUtilities is still used in a single place (for EventsType), but it will likely be removed in .NET 6.0 and 7.0 to simplify things.

FWIW, I believe having native DI resolution support is an important thing. The double disposal issue discussed in the linked topic could have been fixed more easily by simply removing the job.Dispose() call (since the service lifetime is managed by the DI container and not by your activator).

That said, having the option to use ActivatorUtilities to avoid having to register jobs as services probably makes sense, so you may want to support both approaches (IMO, using DI by default makes more sense). In this case, you could have two activators: one using DI and one using the activator thing.

lahma commented 3 years ago

Thanks for the insights, again you have the technical details I don't, much appreciated. I've created an issue to track, hoping to tackle it soon'ish.

kevinchalet commented 3 years ago

Awesome, thanks!

/cc @ArturDorochowicz in case he'd like to share his thoughts.

ArturDorochowicz commented 3 years ago

Hi

I agree that ideally Quartz could support both approaches, without and with job registration.

I'm aware that MVC also supports controllers as services. Even if both version were to be supported in Quartz, I think that defaulting to ActivatorUtilities is better. It allows for untangling adding jobs from registering them in DI. It's a simpler case. And then if someone wants to create their own DI-services-based job factory, they can still easily do this and register jobs on their own.

Removing job.Dispose() works for double disposal, but for the CreateScope=false case (which I think is the default) you still have a resource leak - the container will keep all job instances.

I don't know this project and only had a quick glance on the job in question here, but it seems to me that there's no actual special job registration here. The trouble is caused by the default ctor that isn't supposed to be used because it immediately throws. IOW the ability to customize the job registration isn't actually needed here. This of course doesn't mean that in general this wouldn't be useful.

NoahStahl commented 3 years ago

I took the opportunity to refactor to a Timer-based IHostedService to do the pruning job hourly. @kevinchalet Given the evident possibility of Quartz integration as breaking change source, and the increasing prevalence of supply-chain attacks, any reason Quartz was used for the pruning job? It seems super flexible, but for this purpose possibly overkill and a risk.

kevinchalet commented 3 years ago

Removing job.Dispose() works for double disposal, but for the CreateScope=false case (which I think is the default) you still have a resource leak - the container will keep all job instances.

Not sure how you could have a leak in this case? Singletons are created only once and reused for every resolution whether you're in a scope or not (and basically, your PR broke that).

I don't know this project and only had a quick glance on the job in question here, but it seems to me that there's no actual special job registration here. The trouble is caused by the default ctor that isn't supposed to be used because it immediately throws. IOW the ability to customize the job registration isn't actually needed here. This of course doesn't mean that in general this wouldn't be useful.

Oh sure, my remark about customizing the service registration was for the general case. In OpenIddict we don't need that.

kevinchalet commented 3 years ago

@NoahStahl this was discussed at length here: https://github.com/openiddict/openiddict-core/issues/361. TL;DR Quartz.NET was by far the best option and its extensibility allows scenarios that couldn't be implemented in a trivial IHostedService (e.g web farm scenarios where you want to synchronize jobs execution).

@kevinchalet Given the evident possibility of Quartz integration as breaking change source, and the increasing prevalence of supply-chain attacks, any reason Quartz was used for the pruning job?

Well, if we didn't trust any third-party author, we wouldn't depend on anything and we'd have to continuously re-invent the wheel. It's been the argument used by MSFT for years and it's definitely not an approach I want to adopt.

@lahma would it be safer to revert the DI change before folks start depending on the new behavior and give you a bit more time to decide what's the best approach? (maybe something hybrid, that uses DI first and falls back to ActivatorUtilities if the job was not registered as a service?)

lahma commented 3 years ago

I'll try to create a hybrid approach this evening and if it flies, a new release shortly after. I'll probably will be pinging you two for a review.

ArturDorochowicz commented 3 years ago

Not sure how you could have a leak in this case? Singletons are created only once and reused for every resolution whether you're in a scope or not (and basically, your PR broke that).

Singleton? How come? Jobs are transient. They used to be registered as transient (https://github.com/quartznet/quartznet/pull/1127/files#diff-7becfbcfe9517821071524446ac6951d5bdfed6770b02979f2946306fbc3bb7fL194) and the docs mentioned transient as the expected thing (e.g. https://github.com/quartznet/quartznet/pull/1127/files#diff-2e3238e849dc98b1c91bb8359c40ae771ccc993d14bdf205ada5248c6197ff1bL62)

kevinchalet commented 3 years ago

@ArturDorochowicz my bad, I somehow assumed that they were registered as singletons by default.

Registering jobs as scoped services would address your concern, as they would be disposed of when releasing the scoped container.

lahma commented 3 years ago

Fortunately this turned out to be just a little tweak, https://github.com/quartznet/quartznet/pull/1159 . I've tested the change against openiddict code base and jobs are again happily instantiated with this change.

kevinchalet commented 3 years ago

@lahma this looks too simple to be true 😄

@ArturDorochowicz's concerns remain: if jobs are registered as transient services, they won't indeed be disposed until the root container is disposed, which is not ideal. And we still have the double-disposal issue as the ScopedJob class disposes the underlying job instance.

IMHO, the best approach here would be to remove the "non-scoped" support and always create a scope per job invocation: in this case, both transient and scoped jobs would be correctly disposed.

ArturDorochowicz commented 3 years ago

@ArturDorochowicz's concerns remain: if jobs are registered as transient services, they won't indeed be disposed until the root container is disposed, which is not ideal. And we still have the double-disposal issue as the ScopedJob class disposes the underlying job instance.

IMHO, the best approach here would be to remove the "non-scoped" support and always create a scope per job invocation: in this case, both transient and scoped jobs would be correctly disposed.

@lahma Yes, it seems that it restores the original double disposal and possible resource leak issue.

I agree that the CreateScope=false case should not be supported. It's problematic even disregarding the possible resource leak. Imagine you have a job depending on a deep graph of objects. If there's something with a scoped lifetime added deep in that graph, you will get an exception from DI when resolving the job even though nothing in the jobs code changed. This can be alleviated with appropriate tests, but not everyone will have tests for resolving jobs from the container.

ArturDorochowicz commented 3 years ago

Registering jobs as scoped services would address your concern, as they would be disposed of when releasing the scoped container.

@kevinchalet Yes, that's correct. To be precise, it's not about job lifetime (it logically should be transient) as much as it is about always creating a scope.

lahma commented 3 years ago

I've now removed the support to have CreateScope = false, everything is scoped now in latest commit.

lahma commented 3 years ago

@NoahStahl Quartz.NET 3.3.2 is now available on NuGet thanks to great input from both @kevinchalet and @ArturDorochowicz . Please try it out and hopefully your problems are gone now.

NoahStahl commented 3 years ago

@lahma Thanks Marko! Great response time. I actually refactored my particular project in the meantime to a different method of scheduling the task, so I won't be able to confirm fix. But feel free to close the issue regardless.

kevinchalet commented 3 years ago

@lahma I just gave it a try and it works like a charm! Thanks for fixing it so fast, much appreciated!

I'll open a separate thread to track future improvements (like removing the manual scope creation, since Quartz.NET now always creates a scope for us)

openiddict / openiddict-core

Quartz exception after upgrade #1243

Further technical details