Skip to content

Cron: Use same date/time for each job #9411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

mjansenDatabay
Copy link
Contributor

@mjansenDatabay mjansenDatabay added bugfix php Pull requests that update Php code labels Apr 15, 2025
@mjansenDatabay mjansenDatabay self-assigned this Apr 15, 2025
@klees
Copy link
Member

klees commented Apr 16, 2025

Hi @mjansenDatabay,

please don't, this looks like a food gun that would be lying around or even already a bug to be introduced

Let me explain: The wording for that functionality ("date time provider") we would be modifying here suggests, that I can indeed get the current datetime by using the closure. Even though it is only used for checkSchedule in the class from the core, it is indeed available to be used for all derived classes as it is protected. So this could already be used for other purposes in some existing cron job. Even if it might not be used for other purposes now, the wording at some point will catch someone by surprise, as it strongly suggests that the function will yield a changing value, which would be a constant one after this change here.

Even more, I would strongly suggest that the original ticket is the result of a misconfiguration which even might not necessarily be fixed by this change. Let me elaborate: The job is scheduled to run every 5 minute. The cron.php is also be scheduled to be run every 5 minute. So, every 5 minutes we check, if the job shall run again. Let t_c be the moments when cron.php triggers. Then, depending on start up time of the script, and other cronjobs maybe, the job is actually triggered at t_j = t_c + e_1 where e_1 > 0 is the time until the cronjob actually runs. Let d_c be the scheduled delay for the cron.php and d_j be the scheduled delay for the job. Then t_j + d_j <= t_c + d_c is the check that shall determine if the job will run the next time. Then insert t_j from above t_c + e_1 + d_j <= t_c + d_c. Subtract t_c on both sides: e_1 + d_j <= d_c and finally set d_j = d_c as in the ticket and subtract again: e_1 <= 0. Before, the execution time e_1 was determined by the script startup time and the jobs executed before, after this e_1 will be the startup time (more specifically: the time until line 44 is reached). This might be 0 if measured in s or ms but even then it isn't necessarily be 0 (and never smaller then 0...) and the fix will fail again....

The general problem here is: With the given configuration, we are trying to approximate a dirac comb with another dirac comb of the same period, which will not work very well. Like a moiree effect in time... The correct solution would be to use a function of much higher frequency for the approximation, which means that the cron.php should e.g. be triggered every minute to allow to approximate cron jobs with 5 minute schedule with a sufficient accuracy.

Hope this makes some sense...

Kind regards!

@klees
Copy link
Member

klees commented Apr 16, 2025

Sorry, it somehow got me there 😁

@klees
Copy link
Member

klees commented Apr 16, 2025

And, on a second thought, something like 57s would be even better =)

@mjansenDatabay
Copy link
Contributor Author

mjansenDatabay commented Apr 16, 2025

I agree with your remarks, Richard. When thinking about the PR yesterday evening, I also came to the conclusion that this will not fix the actual issue.

Furthermore, we also discussed this internally this morning, and I came to the same conclusion as you:

The correct solution would be to use a function of much higher frequency for the approximation, which means that the cron.php should e.g. be triggered every minute to allow to approximate cron jobs with 5 minute schedule with a sufficient accuracy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix php Pull requests that update Php code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants