NCronJob-Dev
[V5] Drop the ability to call AddNCronJob multiple times
It's allowed to call AddNCronJob() multiple times.
To be effective, some consistency checks will require that all initial registrations are done. The proper place to run them would be through UseNCronJob.
Very good point!
It's allowed to call AddNCronJob() multiple times.
I would also keep it that way, it may seem redundant, but for folks that split their logic, this is a useful approach.
What advantage is there to requiring the end user to add more boilerplate code like .UseNCronJob()?
What advantage is there to requiring the end user to add more boilerplate code like
.UseNCronJob()?
That we can make certain checks we can't do in AddNCronJob.
That includes if we have duplicates or ambiguous configurations. We can't do them at the moment.
And that forces us to throw later in the process, which really isn't ideal.
Hmm, can you point me to where these checks happen. Is this something we can tackle with static analyzers (Roslyn Analyzers)?
Ok. Rather than throwing an exception on a second call to AddNCronJob() why not just ignore the subsequent calls and throw a warning to the user that only the first call is in effect? It's friendlier and doesn't force the end user to add more boilerplate, and the end result is the same, the user realizes his mistake and removes the second call.
If we really want to force a specific coding convention I think we can handle this with Roslyn Analyzers.
Now if there are other reasons for the UseNCronJob() like pipeline ordering etc. then that's perfectly fine
Sure, we could do this. I know at least one user who reported that he used multiple AddNCronJob calls in different places. (See #138)
Analyzers could be one way of tackling that. Initially, I didn't see any reason to speak against multiple AddNCronJob calls. Given that Add / Use is a very common pattern in ASP, I still don't necessarily see a problem. But I do understand that it increases complexity and maybe doesn't lead the consumer into a "pit of success".
Now if there are other reasons for the UseNCronJob() like pipeline ordering etc. then that's perfectly fine
There are indeed other reasons :wink:
It's not a matter of code style. It's rather about ensuring the configuration of NCronJob is sound and safe before it's deployed in Production (in a staging slot for instance) and preventing the release from happening if that's not the case.
NCronjob being a library, a user can completely abuse it.
If he/she wants to call AddNCronJob() multiple times, I don't see any real reason to only consider the first call. For instance, one can call .NET .AddAuthentication() multiple times. If he/she wants to use source generators to configure it, why not.
The intent of UseNCronJob is eventually to validate that NCronJob can process production data flow in a predefined way. Some weird patterns can be detected during the AddNCronJob calls. Others need a completely finalized registration.
And would that validation lead to detection of an unsafe/ambiguous configuration, I'd prefer the startup to fail (and thus a fatal crash) rather than allowing NCronJob to wrongly process data.
My two (opiniated) cents :wink:
I see he said "Unfortunately I have decentralized my setup, where different parts of my code can just add stuff to the IOC container, including registering a job." This is how I structure my projects as well so typically I add a plugin/modular mechanism that scans all the corresponding assemblies then registers the necessary plugins. For example my auto registration would look like this:
public class HostedServicesStartup : IServiceStartup
{
/// <summary>
/// Add and configure service registrations
/// </summary>
/// <param name="services">Collection of service descriptors</param>
/// <param name="configuration">Configuration root of the application</param>
public void ConfigureServices(IServiceCollection services, IConfiguration configuration)
{
services.AddCronJob<CleanupPasswordRequestJob>(p =>
p.WithCronExpression("35 0 * * *"));
}
}
Here AddCronJob determines if the main services are registered and if they are not then it registers them, otherwise it just sets up the cron job. These are sprinkled throughout my modular applications so nothing is really registered in the Program.cs.
The intent of UseNCronJob is eventually to validate that NCronJob can process production data flow in a predefined way. Some weird patterns can be detected during the AddNCronJob calls. Others need a completely finalized registration.
If the order of the registrations of NCronJob is determined by where the UseNCronJob() appears then features like RunAtStartup won't really "run at startup.". It'll run based on where in the pipeline it is situated.
If the order of the registrations of NCronJob is determined by where the UseNCronJob() appears
That part is unclear to me. To what "order" are you referring to?
It'll run based on where in the pipeline it is situated.
The important part is for it to run after the builder.Build() and before the app.StartAsync().
f the order of the registrations of NCronJob is determined by where the
UseNCronJob()appears then features like RunAtStartup won't really "run at startup.". It'll run based on where in the pipeline it is situated.
I feel that is something positive - because, as a developer, I want to control when certain things happen.
The convention is used for middleware pipeline, which inherently has an order to it. The assumption is that all middleware is order dependent. If UseNCronJob() is not order dependent then is poses the question of why do we need that.
I feel that is something positive - because, as a developer, I want to control when certain things happen.
That's fair, but it defeats the purpose of running at startup, basically it runs wherever you put UseNCronJob.
Fair enough - the naming might be wrong given that you can put it somewhere or never call UseNCronJob (we could throw an exception in that instance).
Today, as you can run BackgroundServices in parallel, you can’t rely that your startup jobs finished. Only everything inside the NCronJob ecosystem (which is a valid constraint).
For me it stands or falls with whether or not we allow multiple AddNCronJob. If not - there is not much benefit which we couldn’t handle inside a single AddNCronJob.
The intent of UseNCronJob is eventually to validate that NCronJob can process production data flow in a predefined way. Some weird patterns can be detected during the AddNCronJob calls. Others need a completely finalized registration.
There are two mechanisms for running jobs: as an orchestration of tasks (run job1 then run job2 if job1 succeeds, otherwise handle error by doing XYZ) and as Independent Jobs, single, standalone tasks executed separately. I do think the data flow is important to be deterministic, this will allow us to develop the durability of the library (i.e. save state, recover state etc.). It might help to explicitly differentiate these two execution modes within the API to improve clarity and usability. Either as a single non-dependent job or as an orchestration of jobs/tasks, where the orchestration follows a more structured pipeline approach.
Ultimately, with the planned dashboard and centralized management server, we'll need capabilities to observe, manage, create, and orchestrate jobs in real-time, enhancing visibility and operational control.
If we need to call UseNCronJob and placement order doesn't matter then we can leverage Microsoft.AspNetCore.Hosting.IStartupFilter and HostingStartupAttribute to ensure that the call is always made:
[assembly: HostingStartup(typeof(NCronJob.Startup.CronJobHostingStartup))]
namespace NCronJob.Startup
{
public class CronJobHostingStartup : IHostingStartup
{
public void Configure(IWebHostBuilder builder)
{
builder.ConfigureServices((context, services) =>
{
AddJobStartupFilters(services);
});
}
private IServiceCollection AddJobStartupFilters(IServiceCollection services)
{
services.AddTransient<IStartupFilter, CronJobStartupFilter>();
return services;
}
}
public class CronJobStartupFilter : IStartupFilter
{
public Action<IApplicationBuilder> Configure(Action<IApplicationBuilder> next)
{
return app =>
{
next(app);
if (ServiceRegistrationTracker.AreNCronJobServicesRegistered)
{
app.UseNCronJob();
}
else
{
var logger = app.ApplicationServices.GetRequiredService<ILogger<CronJobStartupFilter>>();
logger.LogError("Please call builder.Services.AddNCronJob() to register the NCronJob services");
}
};
}
}
}
I think my overall conclusion would be to remove the ability of calling AddNCronJob multiple times. Fair that we had one request, that used it more by accident than intentionally.
One call allows all the stuff we wanted without the ambiguity
I think my overall conclusion would be to remove the ability of calling AddNCronJob multiple times.
That would make things much simpler :wink:
Provided this happens, and we rewire the fluent interface so that it isn't in charge of adding jobs to the registry and the DI container, this would solve all the problems (I can think of) related to analysis of the jobs orchestration configuration.
The rough diagram of the execution of AddNCronJob would become
- Collect the JobDefinitions
- Ensure all dependency paths are unambiguous (and run additional checks: loop detection, ...)
- Otherwise throw.
- Add the JobDefinitions in the JobRegistry
- Register the typed Jobs in the DI Container
@nulltoken I think that is the right approach. Startup Jobs would, as it does today, run when we spin up the BackgroundService.
The only downside with this: Imagine you have a startup job that does DB seeding or migrations. As .NET offers to start jobs in parallel, other background services that use the DB might have to wait. This is a bit unfortunate.
The only downside with this: Imagine you have a startup job that does DB seeding or migrations. As .NET offers to start jobs in parallel, other background services that use the DB might have to wait. This is a bit unfortunate.
This could be controlled by an optional Attribute on the Job that designates order. But the whole .UseNCronJob() defeats the purpose of this. The idea of a startup job was to start it before any middleware. Once the middleware starts it very likely could need access to the DB.
The only downside with this: Imagine you have a startup job that does DB seeding or migrations. As .NET offers to start jobs in parallel, other background services that use the DB might have to wait. This is a bit unfortunate.
@linkdotnet I was only referring to the refactoring of AddNCronJob() implementation. I believe the lib should still offer a way to trigger the startup jobs before app start.
I think my overall conclusion would be to remove the ability of calling AddNCronJob multiple times.
That would make things much simpler 😉
Provided this happens, and we rewire the fluent interface so that it isn't in charge of adding jobs to the registry and the DI container, this would solve all the problems (I can think of) related to analysis of the jobs orchestration configuration.
The rough diagram of the execution of AddNCronJob would become
Collect the JobDefinitions
Ensure all dependency paths are unambiguous (and run additional checks: loop detection, ...)
- Otherwise throw.
Add the JobDefinitions in the JobRegistry
Register the typed Jobs in the DI Container
I'm also for removing the ability to call .AddNCronJob() more than once. However we should address the scenario where additional jobs may be registerred outside the conventional setup. This could be done with a hooks/interceptor mechanism.
I think my overall conclusion would be to remove the ability of calling AddNCronJob multiple times.
That would make things much simpler 😉
Provided this happens, and we rewire the fluent interface so that it isn't in charge of adding jobs to the registry and the DI container, this would solve all the problems (I can think of) related to analysis of the jobs orchestration configuration.
The rough diagram of the execution of AddNCronJob would become
* Collect the JobDefinitions * Ensure all dependency paths are unambiguous (and run additional checks: loop detection, ...) * Otherwise throw. * Add the JobDefinitions in the JobRegistry * Register the typed Jobs in the DI Container
Extracted in #247 as it could be handled in a non breaking way
I still would tackle that on v5 - because it might break folks that have multiple registrations. It might be API compatible but too much of a shift for a minor version