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

Make NCronJob native AOT ready #54

Open linkdotnet opened 6 months ago

linkdotnet commented 6 months ago

Currently, NCronJob is not native AOT ready. That might be a less ideal choice for folks who run this library in the cloud and try to minimize the footprint.

There is a nice article from Microsoft itself: https://devblogs.microsoft.com/dotnet/creating-aot-compatible-libraries/

Basically what we want is:

  1. Add the analyzers:

    <PropertyGroup>
    <IsAotCompatible>true</IsAotCompatible>
    </PropertyGroup>
  2. Fix all errors

  3. Create a new sample with native aot and publish the app and check if it is still doing its thing.

pablopioli commented 3 months ago

In the past have added AOT support to a few apps so I checked what I could do to help.

There are 3 incompatible parts:

  1. The easy. In NCronJobOptionBuilder the code must pass the warning upstream to the user of the library with the attribute [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] The compiler will warn the client code if something is unsupported by AOT.

  2. In JobExecutor there is a call to ActivatorUtilities.CreateInstance this line will never be AOT compatible. It will need a breaking change, requiring all needed types to be registered in the IOC container. But is simple to do.

  3. In JobExecutor there is a call to MakeGenericType this is more complicated and will requiere a redesign to as how the notifications are handled.

Those are my two cents, and seeing this issue I thought on discussing it and see how can I help.

pablopioli commented 3 months ago

Just adding, the updated documentation says to use IsTrimmable instead of IsAotCompatible in libraries

https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/prepare-libraries-for-trimming?pivots=dotnet-8-0

linkdotnet commented 3 months ago

@pablopioli Interesting - I can't find where IsTrimmable is a replacement for IsAotCompatible - more or less IsTrimmable is a precursor for AOT.

In JobExecutor there is a call to ActivatorUtilities.CreateInstance

Yeah - this kind of a fallback or convenient method if you forgot to register a job. We could also just "fail" in those moments. @falvarez1 and I had some discussion around that topic.

In JobExecutor there is a call to MakeGenericType this is more complicated and will requiere a redesign to as how the notifications are handled.

Yes - could we "ignore" that somehow and tell the compiler that we never run into the issue that the type might be trimmed out at this point in time? If the user didn't register IJobNotificationHandler<> properly you can't get into the code.

pablopioli commented 3 months ago

@pablopioli Interesting - I can't find where IsTrimmable is a replacement for IsAotCompatible - more or less IsTrimmable is a precursor for AOT.

https://learn.microsoft.com/en-us/dotnet/core/deploying/native-aot

The important one for a library is the trim analyzer. The trim process is very agressive and if unsure it will default to remove the member. With properties there is not much of a problem, you use it or not. But for a library like this the constructors are very problematic. NCronJob could just not find any constructor at all.

The other analyzers are for single file deployment (that is a decision for the consuming app) and the AOT analyzer (if your library can support trimming the tooling will simply convert the bytecode to native code.)

In JobExecutor there is a call to ActivatorUtilities.CreateInstance

Yeah - this kind of a fallback or convenient method if you forgot to register a job. We could also just "fail" in those moments. @falvarez1 and I had some discussion around that topic.

Yes, it is more a design decision than a technical one.

In JobExecutor there is a call to MakeGenericType this is more complicated and will requiere a redesign to as how the notifications are handled.

Yes - could we "ignore" that somehow and tell the compiler that we never run into the issue that the type might be trimmed out at this point in time? If the user didn't register IJobNotificationHandler<> properly you can't get into the code.

No, you can't suppress trimming warning. You can ignore them if you are sure that it will not fail anyway. The compilation process will warn you but the program will run without problems. Unless you are treating warning as errors and then you are toasted.

linkdotnet commented 3 months ago

We can live with the current state. Given that this was just an idea and not a real "need", we can keep it in the backlog.

That said I appreciated your input a lot @pablopioli! Let me know if you have other ideas!

nulltoken commented 3 weeks ago

@linkdotnet Do we want to pave the way for this in v4?

As identified in #106, we could

Maybe add a .UseNCronJob() extension method that would harvest the JobRegistry
and ensure every described type has been previously registered.
Otherwise, throw an InvalidOperationException ("The following types haven't
been registered in the dependency injection container: ...").

And then drop the call to ActivatorUtilities.CreateInstance in JobExecutor.

linkdotnet commented 3 weeks ago

I have to check some issues and prs - I can vaguely remember why the code is there, but have to dig a bit deeper. basicslly we wanted to make it easier for users in certain scenarios