Cron Busy ReRuns: prevent already running cron.php from re-running

Can you just post a patch?
http://drupal.org/patch

First of all, +1 on correcting this bug.
I noticed this problem the same way as you did: by sending our newsletter to 1000+ emails. Each time I would run cron.php again would mean that the people would receive the newsletter one more time, i.e. many people received multiple copies of the newsletter.

What's more, cron.php can be run by anybody. There is never a guaranty that it won't be run twice, even if you know what you are doing: search engines may have found a link to cron.php somewhere and try to index the page at the same time as your scheduled run in taking place.

Thanks for providing test files, in your zip.

The code needs some very minor adjustments, though:
1) you insert some new lines where you shouldn't.
2) comments must be indented, too. (// Clean up).
3) remove your comment block, with your name and email: there is no mention of the author of any patch anywhere in the code, and I guess this patch will not get committed with it.
4) improper use of capitalization in the watchdog message.

Now, an important question:
What if because of a severe failure (server crash?) the cron is no longer running, but the variable cron_busy is still set as TRUE?
Maybe we need to add a function in admin/settings to allow to reset the variable as FALSE in cases like.

I attach a new patch.

In search_cron(), a shutdown function is registered which does some cleanup if the function fails or times out. Can this be done in cron.php? Seems like this would be the approriate way to set the watchdog error "cron run did not complete" as well as resetting the 'cron_busy' variable?

Interesting idea.

http://php.net/register_shutdown_function
the comments mention differences between *nix and windows platforms.
I haven't had time to study the question in details, though.

I'm concerned about cron runs which exceed the maximum execution time set by PHP.

I'm not sure, but registering a shutdown function still might not execute if the script exceeds any alotted time limit (especially since it can be an Apache-level time limit instead).

I think the best solution might be to add better locking to simplenews. (Or a queue API for Drupal so modules won't have to think about making a proper queue.)

I know from the search_cron() changes that register_shutdown_function() does allow you to get around PHP's time limit. Apache's limit, that I don't know.

While it would be lovely to be able to monitor/kill the processes started by cron, is there any cross-platform way to do so?

I can more readily imagine a new feature where cron is invoked from an admin page, with the option to only invoke the hook_cron implementations of a subset of the installed modules.

While it would be lovely to be able to monitor/kill the processes started by cron, is there any cross-platform way to do so?

I can more readily imagine a new feature where cron is invoked from an admin page, with the option to only invoke the hook_cron implementations of a subset of the installed modules.

Since this issue is not moving, maybe it would help to focus on the critical issue and keep new features separate.

So, it seem to me that the critical issue is simply preventing cron from running if another instance is in progress, and doing appropriate cleanup in case of timeout, crash, etc.

To make good progress on this, it seem as though it would be useful to be able to reproducibly cause these faults. Timout is probably the easiest by setting PHP's max_execution_time to something short on a test site.

Crash could perhaps be simulated by restarting apache?

This is a simple way to a) prevent multiple crons from running b) automatically expire the lock after a certain time. If you just use TRUE/FALSE and the code aborts then you are stuck. I am using a time based semaphor so after four minutes you can run cron.

Eh, that sounds clever. I like this idea.
the variable con_busy is no longer used, so maybe we need an update to remove it.

the function returns TRUE after the run. Do we need to return FALSE or 'busy' in the first part: if() { return 'busy'} ?

Code looks good. Two nits:

1. Add some code comments explaining that we can't run cron.php on top of each other and the side-effects it causes.

2. Maybe change variable_set('cron_start', 0) to variable_del('cron_start')?

is the 4 minute timeout delay long enough? See the original posting at the top:

Since the newsletter takes hours to send once the simplenews.module invokes cron.php to send the a newsletter to any subscribers it has not yet been sent to, our re-loading of cron.php restarted the whole sending process

Maybe the timeout delay needs to be settable somewhere (even if only in settings.php)?

About the time delay, look at the code just above what would be patched:

  if (!ini_get('safe_mode')) {
    set_time_limit(240);
  }

Either safe mode is on, in which case cron is probably only allocated 30 s. to do its job, or it's off, and it is allocated 4 min.
So, chx' s patch makes sense in this regard.

Having said that, I don't know what would happen if a newsletter takes more than 30 s. or more than 4 min. to send. But simplenews.module allows to configure the number of newsletters to send at each cron run, which I guess solves the problem.

From PHP.net: The set_time_limit() function and the configuration directive max_execution_time only affect the execution time of the script itself. Any time spent on activity that happens outside the execution of the script such as system calls using system(), stream operations, database queries, etc. is not included when determining the maximum time that the script has been running.

This means that assuming cron never runs for more than 4 minutes is false.

A proper solution is to use register_shutdown_function(). This allows us to always reset the "cron_start" variable when cron stops, for any reason. Modules should already be using this when the tasks they do can take too long (like node search indexing does). We can even check the value of "cron_start" in the shutdown function to see if the shutdown is graceful or not.

The only case where cron could then still lock itself out is when the database connection is lost in mid processing, so the variable cannot be reset. It's probably a good idea to build in a long maximum delay (e.g. an hour) after which cron is automatically unlocked.

Also, the watchdog message doesn't make sense at all, even if it was written under the assumption that cron never runs longer than 4 minutes:

+  if (variable_get('cron_start', FALSE) + 240 > time()) {
     watchdog('cron', t('Last cron run did not complete.'), WATCHDOG_WARNING);
   }

This message is only true if 4 minutes have not yet passed since the last invocation of cron.php. So the correct message would be "Attempting to re-run cron while it is still running."

thanks for clarifying this- from previous reading I was very wary of the magic 4 minute setting.

Here's a patch for HEAD which introduces the approach I outlined... Summary:

• We prevent cron from running multiple times. There is a time limit of 4 minutes per cron run, though this only counts actual PHP processing time, not waiting for DB, I/O, ...
• We gracefully shut down even if the time limit is hit (though we do inform the admin of this).
• We assume that any cron run that takes longer than an hour is in fact stuck (e.g. db connection dropped).

Looks okay on initial reading of the code. (This is not a full review.)

The patch looks good and seems to answer all the different concerns expressed above (shutdown function and proper handling in case of a crash).

My two points in #15 still stand, though.

beginner: I added an update to delete 'cron_busy'. The function does not need to return FALSE because null is equivalent to false for boolean evaluations (if).

Committed to HEAD as there were no other objections.

I get this error too , My Db size is > 1GB ,