Make cron time limit a variable

Peter H,

I will assume that you upgraded to 6.14, otherwise you may want to copy and paste what they have done there although I'm changing your issue to the new code which...

Hi Drupal Core guys,

I'm not too sure what that $time_limit variable comes from, but it is not being set anywhere. The result now is that cron.php cannot timeout since passing 0 (or NULL) to set_time_limit() is equivalent to turning off that feature altogether.

I'm just questioning the intend of the variable. Is is expected to be set and did some other code not make it in 6.14?! Just in case I checked the default.settings.php and it is not there (I actually grepped the whole environment and could not find anything defining the variable.)

I think that's a problem since there is a hard limit of 1h for the semaphore!

I would propose something like this just before the call:

  if (function_exists('set_time_limit')) {
    if (!$time_limit || $time_limit > 3500) {
      $time_limit = 3500;
    }
    else if ($time_limit < 240) {
      $time_limit = 240;
    }
    @set_time_limit($time_limit);
  }

Thank you.
Alexis Wilke

Edit: I actually found the "right" issue, I guess... although it looks like a big mess! 8-) #193383: set_time_limit: Centralize calls and prevent warnings and errors

P.S. 2 -- the $time_limit variable was removed and the value of 240 was put back in there (in CVS only for now). So this issue is still valid, although you may want to post in the other issue since that's where they are very much talking about the time limit for CRON.

@2, per hass's comment in #193383-129: set_time_limit: Centralize calls and prevent warnings and errors, the umlimited time limit issue was accidentally introduced in 6.14. The fix (reverting the time limit to 240s) will be in 6.15 and is currently available in 6.x-dev.

The 1h semaphore limit thing is shall we say a bit conservative...

Reverting the title of this issue. IMO it's unlikely that the figure of 240 will be increased, however what might be supported is a check to make sure that any higher time limit set in php.ini or .htaccess is not *reduced* to 240s. Then you can configure your own time limit without having to hack core.

gpk,

Agreed. Although if I wanted a longer time for CRON but not anything else, the 240 in Core will have to be changed each time I upgrade. Another way could be to offer a variable that one can set to a value of one's choice with maybe a limit of 30 minutes.

Thank you.
Alexis

I agree with AlexisWilke that it should be variable. Cron jobs with search_files module for instance, can take up a very a long time, but we might not want to raise the limit for all other processes in Drupal.

The problem still exists in D7.

This is not a bug.

Oh and d8

Here's a simple patch.

Looks good to me, the bot seems hung however. I'll mark this rtbc once it comes back green.

If that's a new variable, I suggest it to be configurable via admin/system/config/cron

I added the cron time limit as a new config to the system module. The patch is also extending the cron settings form.

Per default, the value is set to 240 as it was hardcoded before.

Patch from #11 breaks default values in cron config form:

Hi valthebad,

does the patch breaks your existing installation or does this error also occures on a fresh install?

There are many settings in core that have no UI. So lets leave the UI as a follow up and get the new functionality into D8.

#8: 547998-8.patch queued for re-testing.

#16: patch broke existing installation. Fresh install went smoothly

#11: system_Added-Cron-Time_Limit-547998-11.patch queued for re-testing.

Probably better to use a number field over the textfield (https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Render!Element!Nu...), suffix should be field_suffix:

  $form['cron']['cron_time_limit'] = array(
    '#type' => 'number',
    '#title' => t('Time limit'),
    '#description' => t('Set the cron execution time limit in seconds. If set to zero, no time limit is imposed.'),
    '#default_value' => config('system.cron')->get('threshold.time_limit'),
    '#min' => 0,
    '#step' => 1,
    '#field_suffix' => t('seconds'),
  );

This improves UX and there's no more need for a specific validation callback.

The patch needs a re-roll and breaks the patch from https://www.drupal.org/node/2319177:

   // Try to acquire cron lock.
-  if (!lock()->acquire('cron', 240.0)) {
+  if (!lock()->acquire('cron', $time_limit)) {

I suggest to make the lock relative to the cron timeout:

    if (!$this->lock->acquire('cron', $time_limit * 3.75)) {

Since cron settings are no longer in "core" but in the "automated_cron" module (see https://www.drupal.org/node/2507031) the form to edit global cron settings is in CronForm.

Attached a patch that will turn the time limit into a configurable variable.
Left out the form UI because there's a discussion going on about this in https://www.drupal.org/node/2581949.

Fixed test.

1. +++ b/core/lib/Drupal/Core/Cron.php
   @@ -111,12 +111,13 @@ public function run() {
   +    $time_limit = \Drupal::config('system.cron')->get('treshold.time_limit');
   

   Please inject the cron configuration

2. +++ b/core/lib/Drupal/Core/Cron.php
   @@ -111,12 +111,13 @@ public function run() {
   -    if (!$this->lock->acquire('cron', 900.0)) {
   +    if (!$this->lock->acquire('cron', $time_limit * 3.75)) {
   

   That number feels kinda arbitrary, I know its simply 900 / 240 but maybe a quick line of documentation would be nice

3. +++ b/core/modules/system/config/install/system.cron.yml
   @@ -1,3 +1,4 @@
      requirements_error: 1209600
   +  time_limit: 240
   diff --git a/core/modules/system/config/schema/system.schema.yml b/core/modules/system/config/schema/system.schema.yml
   

   Given that we add it, we need to write an hook_update_N() to include the number by default, and then some test for that as well.

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

wanted to bump 1 more time.