HangfireIO / Cronos

A fully-featured .NET library for working with Cron expressions. Built with time zones in mind and intuitively handles daylight saving time transitions
MIT License
974 stars 114 forks source link

DST execution skipped #36

Closed mw911 closed 3 years ago

mw911 commented 3 years ago

Hi first of all I want to thank you for the great library. One thing doesn't seem to be working correctly. I am in Europe and DST was applied on 28/3/21 from 2 to 3 am CET which is one hour ahead of UTC. When I ask for the next occurrence 1 minute before DST it returns the right UTC timestamp. Doing the same at DST change it returns the next occurrence 1 week later. Dim expression As Cronos.CronExpression = Cronos.CronExpression.Parse("0 2 0") Dim nextOccurrence As DateTimeOffset = expression.GetNextOccurrence(start, timeZone, True) Return nextOccurrence.DateTime

log time - GetNextOccurrence - start 2021-03-28 01:59:01,812 - 2021-03-28T01:00:00.0000000 - 2021-03-28T00:59:00.0000000Z 2021-03-28 03:00:01,163 - 2021-04-04T00:00:00.0000000 - 2021-03-28T01:00:00.0000000Z

Am I doing something wrong here? Thanks. Martin

odinserj commented 3 years ago

Hi Martin, thank you for your report. Will investigate this next week, but looks weird.

aidmsu commented 3 years ago

Hi, @mw911 . Thank you for your report!

First of all, could you tell us more about your use case? Why did you choose GetNextOccurrence method overload with inclusive = true in a iteration process?

Let's consider using method:

DateTimeOffset? GetNextOccurence(DateTimeOffset fromUtc, TimeZoneInfo zone, bool inclusive)

It works with DateTimeOffset in specific time zone. There aren't types in .NET appropriate for local date and time in specific time zone, so we have to use a pair fromUtc and zone. In the world NodaTime with ZonedDateTime and LocalDateType types we would implement another method:

ZonedDateTime? GetNextOccurence(ZonedDateTime fromZoned, bool inclusive)

It would accept and return ZonedDateTime object which represents local date and time in specific time zone. It would be much clearer that the method operates with local time but not utc time. But we don't have such types so we convert fromUtc to fromZoned inside GetNextOccurence method before we start looking for next occurrence.

In your case we are dealing with transition to summer time. According to Cronos docs:

During the transition to Summer time, the clock is moved forward, for example the next minute after 01:59 AM is 03:00 AM. So any daily Cron expression that should match 02:30 AM, points to an invalid time. It doesn't exist, and can't be mapped to UTC.

Cronos adjusts the next occurrence to the next valid time in these cases.

GetNextOccurence looks for the first next occurrence after fromUtc satisfying specific cron expression, THEN if found occurrence points to invalid time Cronos ajusts occurrence to the next valid time.

Consider your case in details.

0 2 * * 0 - cron expression "Every Sunday at 02:00 am" CET - time zone with DST transition forward from 2021-03-28 01:59:59 +1:00 am to 2021-03-28 3:00:00 am +2:00. So [2:00; 3:00) is invalid time.

2021-03-28 00:59 UTC is 2021-03-28 01:59 +1:00 CET. Next appropriate occurrence after 2021-03-28 01:59 is 2021-03-28 02:00. But it's invalid time in CET so we ajusts time to next valid local time: 2021-03-28 03:00 +2:00 CET

2021-03-28 01:00 UTC is 2021-03-28 03:00 +2:00 CET (as 2021-03-28 02:00 +1:00 CET does not exist). Next appropriate occurrence AFTER 2021-03-28 03:00 is 2021-04-04 02:00 (at 02:00 next sunday). So returned value will be 2021-04-04 02:00 +2:00 CET

mw911 commented 3 years ago

Hi Andrej

really many thanks for looking into it and coming back to me including such a detailed explanation+questions.

My app (running on a server) executes scheduled tasks. Every minute it uses the current time (truncated to the minute) - let's name it Event time - and compares it with the next occurence of each task - therefore with include=true. It is not an iteration process. These tasks could also use a "simple" schedule which I recently re-wrote based on NodaTime due to the extact same issue I observed with DST (and other problems with my algorithm).

I was able to follow your points and that the behavior I observed is indeed in line with the Cronos docs. I just have thought, that it is strange, that I get the expected result when doing a "forecast" on the minute right before DST but not on DST. Unfortunately Cronos doesn't support NodaTime ... but now I am thinking of deducting 1 second from the Tick time above and use include=false instead. Not sure if that will work and/or introduces other side effects. I already have ZonedDateTime available for the "simple" schedule and that would return the right time in UTC when I deduct 1 second.

Accordingly, it looks like I have found no error but it could not be solved otherwise without NodaTime. I will check next week and let you know the outcome of my planned change.

Best regards Martin

odinserj commented 3 years ago

that I get the expected result when doing a "forecast" on the minute right before DST but not on DST

The problem is that the 2021-03-28T01:00:00.0000000Z instant in UTC is ambiguous and can be mapped to the following local time:

So both behaviours are expected in different conditions, but mutually exclusive – we need to pick one mapping. And in my opinion it's much better to document that any iterations should be performed with inclusive=false due to this corner case. Is it possible to modify your implementation in this way?

mw911 commented 3 years ago

In the meantime it all makes sense and as I said, this was forcing me to re-write the logic for the "simple" schedule using NodaTime. Due to having that foundation, I was able to change the relevant line (simplified) from

GetNextOccurrence(Date.UtcNow.Truncate(TimeSpan.FromMinutes(1)), timeZone, True)

to (_now is NodaTime.ZonedDateTime truncated to the current minute)

GetNextOccurrence(_now.PlusSeconds(-1).ToDateTimeUtc, timeZone, False)

and it is working fine for both DST cases.

Many thanks for your help with this and I hope that these comments might help others in a similar situation.

Best regards Martin

rogerfar commented 2 years ago

@odinserj Sorry to bring this up again, but I think there is actually still an issue here.

I have this code: image

As you can see I'm passing a locally formatted DateTimeOffset: 2021-08-15 5:18:00 PM -07:00.

I explicitly retrieve my current timezone, MST, then I parse the cron (which is 0 * * * *) and then call GetNextOccurrence with that timezone and Include true.

The result is though: 2021-08-15 7:00:00 PM -06:00.

Which is wrong because it's supposed to 6PM instead of 7PM, and it's reporting -06:00 instead of -07:00.

When I go back in time to like March this year, it does work correctly. It seems that DST isn't regarded when using local times.

rogerfar commented 2 years ago

And ignore my remarks! Turns out I was constructing my dateTime input object incorrect making it shift 1 hour over.

I was doing

var now = DateTimeOffset.Now;
now = new DateTimeOffset(now.Year, now.Month, now.Day, now.Hour, now.Minute, 0, TimeZoneInfo.Local.BaseUtcOffset);

Instead of

var now = DateTimeOffset.Now;
now = new DateTimeOffset(now.Year, now.Month, now.Day, now.Hour, now.Minute, 0, TimeZoneInfo.Local.GetUtcOffset(now));