Because cron_semaphore is an unreliable piece of crap.

CommentFileSizeAuthor
#22 cron_lock_D6.patch1.77 KBcatch
#12 cron_locking_240_with_update.patch2.7 KBBerdir
#10 cron_locking_240.patch2.2 KBBerdir
#10 cron_locking_900.patch2.2 KBBerdir
#3 cron_locking.patch2.2 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

I though we did that already. Crap.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

For a moment I thought the HEAD install I was looking at was from before a patch went in, but http://api.drupal.org/api/function/drupal_cron_run/7 dashed my hopes.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Active » Needs review
FileSize
cron_locking.patch2.2 KB

Yes, I'm wondering too if this is going to work, seems almost too easy :) Maybe we want to lower the timeframe to something less than a whole hour?

Dries’s picture

Dries CreditAttribution: Dries commented

OK with me. (I thought we had a use issue about this already.)

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Yeah there is/was: #553600: Apply locking framework to cron runs.

That issue had never a patch however, so I marked it as a duplicate.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Maybe 15 minutes for the lock_acquire()? Even less? Patch looks good and I think we already have some cron tests.

Dries’s picture

Dries CreditAttribution: Dries commented 
+++ includes/common.inc
@@ -4575,27 +4575,15 @@ function drupal_cron_run() {
+  if (!lock_acquire('cron', 3600.0)) {

I agree that one hour is rather long and that 15 minutes should be plenty.

(It's odd that lock_acquire() takes a float.)

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Needs work

Currently, we set the time limit to only 240, is there any reason why the locking should have a different value? (not saying that we should change it to 240, but simply the same value...)

lock_acquire() can handle lock times which are less than 1s, that is why it takes a float.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Component: menu system » base system

Marked #146551: Reset cron_semaphore when cron is run manually in admin/reports/status

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
cron_locking_900.patch2.2 KB
cron_locking_240.patch2.2 KB

Ok, here are two patches. One with timeout of 900s and another with 240s.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Needs work

240s seems right. We should have a hook_update_N() to remove the cron_semaphore variable though, apart from that look RTBC to me.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
cron_locking_240_with_update.patch2.7 KB

There you go ;)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Reviewed & tested by the community
Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
Issue tags: +API change

I call this an API change because it changes how Drupal has always done business, affecting how people (or modules) may interact with the system.

Could you please summarize the impact of this patch on real Drupal users? (Besides "making life better").

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

This shouldn't make any difference whatsoever. Only possibility is if there's some kind of module for unlocking the cron semaphore, which wouldn't surprise me since it's so annoying, this wouldn't be needed since the lock has a time limit. It also (along with the queue system) makes some of the super cron etc. modules a bit less useful than they should be, but those exist to work around how terrible the existing cron system is / has been.

Eric_A’s picture

Eric_A CreditAttribution: Eric_A commented
Version: 7.x-dev » 6.x-dev

Pretty sure this bug is in 6.x, too ;-)

EDIT: This semaphore removing probably cannot be backported. So I should not have changed the version number on this issue, then? If it is portable, then this can be set to open, I guess.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Fixed » Patch (to be ported)
Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Version: 6.x-dev » 7.x-dev
Status: Patch (to be ported) » Closed (fixed)

Drupal 6 is API frozen. -1 here.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented

Currently, we set the time limit to only 240, is there any reason why the locking should have a different value?

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.

Given that, I wonder if 240 seconds is a little too short for the lock? I think I have seen legitimate cron runs last around that long. It's probably not a huge deal, though.

ikeigenwijs’s picture

ikeigenwijs CreditAttribution: ikeigenwijs commented

subscribe, having cron troubles

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
FileSize
cron_lock_D6.patch1.77 KB

I needed a backport of this for a client site, here it is.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Issue summary: View changes

Given that, I wonder if 240 seconds is a little too short for the lock? I think I have seen legitimate cron runs last around that long. It's probably not a huge deal, though.

Yep, it can be problematic though. I've seen a lot of concurrent cron runs piling up on a site because of this, see #2319177: Cron lock time limit is too short and does not prevent multiple, concurrent cron runs.