Environment::setTimeLimit() ignores current value, can reduce timeout value

FYI, this is not the case in D6 anymore :
http://drupal.org/node/144337

Doesn't mean we cannot fix this in D5, though

For the record, this is node.module line 2953 at time of writing.

We are not going to backport the whole batch API to Drupal 5, but patches that may be accepted include:
- Make this use a progress bar without batch API
- Only raise the limit to 240, never lower
- Remove the time limit

I pasted a solution for B in chat yesterday.

if ((int)ini_get('max_execution_time') < 240) {

This *should* be portable, right?

Here's a patch for 6.x/7.x. Since ini_get() can return an empty string in very different circumstances, this patch makes sure we get a sensible number before attempting anything. (Otherwise an undefined max_execution time might get reset to 240.)

And one for 5.x which I've tested. Obviously absence of batch API in 5.x makes this one more useful since the more extensive "if" expression is that much more likely to be evaluated.

Oh my, the 6.x patch at #5 has a typo. How did that happen?!

Anyway, also need to check for time limit <> 0 since 0 means unlimited.

Patch is corrected. Missing one opening parenthesis '(' after 'if'.

- 1st condition has unnecessary braces
- "certain" in the comment should be removed
- 0 is a valid value for $time_limit, means unlimited, and is not checked here

According to #9 ...

Simple reroll

Test failure : #335122: Test clean HEAD after every commit

Go testbot!

There is a patch over at #193383: set_time_limit: Centralize calls and prevent warnings and errors for 7.x that does the same as #14/#17 plus a bit more.

@gpk: absolutely right, marking this one as duplicate.

For the record, although patches have been committed to 6.x/7.x from #193383: set_time_limit: Centralize calls and prevent warnings and errors, these have not fixed the original issue described here. It is probably not material for 6.x/7.x because the node access table is usually rebuilt using the batch API, but still could be a problem for 5.x. Feel free to reopen if it is...

And apologies for interfering in this issue and trying to combine it with the other one :-P all my half-baked "improvements" really did was to delay the finding of a fix...!!!

there are other modules or files calling set_time_limit(240)
Drupal 6.19
includes/common.inc line 2720
includes/locale.inc line 1042
modules/node/node.module line 2335

sites/all/modules/devel/devel_generate.inc line 5 (if you have devel module installed)
sites/all/modules/xmlsitemap/xmlsitemap.generate.inc (if you have xmlsitemap module installed)

#12: time_limit_174617_7.x.patch queued for re-testing.

"It is probably not material for 6.x/7.x because the node access table is usually rebuilt using the batch API, but still could be a problem for 5.x. Feel free to reopen if it is..."

5.x is dead :) so this is a won't fix and moving to 5.x

I cna live with everything ctach decides :)

I would normally not dare open an issue set as duplicate by catch, but I'm doing an exception now. I hope no one is offended that I opened this old old issue.

Posted by gpk on September 23, 2009 at 2:36pm
For the record, although patches have been committed to 6.x/7.x from #193383: set_time_limit: Centralize calls and prevent warnings and errors, these have not fixed the original issue described here. It is probably not material for 6.x/7.x because the node access table is usually rebuilt using the batch API, but still could be a problem for 5.x. Feel free to reopen if it is...

Indeed, the other issue did not solve this one. The hardcoded 240s timeouts comment clearly states that the intention is to raise the limit:
// Try to allocate enough time to rebuild node grants

Instead, it is likely to decrease the limit - especially if the permission rebuild has been initiated via drush, which by default has no time limit at all (php-cli default).

We encountered the problem when we saw timeouts by running the rebuild via the normal batch first, but got another timeout (actually 502 from Nginx/php). We then tried to run it directly via drush, and then encountered the 240s limit:
drush php-eval 'node_access_rebuild();'

The same issue is still there, not only in D7, but also D5, D6 and D8 (at least based on a glimpse on the code).

As this issue has been there for 7 years, I dont have high hopes for getting it in core. I'm posting this now primarily for the reason that I can share a new version of swendel's minor patch at #12. His version did not apply to the latest Drupal 7.26, as it has changed. I'm going to use the patch for our site.make.

@bibo: This entire issue is actually related to a more problematic area which is drupal_set_time_limit(). I started an issue here: https://drupal.org/node/2212033. The valid patch only checks the the current site time is 0 then set_time_limit() is not invoked; one option would be to check if the actual server's set max_execution_time is greater than that attempting to be set.

I started an issue here: https://drupal.org/node/2212033. The valid patch only checks the the current site time is 0 then set_time_limit() is not invoked; one option would be to check if the actual server's set max_execution_time is greater than that attempting to be set.

