Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.As David Rothstein already realized in #805702-20: Cron should use locking framework:
As far as I understand, set_time_limit() only affects the actual PHP code execution time, which is not the same as the elapsed 'wall clock' time, which can be substantially different due to database queries, etc (see the note at http://php.net/manual/en/function.set-time-limit.php). So there is no particular reason why the lock should be held for only as long as the maximum script execution time.
Exactly. I've seen many concurrent cron runs piling up on a server, because of the search api server becoming unavailable. Search API tries to index during cron run and its connections waiting for response are not covered by the php time limit.
Reverting the lock time limit to a safe value of 3600s as it was before #805702: Cron should use locking framework seems to be reasonable and avoids multiple, concurrent cron runs.
Patches for d8 and d7 attached.
Comments
Comment #1
swentel CreditAttribution: swentel commentedrerolled, no reason to hold this up.
Comment #2
alexpott1 hour seems like a super long time - up from 4 minutes.
Comment #3
catchThis was discussed in #805702: Cron should use locking framework a bit, looks like we changed to 240s on purpose but I'd suggested 15 minutes before that - what about 15 minutes then? Agreed an hour seems much too long - that's enough for things to get backed up on a busy site.
Comment #4
swentel CreditAttribution: swentel commentedHmm right, I was somehow misreading this completely to 300 (which then would only be one minute more) - new patch to 15 minutes.
Comment #5
catchAlso not necessarily in this issue, but what about renewing the lock after each cron handler and before running queues?
Comment #6
BerdirSounds like we have an agreement on 15min then?
Doing it per hook/queue runner sounds interesting but yes, separate issue.
Comment #8
catchOpened #2340043: Re-aquire lock during cron runs.
Committed/pushed to 8.0.x, thanks!
Comment #9
fagoBackported it.
Comment #15
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedPatch looks good and it still applies to D7. Code is the same as in D9. Setting this as RTBC.
Comment #17
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedBut considering the state of D7, don't we want to make it configurable via settings.php? If we want as many sites as possible to benefit from this fix then the opt-out variant should be better in my opinion (who wants to keep the current lock time limit would be able to do so).
Not creating a new issue yet in case we will commit #9, but if not, I will create a D7 backport follow-up issue.
Comment #18
mcdruidYes, I think we should make this configurable in settings.php but would be happy to change the default to 15 mins to align with D9.
Comment #19
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedI am closing this as Fixed in D8 and created a follow-up for D7, so we can finish the new approach there.
#3308929: [D7] Cron lock time limit is too short and does not prevent multiple, concurrent cron runs