NCronJob-Dev / NCronJob

A Job Scheduler sitting on top of IHostedService in dotnet.
https://docs.ncronjob.dev/
MIT License
159 stars 11 forks source link

Optionally make startup jobs run early #111

Closed nulltoken closed 3 weeks ago

nulltoken commented 3 weeks ago

Is your feature request related to a problem? Please describe. When hosted in an aspnet.core app, some tasks may require to be done before the server is even ready to accepts requests (cache warming up, tire-kicking internal healthchecks, ...).

Currently, by default, the startup jobs seems to be triggered alongside Kestrel.

Given the following

using NCronJob;
using NCronJob.TestProgram;

var webApp = WebApplication.CreateBuilder();

webApp.Services.AddNCronJob(s => s.AddJob<TestJob>().RunAtStartup());

await using var run = webApp.Build();

run.MapGet("/", () => string.Empty);
await run.RunAsync();

public partial class Program;

....

public class TestJob : IJob
{
    public async Task RunAsync(IJobExecutionContext context, CancellationToken token){

        Console.WriteLine("** Here begins the startup job");
        await Task.Delay(TimeSpan.FromSeconds(10));
        Console.WriteLine("** Here ends the startup job");
    }
}

Here's the output

** Here begins the startup job
warn: Microsoft.AspNetCore.Server.Kestrel.Core.KestrelServer[8]
      The ASP.NET Core developer certificate is not trusted. For information about trusting the ASP.NET Core developer certificate, see https://aka.ms/aspnet/https-trust-dev-cert.
info: Microsoft.Hosting.Lifetime[14]
      Now listening on: https://localhost:60258
info: Microsoft.Hosting.Lifetime[14]
      Now listening on: http://localhost:60259
info: Microsoft.AspNetCore.Mvc.Infrastructure.DefaultActionDescriptorCollectionProvider[1]
      No action descriptors found. This may indicate an incorrectly configured application or missing application parts. To learn more, visit https://aka.ms/aspnet/mvc/app-parts
info: Microsoft.Hosting.Lifetime[0]
      Application started. Press Ctrl+C to shut down.
info: Microsoft.Hosting.Lifetime[0]
      Hosting environment: Development
info: Microsoft.Hosting.Lifetime[0]
      Content root path: D:\Code\NCronJob\tests\NCronJob.TestProgram
info: Microsoft.AspNetCore.Hosting.Diagnostics[1]
      Request starting HTTP/2 GET https://localhost:60258/ - - -
info: Microsoft.AspNetCore.Routing.EndpointMiddleware[0]
      Executing endpoint 'HTTP: GET /'
info: Microsoft.AspNetCore.Routing.EndpointMiddleware[1]
      Executed endpoint 'HTTP: GET /'
info: Microsoft.AspNetCore.Hosting.Diagnostics[2]
      Request finished HTTP/2 GET https://localhost:60258/ - 200 - text/plain;+charset=utf-8 89.9289ms
info: Microsoft.AspNetCore.Hosting.Diagnostics[1]
      Request starting HTTP/2 GET https://localhost:60258/favicon.ico - - -
info: Microsoft.AspNetCore.Hosting.Diagnostics[2]
      Request finished HTTP/2 GET https://localhost:60258/favicon.ico - 499 - - 2.7169ms
info: Microsoft.AspNetCore.Hosting.Diagnostics[16]
      Request reached the end of the middleware pipeline without being handled by application code. Request path: GET https://localhost:60258/favicon.ico, Response status code: 499
** Here ends the startup job

Describe the solution you'd like Maybe an optional parameter to RunAtStartup()? The requirement would be to ensure those jobs are finished before Kestrel spins up.

Wouldn't this be easy to make this auto-magically happen, I'd be fine with something like

run.MapGet("/", () => string.Empty);

await run.Services.GetRequiredService<IStartupJobsRunner>().GoBaby();

await run.RunAsync();
linkdotnet commented 3 weeks ago

Your observed behavior is absolutely correct. ASP.NET starts all IHostedService alongside app.Run() and therefore also all jobs and pipelines.

I am not sure if this is more of a documentation issue. You could have a singleton TaskCompletionSource that is set SetResult and you have a middleware that await s said TCS.

Edit: Everything outside can lead to other issues where stuff might not be initialized (even inside a IHostedService).

nulltoken commented 3 weeks ago

I am not sure if this is more of a documentation issue. You could have a singleton TaskCompletionSource that is set SetResult and you have a middleware that await s said TCS.

Sorry, I was unclear in my request.

I'm already used to run "things" in plain asp.net before the app is run and know how to achieve the required goal without NCronJob.

But as NCronJob seems to be task oriented and even offer a RunAtStartup() hook for jobs, I was wondering if trying to package this feature in the lib would make sense (Maybe RunBeforeStartup()?).