Thanks for your response. I agree that drupal_set_time_limit() is likely the best place to fix it "globally". I even thought about changing it there in my patch, but then again as it is indeed has a more "global" effect, it could be interpreted as equal to an API change.. which then again would probably go to D8, and less chance of being fixed in D7. I thought that maybe a fix limited to node_access_rebuild(); would have less impact and more chance of getting in core.

Apart from that the placement, it seems the effect of the patches is otherwise identical:

 function drupal_set_time_limit($time_limit) {
   if (function_exists('set_time_limit')) {
-    @set_time_limit($time_limit);
+    $current_time_limit = @ini_get('max_execution_time');
+    if (FALSE !== $current_time_limit) {
+      $current_time_limit = (int) $current_time_limit;
+      if (0 < $current_time_limit) {
+        @set_time_limit($time_limit);
+      }
+    }

vs this oneliner:
if (!ini_get('safe_mode') && is_numeric($time_limit = ini_get('max_execution_time')) && $time_limit > 0 && $time_limit < 240) {

.. one option would be to check if the actual server's set max_execution_time is greater than that attempting to be set.

As you probably noticed, the one liner includes that logic also. And doesnt mess if safe_mode is active. Changing the set_limit only if it is not already "unlimited" OR greater than 240 seems logical.

Anyway, I hope either this issue or #2212033: Drupal's drupal_set_time_limit may lower or enforce a time limit gets addressed sooner or later :)

There is a similar issue for D6: #1098382: node_access_rebuild hardcode 240 seconds limit

I would however vote to solve it via in a more generic way via #2212033: Drupal's drupal_set_time_limit may lower or enforce a time limit

This is a general problem, not limited to node_access_rebuild. Because there are a wealth of contrib modules that use the core drupal_set_time_limit() function, the correct place to fix the problem is in core.

This patch is taken from #2212033: Drupal's drupal_set_time_limit may lower or enforce a time limit with a minor update.

D7 version of the patch from #30.

Closed three duplicate issues:

• Core: #2212033: Drupal's drupal_set_time_limit may lower or enforce a time limit
• Migrate: #2287375: setBatchTimeLimit() shouldn't override the timeout if value in php.ini is higher
• XMLSitemap: #2287383: Don't set drupal_set_time_limit() if max_execution_time is > 240

Moving the issue back to the D8 queue so it can be reviewed there first.

The documentation of drupal_set_time_limit() notes that resetting the time limit in the midst of page execution affects only the remaining time and that the function should be called

with an appropriate value every time you need to allocate a certain amount of time to execute a task [rather] than only once at the beginning of the script.

So, if e.g. it's called 30 seconds into page execution to add 30 more seconds and the current max_execution_time value is 40, ignoring the call could reduce the total amount of time available (40 < (30 + 30)) compared to what drupal_set_time_limit() currently does.

Yes, the current implementation is problematic, but it appears to be at least partly by design.

If we're going to change this:

• The function name is potentially confusing, as in some cases it won't do what it claims--set the time limit. drupal_increase_time_limit() would be a more accurate function name.
• Ideally we would compare the new limit to the combination of max_execution_time and the time that's already elapsed since drupal_set_time_limit() was last called or the page execution began.
• The documentation needs updating.

This is not critical.

This is ridiculous that this has gone on for year after year. The way to fix this w/o dealing with patching core after every upgrade is to disable the set_time_limit() call in cli/php.ini. The actual function in common.inc is this:

function drupal_set_time_limit($time_limit) {
  if (function_exists('set_time_limit')) {
    @set_time_limit($time_limit);
  }
}

They went out of their way to skip or suppress errors dealing with this function to accommodate for the function being disabled.

Just add this to your [cli] php.ini:

max_execution_time = 0
disable_functions = set_time_limit

Patch in #30 still applies to 8.x. However, it's unclear what the status of this issue is. It was marked as a duplicate (of something...) above in #23, and also as won't fix.

@corwin: So maybe that should be an addition to the comments in default.settings.php?

Rerolled.

assgning is misspelled in this patch.

I have no comment on the accuracy of the documentation, but ... after reading it, I was very confused about why this would work, and it wasn't until I scrolled up in the comments on this issue that I understood it. So... maybe it's not really great documentation? Could it perhaps be explained a bit more? Here's my suggestion:

The Batch API calls the PHP function set_time_limit() to hard-code its execution time limit to 240 seconds, regardless of the PHP max_execution_time setting. If you want the Batch API to use the PHP setting, disable the set_time_limit() function and set the PHP limit to a different value (this will not break the Batch API call, which is set to ignore errors). You can do this in your php.ini file, or by uncommenting the following lines.

I believe the patch in #31 is the best approach - at least for handling batch processes over the command line. Rebuilding a lot of nodes via Drush can timeout due to this hardcoded limit. However, I don't want to fully disable altering of the time limit in case users hit something in the UI which requires it.

I also believe that the approach of the patch #30 and #31 is the best one.

But as nedjo told,

The function name is potentially confusing, as in some cases it won't do what it claims--set the time limit. drupal_increase_time_limit() would be a more accurate function name.

if we add a limitation that `drupal_set_time_limit()` cannot decrease the time limit, the function name is confusing and it's problematic when we want to decrease the time limit intentionally.

And since the original behavior of `set_time_limit()` is confusing originally, it's hard to solve this issue with a simple solution.

* This function is a wrapper around the PHP function set_time_limit().
* When called, set_time_limit() restarts the timeout counter from zero.
* In other words, if the timeout is the default 30 seconds, and 25 seconds
* into script execution a call such as set_time_limit(20) is made, the
* script will run for a total of 45 seconds before timing out.

I'd like to propose adding second argument to the function to switch the behavior though this is not concise.

    function drupal_set_time_limit($time_limit, $allow_reduction = TRUE) {
      if (function_exists('set_time_limit')) {
        $current = ini_get('max_execution_time');
        // Do not set time limit if it is currently unlimited.
        if ($current != 0) {
          // When reduction is allowed, set any value.
          // When reduction is not allowed, change the value only if the current
          // value is less than the passed value. That is, the value is only
          // increased.
          if ($time_limit == 0 || $allow_reduction || $current < $time_limit) {
            @set_time_limit($time_limit);
          }
        }
      }
    }

Here are patches.

This issue still exists in the new method, so we can still improve this. While the approach in #44 allows us to keep BC by not changing the behavior by default, I don't think this is a thing we actually want to do.
I think instead the approach in #31 is the correct approach.

Adding a patch for 9.5.x but following @borisson_'s advice in #57.

Cleaning up the inline comments.

Created an MR from the latest patch.

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Hiding patches.

Even though a small change could we get a test? Seems like a scenario that should be covered.

Added some small tests.

Since I just added tests and not actual fix and the tests were super simple think I'm good to mark this one.

   * This function is a wrapper around the PHP function set_time_limit(). When
   * called, set_time_limit() restarts the timeout counter from zero. In other
   * words, if the timeout is the default 30 seconds, and 25 seconds into script
   * execution a call such as set_time_limit(20) is made, the script will run
   * for a total of 45 seconds before timing out.
   *
   * If the current time limit is not unlimited it is possible to decrease the
   * total time limit if the sum of the new time limit and the current time
   * spent running the script is inferior to the original time limit. It is
   * inherent to the way set_time_limit() works, it should rather be called with
   * an appropriate value every time you need to allocate a certain amount of
   * time to execute a task than only once at the beginning of the script.

Here's the current docs from the \Drupal\Component\Utility\Environment::setTimeLimit() - they need a lot of work with this change. And also this change represents a regression because it does not cover the

   * This function is a wrapper around the PHP function set_time_limit(). When
   * called, set_time_limit() restarts the timeout counter from zero. In other
   * words, if the timeout is the default 30 seconds, and 25 seconds into script
   * execution a call such as set_time_limit(20) is made, the script will run
   * for a total of 45 seconds before timing out.

part of it and it might result in current working code now timing out because we'll fail to add more time.

- resolved regression issue
- updated IS for proposed solution

For the feedback on the MR.

Review comments resolved.

Left a comment on MR.

Believe this should be good now.

The problem with this change is that it does not work as expected.

If the time limit is set to 500 by your php.ini... and after 499 seconds the code calls \Drupal\Component\Utility\Environment::setTimeLimit(100)... you'll get 100 more seconds as per the PHP manual

When called, set_time_limit() restarts the timeout counter from zero. In other words, if the timeout is the default 30 seconds, and 25 seconds into script execution a call such as set_time_limit(20) is made, the script will run for a total of 45 seconds before timing out.

See https://www.php.net/manual/en/function.set-time-limit.php

This change will prevent that from happening.

The issue does not really contain any justifications from overriding PHP's behaviour and why this is important.

If the issue is that we want to be able to config node_access_rebuild() and cron to have longer than 240… I think we should solve that - rather than trying to work around PHP vagaries.

If the time limit is set to 500 by your php.ini... and after 499 seconds the code calls \Drupal\Component\Utility\Environment::setTimeLimit(100)... you'll get 100 more seconds as per the PHP manual

With the current change, it does allocate more time instead of 100 more seconds, it allocates 600 more seconds. This solves the problem that if the time limit set in php.ini was 500 and after 2 seconds code calls \Drupal\Component\Utility\Environment::setTimeLimit(100), the time limit reduces from 498 to 100 seconds. The fix allocates always the maximum possible so that the process can complete. This allows site administrators to increase the time limit in php.ini for timeouts occurring on their site where reduction in time happens due to settimelimit in code.

@sukr_s oh I see... so now we need to explain why calling setTimeLimit(100) can result in adding 600 seconds and not 100. Again I think we're solving the problem wrong. But it gives me a good idea.

I think we should deprecate this method - it is complex to use and doesn't not what we want. I think we should add a new method with this code that is called something like ensureMinimumTimeLimit() that only sets the time limit when the current limit is smaller that that is being set.

This new method should also document that it behaves different from PHPs set_time_limit and is designed to be called once per process and early on.

@alexpott the sarcasm is not appreciated.

No developer sets the exact time they need. Excerpt from core

 // Try to allocate enough time to run all the hook_cron implementations.
    Environment::setTimeLimit(240);

They hope that the process will end in the given time. The intent is not to run for another 240 seconds, but to have the task completed. When that does not work, the site administrators increase the time in php.ini which has no effect due to the behaviour of settimelimit.

Enhancing the current function will strength the execution of Drupal core. I do not agree that a new method is required for this. And additional education required for all developers to use the new method instead of the standard function.

@sukr_s I'm sorry. I didn't intend to be sarcastic. I guess I missed some words out.

#83 should have started like: @sukr_s oh I see... thank you for pointing this out. This means that we now need to explain why calling setTimeLimit(100) can result in adding 600 seconds and not 100.

@sukr_s re #85 I disagree, We need a new method otherwise we are not helping things. The workings of PHP's set_time_limit() are clear making the wrapper Environment::setTimeLimit() work different does everyone a disservice. A method that is well named and documented is a good thing.

I had a look at some our frameworks and applications to see way they do... Symfony and Laravel have no wrappers are this method and don't really use it. Wordpress doesn't wrap the method and has plenty of calls directly to the PHP builtin https://github.com/search?q=repo%3AWordPress%2Fwordpress-develop%20set_t... ... and craftcms has something funny... https://github.com/craftcms/cms/blob/7f77ea0be02f5be1ebc6733115f4469f0b3...

We could consider removing the method completely and add a config setting to automated cron to set the time limit to 240 (or whatever is configured) before it run's cron. And then Drupal will obey PHP's setting always with no oddness apart from automated cron - where it will do something special which feels okay because the whole way of running cron during kernel termination is funky.

There's one other place we run cron from the browser which is the manual 'run cron' button from the status report, but this could use the same setting (might need to be in system somewhere instead of automated cron).

In the interest of progress, should this issue be rescoped to the deprecation? Or closed as outdated and two new tasks created (with credit transferred as needed) -- one for the deprecation and one for the new service?

Now that we are up to 90 comments and back to square one, it seems maybe more efficient. Unless we will be using any of the work so far.

IMO the current changes can be committed and issue closed. Additional function and deprecation is not required. The promise of settimelimit is to allocate additional time. The changes does exactly that. Since the developer cannot guarantee the amount of time needed when calling this function, it can be increased by setting the php.ini in production by administrator to ensure the job is done. Adding more time than called for by the developer does not impact the site since the process will end once the task is complete irrespective of the remaining time.

This method is similar to Json::encode & Json::decode wrappers implemented in Drupal. These wrappers provide a "preferred way" in Drupal that developers don't need to remember all the flags to use each time. This method on similar lines has a "preferred behaviour" which is allowing administrators to allow increasing the time limit without changing the default behaviour.

I am not seeing a lot of support for the changes here, and I disagree with #91 that it is analogous to Json::encode. With the Json::encode example we are passing preferred options to the PHP function.

With setTimeLimit we are also doing that (in its current state).

With the changes in the MR we are changing the behaviour by adding to the php ini time limit - this could be a huge behaviour change for people calling this function.

I think our best bet is deprecate + new function.

Yes I agree with #92, we should replace this with something that's more explicit/less confusing.

Opened #3473323: Deprecate Environment::setTimeLimit() with no replacement.

Moving over credit