Repository navigation
[BUGFIX] taskhelper: don't let the execution time delay the next cron execution - #383
Merged
Merged
Conversation
… execution The cron runner computes the next date from the moment an execution starts (the value received from the timer), but then armed the next timer with `next.Sub(now)`, where `now` is still that start time. The timer therefore waited the full interval counted from the end of the execution, so every execution was delayed by the time the previous one took. With a task scheduled every 5 minutes that runs for 5 seconds, the task started at 18:45:00, then 18:50:05, then 18:55:10, and so on. The timer is now armed with `time.Until(next)`, the time actually left until the next date. If an execution lasts longer than that, the next date has already passed and the task is executed again right away, as `time.Ticker` does for NewTick. The NewCron documentation now describes this behavior, and TestCron_ExecutionTimeDoesNotDelayTheNextExecution checks the time between the start of two executions with a task shorter and a task longer than the schedule. Both cases fail without the fix. Fixes perses#285 Signed-off-by: NopeCharon <115794254+NopeCharon@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #285.
The cron runner computes the next date from the moment an execution starts (the time received from the timer), but it then armed the next timer with
next.Sub(now), wherenowis still that start time. So the timer waited the full interval counted from the end of the execution, and each execution was delayed by the time the previous one took. That's the behavior described in the issue: with a task every 5 minutes that runs for 5 seconds, it starts at 18:45:00, then 18:50:05, then 18:55:10, and so on.The timer is now armed with
time.Until(next), the time actually left until the next date. If an execution lasts longer than that, the next date has already passed and the task is executed again right away, the same waytime.Tickerbehaves forNewTick. The doc ofNewCronnow describes this.The change is limited to the line arming the timer, so it doesn't conflict with the guard on the zero date proposed in #382.
Validation
TestCron_ExecutionTimeDoesNotDelayTheNextExecutionwith@every 1s, which checks the time between the start of two executions:make test,golangci-lintv2.15.0 andmake checkformatpass.Checklist
[<catalog_entry>] <commit message>naming convention.