Would it be outside of the product scope of NCronJob, I'd totally understand it.

In that case, indeed a slight warning in the documentation might be helpful to the user (Maybe "In an asp.net context, the server may start serving requests before the Startup jobs are finished"?).

\ With regards to the doco, it currently states

NCronJob allows you to configure jobs to run at application startup.
This is useful for tasks that need to be executed immediately when the application starts, 
such as initial data loading, cleanup tasks, or other setup procedures.

When considering high traffic, zero-downtime business critical apps, I tend to prefer to deal with "initial data loading" or "other setup procedures" before starting to accept traffic. And potentially crash the app fatally would something goes sideways during those pre-run steps, so that it's never considered by the load-balancer.

Hence the feature request :wink: \

linkdotnet commented 3 weeks ago

Valid points overall. I do see your idea.

Would it be outside of the product scope of NCronJob, I'd totally understand it.

No absolutely not (if reasonable and understandable). I just would like to avoid that people are shooting themselves in the foot.

In that case, indeed a slight warning in the documentation might be helpful to the user (Maybe "In an asp.net context, the server may start serving requests before the Startup jobs are finished"?).

That is a good point for sure at the moment, independent of your idea. I do think it lacks clarity. I am also confident that the current approach isn't sufficient enough to support that idea. A single AddNCronJob will not do the magic here. Here app.UseNCronJob might came into play to support exactly that scenario.

nulltoken commented 3 weeks ago

Here app.UseNCronJob might came into play to support exactly that scenario.

Yep!

linkdotnet commented 3 weeks ago

I am absolutely fair if we move towards v4. I dont think it will be trivial to have no breaking change, this feature and a solid code base in place all at the same times

nulltoken commented 3 weeks ago

I am absolutely fair if we move towards v4. I dont think it will be trivial to have no breaking change, this feature and a solid code base in place all at the same times

Makes sense.

linkdotnet commented 3 weeks ago

Okay I fiddled a bit around and the simplest we can do right now is something like:

public static class NCronJobApplicationBuilderExtensions
{
    public static IApplicationBuilder UseNCronJob(this IApplicationBuilder app)
    {
        var jobManager = app.ApplicationServices.GetRequiredService<StartupJobManager>();
        jobManager.ProcessStartupJobs().GetAwaiter().GetResult();

        return app;
    }
}

While still a breaking change for v3 that might be the easiest to support the feature. We can also have options like if we block until all jobs are done.

My only concern at the moment is, that we are tieing NCronJob directly to ASP.NET (more or less). If you just want to have a Windows Service written in .NET (https://learn.microsoft.com/en-us/dotnet/core/extensions/windows-service) that might not work out of the box. Maybe if we sit on top of IHost, then we could at least support this use case. I guess what I am trying to say is, that I don't want to close the door for other options.

@falvarez1 do you have a strong opinion on this one?

nulltoken commented 3 weeks ago

While still a breaking change for v3 that might be the easiest to support the feature.

I think this should be tagged as v4, with a light migration guide.

My only concern at the moment is, that we are tieing NCronJob directly to ASP.NET (more or less).

Fair.

But as WebApplication is IHost, that would work as well.

Just one thing, shouldn't the code leverage ConfigureAwait(false)?

linkdotnet commented 3 weeks ago

I think this should be tagged as v4, with a light migration guide.

Exactly - I already tagged it for v4.

But as WebApplication is IHost, that would work as well. Yes - at least for those two cases. TO be fair that should cover probably > 90% of use cases where a IHostedService is involved. We only have to check for the signature:

public static IApplicationBuilder UseNCronJob(this IApplicationBuilder app)
// And maybe?
public static IHost UseNCronJob(this IHost app)

So that chaining feels still natural for folks.

Just one thing, shouldn't the code leverage ConfigureAwait(false)?

Probably yes. We might even offer both versions:

public static IApplicationBuilder UseNCronJob(this IApplicationBuilder app)
public static Task<IApplicationBuilder> UseNCronJobAsync(this IApplicationBuilder app)
linkdotnet commented 3 weeks ago

I opened the v4 branch - which basically tackles exactly your requested feature (basically the first commit on the v4 branch). Documentation and whatnot is missing at the moment. Just that you get an idea. @nulltoken

nulltoken commented 3 weeks ago

@linkdotnet Would you open a PR against that new branch (just for the sake of having the discussion near the related code)?

linkdotnet commented 3 weeks ago

Yes - for now against main should be fine

linkdotnet commented 3 weeks ago

https://github.com/NCronJob-Dev/NCronJob/pull/115

linkdotnet commented 3 weeks ago

Merged in v4 - while there is still some work, I would close the ticket @nulltoken Just reopen if you think there is something open.