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
1.01k stars 118 forks source link

feature request: GetPriorOccurrence #24

Open mcshaz opened 4 years ago

mcshaz commented 4 years ago

Would you please consider returning the return the DateTime when a task would have been due going back in time.

I am currently using the real hack

_expression.GetOccurrences(scheduled.AddDays(-60), scheduled, _timeZoneInfo, false, false).Last();
odinserj commented 3 years ago

Please describe the idea behind this feature – we need to understand when it's required, because usually we are iterating forward.

atiris commented 3 years ago

I have 2 use cases for you.

In my case I'd like to show the user a percentage countdown or slider of how much time is left until the next run. Previous run is not stored anywhere (and of course at the first occurrence it is not possible to do otherwise than with such a function).

So, when entering an expression, for example, 0 10 * if it is 18:00, the user would visually see that the length of the last interval is 24 hours and 33% of it has already elapsed. I can't do this if I don't have a previous date (I never have it on the first occurrence).

I've actually tried different options (here and in NCrontab) but if there is no internal function that returns GetLastOccurrence or GetLastOccurrences (number) or GetPriorOccurrence or GetPreviousOccurrence, then calculating the previous value will be really difficult. I'm even willing to go to the first cron library that will allow me to do this 😄

Another thing is that I have data in the log when my process actually started. I would like to compare the last 5 planned launches against the actual launches. If I can't tell when the last N scheduled occurrences were according to the given expression, I can only say that it started then. But I don't know if it should have started then.

celluj34 commented 3 years ago

This sounds like a domain issue to me - what happens if the process is interrupted, so it didn't complete successfully, or the process never stared? Is it still "correct" to display the last occurrence even if it didn't happen? IMO I would store the last run in a database and show that instead of trying to backwards-calculate when something could have happened.

atiris commented 3 years ago

@celluj34 If the process is interrupted, then nothing bad happens 😸 - user still visually shows that last planned launch was scheduled for time X, the last actually triggered run (which of course is recorded in the database) was recorded at time Y and the next run is planned in time Z.
How please, without this functionality, do you want to display that the scheduled startup (either in the event of an outage or in a situation where the actual startup is shifted from the schedule) has been set for some other time?

Yes, sure, I could also store all scheduled launches in the database, and whenever the user changes the expression all data about plans in the future are deleted, rescheduled and saved again, but it seems to me like a useless work to show the user one date (the last scheduled start time).
As you mention, the case when the process starts for the first time (and you do not have data on the previous start) this date allows you to display the "length of one run" in the timeline .. without knowing the previous scheduled time, I can not visually show the user what is the last scheduled length between launches. if you want, I'm sure you can imagine it 🌈

I'm not saying this can't be simulated, but why is it such a problem (even if everyone who requires this has a fundamental bug in the application logic) to provide this functionality?

ghost commented 3 years ago

I had a need for this using the same cron expression

Would you please consider returning the return the DateTime when a task would have been due going back in time.

I am currently using the real hack

_expression.GetOccurrences(scheduled.AddDays(-60), scheduled, _timeZoneInfo, false, false).Last();

I've been exploring something similar. I think it's out of scope of this library, but if you're looking for some code that evaluates an actual cron expression and returns the previous expected execution time, you can use this. May be a little ugly, but I prefer it over just subtracting 60 days.

static TimeSpan CalculateNextRunTime(DateTime dateTime, string cronExpression, CronFormat? format)
{
    CronExpression expression = CronExpression.Parse(cronExpression, format == null ? CronFormat.IncludeSeconds : CronFormat.Standard);
    DateTime? nextUtc = expression.GetNextOccurrence(dateTime);
    return nextUtc.Value.Subtract(dateTime);
}

static DateTime CalculatePrevRunTime(string cronExpression, CronFormat? format)
{
    CronExpression expression = CronExpression.Parse(cronExpression, format == null ? CronFormat.IncludeSeconds : CronFormat.Standard);
    DateTime date = expression.GetOccurrences(DateTime.UtcNow, DateTime.UtcNow.Add(CalculateNextRunTimeFromNow(cronExpression, null) * 2), true, true).First();
    DateTime lastExpectedRunTime = date.Subtract(CalculateNextRunTime(date,cronExpression, null));

    return lastExpectedRunTime;
}

Example Usage:

string cronExp = "*/5 * * * * *";
Console.WriteLine(CalculatePrevRunTime(cronExp, null) + " Previous time");
Console.WriteLine(DateTime.Now.ToString() + " Now");
Console.WriteLine(DateTime.Now.Add(CalculateNextRunTimeFromNow(cronExp, null)) + " Next Run Time");
odinserj commented 3 years ago

Thanks everyone for your considerations and detailed descriptions. I've also re-checked other implementation like CronExpression in Quartz.NET (it has GetTimeBefore method but it's not implemented) and NCrontab and together with my prior knowledge of different Cron expression-related packages I don't know any of them which implement the GerPriorOccurrence feature.

We've written Cronos because there was no Cron-related package in .NET (and looks like in other programming languages too, because I've found nothing) which supports GetNextOccurrence with time zones. The only proper support for time zones was implemented in Linux's native Vixie Cron program, but it is unable to calculate next occurrences, it has only bool IsMatch(DateTime) method that return true or false for the specific date/time only.

And it looks like that GetPriorOccurrence method requires to re-implement everything in Cronos and then refactor everything to avoid any possible duplicates between GetPriorOccurrence and GetNextOccurrence methods. This is a really huge work that also requires ton of new unit tests to ensure everything is working correctly.

So I think we've finished innovating with Cronos and will step back and let others to implement this feature. Hope for understanding.

RZeni commented 2 years ago

This was the workaround that worked best for me. In my case a default of 1 month ago was sufficient, a more general use case probably wants a null default.

`

public DateTime GetPreviousOccurence(string cronString)
{
    var expression = CronExpression.Parse(cronString);
    var nextCron = expression.GetNextOccurrence(DateTime.UtcNow);

    if (nextCron == null) return DateTime.Now.AddMonths(-1);
    var upperBound = expression.GetNextOccurrence(nextCron.Value);

    if (upperBound == null) return DateTime.Now.AddMonths(-1);
    var difference = upperBound.Value.Subtract(nextCron.Value);

    return DateTime.Now.Subtract(difference);
}

`

jbennink commented 1 year ago

@RZeni Won't this give the wrong result if not all schedules have the same distance. Ie 0 10 * * 1-5 Would on a Friday at 08:00 give Friday 10:00 as nextCron. But upperBound would be the next Monday at 10:00. When you subtract the resulting 3 days difference from Now (or better nextCron you do not arrive at the correct date but at Tuesday, which is wrong since the correct value in this case would have been Thursday 10:00.

This illustrates that a solution is not trivial and the hack solution @mcshaz already mentions is not that bad. You just have to account for schedules that might only run once a year so probably use schedules.AddYears(-2) just to be safe.

I also agree with @celluj34 that this feels like a domain issue and should not be part of a generic cronexpression library.