Time zone support

[falvarez1]

trafficstars

Overview

This Pull Request introduces timezone support to the CronScheduler component of our system. Previously, job scheduling was based solely on the server's local time, which could lead to discrepancies in job execution times, especially in systems deployed across multiple time zones or serving international users.

Motivation

The necessity for timezone support arises from the need to schedule and execute jobs according to various regional times, accommodating users or operations in different time zones. This enhancement ensures that NCronJob is robust, flexible, and more aligned with global use cases.

Changes Made

1. RegistryEntry Modification:

• [x] Added a TimeZoneInfo property to RegistryEntry. This allows each job to specify the timezone in which its cron schedule should be evaluated.

2. Cron Expression Evaluation:

• [x] Modified the ScheduleJob method to compute the next run time of a job based on the timezone specified in its RegistryEntry. This utilizes the GetNextOccurrence method from the Cronos library, which supports timezone adjustments.

3. Job Scheduling and Execution:

• [x] Adjusted the job scheduling logic to use DateTimeOffset instead of DateTime, providing better support for time zones.
• [x] Ensured that the calculated next run time considers the job's specified timezone, converting UTC times to the appropriate local times as needed.

Impact

• Reliability: Jobs can now be scheduled according to their designated timezones, increasing the reliability of time-sensitive job executions.
• Usability: By supporting multiple timezones, the system can cater to a wider range of users and use cases, enhancing its overall usability.

[linkdotnet]

I will have a look after #21 if it fine with you

[falvarez1]

I will need to make some modifications to this code once #21 is merged because of the updates to CronScheduler.

[falvarez1]

I resolved the merge conflicts, modified the README and added tests. I'm not sure how you want the CHANGELOG.md handled, it doesn't have a version for the last updates, just marked as unreleased.

[linkdotnet]

Maybe I should have explained that earlier a bit more in detail or create a CONTRIBUTING.md.

The idea behind the CHANGELOG.md is basically only adding in the Unreleased section. When I create a release via a GitHub Action, this information will be taken and put into the release page information and (currently buggy) put into the csproj file so that you can see the changes also on NuGet.

The Action will create the version tag etc, so there is no need to enter/know upfront the version information. Once it's released the GitHub action will make a new commit where it moves everything from Unreleased to the newly created version section in the CHANGELOG.md.

But there is a small detail: The workflow distinguishes between stable releases and previews. If it is a preview, then this process will not kick in.

[linkdotnet]

There seems to be a deadlock (GitHub runner timed out after 6h)

[falvarez1]

There seems to be a deadlock (GitHub runner timed out after 6h)

Indeed there is, I must've missed running the retry tests. Ironically that's the deadlock I was fixing with #30 Once #30 is merged I can pull that in and try again, but trying to fix this here may be redundant.

I'll merge and test locally.

[falvarez1]

This is a weird one. It works fine using Visual Studio and Jetbrains IDE testing mechanisms. Also works fine if I target the individual tests that are failing using dotnet test --filter "FullyQualifiedName=NCronJob.Tests.NCronJobRetryTests.JobShouldFailAfterAllRetries" for example. If I step through the test everything completes just fine. It only seems to deadlock if I run all of the tests using dotnet test, although the test itself is marked as Passed. Sounds like a race condition, probably with disposing objects. This is after I merged #30 locally, meaning it doesn't change the behavior 🤔.

[linkdotnet]

This is a weird one. It works fine using Visual Studio and Jetbrains IDE testing mechanisms. Also works fine if I target the individual tests that are failing using dotnet test --filter "FullyQualifiedName=NCronJob.Tests.NCronJobRetryTests.JobShouldFailAfterAllRetries" for example. If I step through the test everything completes just fine. It only seems to deadlock if I run all of the tests using dotnet test, although the test itself is marked as Passed. Sounds like a race condition, probably with disposing objects. This is after I merged #30 locally, meaning it doesn't change the behavior 🤔.

It indeed sounds like Dispose. For me EachJobRunHasItsOwnScope is deadlocking inside the while-loop.

[linkdotnet]

On another side note: GitHub runners are only single threaded (v)CPU's IIRC. So this could also influence local VS GH Runner

[linkdotnet]

It indeed sounds like Dispose. For me EachJobRunHasItsOwnScope is deadlocking inside the while-loop.

The test in question has the following definition:

WithCronExpression("* * * * *").And.WithCronExpression("* * * * *")

On main that leads to two entries in the jobQueue - on the future branch it doesn't (only 1 entry).

Edit 1: registry.GetAllCronJobs() yields only one element instead of two.

[linkdotnet]

Okay I found the culprit -.- and I am not sure why it did work on main:

cronJobs = jobs.Where(c => c.CronExpression is not null).ToFrozenSet();

As we are using a record, two equal definitions of a CRON job will of course only result in one element in a set. So probably we want a ImmutableArray<RegistryEntry> here.

Edit: public sealed class CrontabSchedule from NCrontab and public sealed class CronExpression: IEquatable<CronExpression> from CRONOS.

So yes - before two CRON Expressions were always different where as with CRONOS they can be the same.

[falvarez1]

Oh wow I just found the same thing and came here... I was chasing a red herring thinking the issue was with Retry tests.

[falvarez1]

I'd argue that we don't want two separate but identical Schedule entries for the same Job. If the point is to run that same Job twice the concurrency control will help here. But I don't think we should allow the same schedule to be created more than once per Job per service.

[linkdotnet]

Well that depends - if you want to run multiple jobs at the same time with different parameters I do think that is totally fine and valid.

For the sake of simplicity I generalized that - so if you explicitly add the same cron expression twice you can run it twice.

For sure we could also turn it around and have the schedule as the principal and the job as dependent so that there is a 1 to n relation ship between schedule and job. This would only allow one run in this case.

[falvarez1]

Well that depends - if you want to run multiple jobs at the same time with different parameters I do think that is totally fine and valid.

Yes that is valid, but wouldn't storing as a record and pass different parameters determine that they are two separate records and then add them to the FrozenSet just fine?

Anyhow I converted to ImmutableArray and the test passes

    private readonly JobExecutor jobExecutor;
    private readonly ILogger<CronRegistry> logger;
    private readonly ImmutableArray<RegistryEntry> cronJobs;

    public CronRegistry(IEnumerable<RegistryEntry> jobs,
        JobExecutor jobExecutor,
        ILogger<CronRegistry> logger)
    {
        this.jobExecutor = jobExecutor;
        this.logger = logger;
        cronJobs = [..jobs.Where(c => c.CronExpression is not null)];
    }

If you prefer we can decide if this is the correct approach in a separate Issue.

[falvarez1]

FYI the test also passes if you keep the FrozenSet and use different parameters like so:

ServiceCollection.AddNCronJob(n => n.AddJob<ScopedServiceJob>(
    p => p.WithCronExpression("* * * * *").WithParameter("First")
        .And
        .WithCronExpression("* * * * *").WithParameter("Second")));

[linkdotnet]

Yeah - let's discuss the semantics around this in a separate issue.

It triggered the tests again - they weren't stuck, but some seemed to fail. I will have to check this the upcoming days and change something in the Actions, because .net9.0 interferes with the output so that we can't see the failing test.

[linkdotnet]

LGTM so far - I guess only a rebase / merge is one open thing.

[linkdotnet]

@falvarez1 I took the liberty to update the branch for you.

That said I will squash and merge that thing and will create some documentation.