set_time_limit: Centralize calls and prevent warnings and errors

Oh, I get that warning about set_time_limit() disabled always, too. It only means that cron tried to enhance it's allowed lifetime through set_time_limit() to be absolutely sure it'll not fail, BUT your hosting provider's policy disallows that setting.

The only way to remove that warning would be to change php configuration (a job for administrator of the server), but this is a peaceful little warning, which is safe to ignore. As long as your cron is short (keep the amount of search-indexing items per cron-run low, as seen somewhere on the admin UI), you're OK.

I always thought that this warning is 'by design', until I checked in the code: Drupal already attempts to detect the php configuration, and so to avoid the warning, but fails on that in my case.

Drupal checks for 'safe_mode' flag - if that is set, then set_time_limit() is not attempted. But my hosting provider doesn't use safe_mode; probably for better granularity of permissions, safe_mode is off, but there's a big list of function names inside 'disable_functions' instead. That's not checked by Drupal, so set_time_limit() is attempted, and always shows something like:warning: set_time_limit() has been disabled for security reasons in /home/jirux/test-webspace/xt6/includes/common.inc on line 2561.

We may as well check 'disable_functions' too... So attaching a patch - works fine for me, no more warnings shown. Changes all three instances in core. (As always, needs to go into 6.x first, rather than 5.x).

The patch got a bit of offset now, so why not re-roll... Otherwise no change. I know this is nothing critical, but still a bit of feedback might be nice ;)

Another bit of offset today, but I'm more interested in opinions and reviews. This is really a simple patch.

Isn't it possible to disable set_time_limit in php configurations without also being in safe mode?

Yes, of course (if I understood #5 question correctly) - the problem here stands as follows:

- If ini_get('safe_mode') is TRUE, Drupal doesn't attempt to set_time_limit(), because it's bound to fail. That's the existing behavior already (not changed here), which doesn't apply to my hosting's config, though: The host doesn't run in safe mode.

- But my host still disabled the function, through 'disable_functions' (as #5 suggested, if I got the point right). The existing Drupal code doesn't see that, so set_time_limit() is attempted, and always fails (and prints warning).

- With the patch, set_time_limit() is only attempted if none of these two blockers are present, i.e. php being NOT in safe mode *AND* set_time_limit NOT present between the disabled functions (note that both the '&&' subparts are in fact negative conditions). Only then there's a chance to set the limit successfully.

Unless there are some other config options able to block that function too, I think this is the correct behavior.

Sorry JirkaRybka, that was clearly too early in the morning, makes complete sense now sorry.

No problem, thanks for looking into this anyway :)

So, what's the conclusion here: Anyone going to set RTBC, or to voice some additional concerns?

Big offsets again; this new patch is also done using cvs diff.

+1 for this patch.

Don't know any reason why it shouldn't be put in... It's always best to keep extraneous error messages away from the user, and this takes care of a very common situation which the original code tried unsuccessfully to deal with.

Good patch, but now august 2008, I installed 5.9. and have same trouble.
set_time_limit() has been disabled for security reasons in /includes/common.inc on line 2008.

There is a cure for this???

I think the patch can be simpler than this. If a function is disabled by disable_functions, function_exists() will return false.

Attached are patches for D5 and D6.

According to the comments on php.net this still wouldn't work if the functions are disabled by the suhosin module. But I think this will fix the problem for most of the users.

HEAD is somewhat inconsistent. The cron function no longer checks for safe_mode but instead just suppresses the error. Node and locale still use the old method.
I think the cleanest way would be to create a drupal_extend_time_limit() method which does all the required checks and can be re-used by contrib. I thought this was proposed somewhere, but can't find it anymore..

Yes, I applied patch on 5.9 - cron working without any warnings.
Thanks.

Resetting version number. We always use the highest affected version.

:) I updated to 5.10 . This warning appear AGAIN :)))) Im tied :))

Logrise - yes, it's not been committed yet. Needs to go into 6.x first, then into 5.x.

Ok, understood. Will waiting. On the 6.4 site is the same
set_time_limit() has been disabled for security reasons in file /includes/common.inc in line 2515.

Since the patch is trivial and Logrise has confirmed it works on both branches, marking RTBC (have only done visual review myself).

   // If not in 'safe mode', increase the maximum execution time:

Those comments need updating to reflect what the code is actually doing.

Also, we should get some tests around this for D7.

it works in my Drupal 6.6

grepped Drupal 7 after partially applying patch for D6.

locale.inc and node.module are fixed as shown below. common.inc hunk failed to apply.

common.inc uses @ (which looks really jarring; our code policy is normally to check fail conditions rather than suppress errors). run-tests.sh is a questionable case. I can't see anyone accessing a shell and run unit-tests with safe-mode PHP, to be honest. It makes no sense.

Options:

1.) Remove common.inc (first hunk) from D6 patch.
2.) Change common.inc to use the same condition check as the other files.

includes/common.inc-  // Increase the maximum execution time.
includes/common.inc:  @set_time_limit(240);
--
includes/locale.inc-  // If not in 'safe mode', increase the maximum execution time.
includes/locale.inc:  if (!ini_get('safe_mode') && function_exists('set_time_limit')) {
includes/locale.inc:    set_time_limit(240);
--
modules/node/node.module-      // If not in 'safe mode', increase the maximum execution time.
modules/node/node.module:      if (!ini_get('safe_mode') && function_exists('set_time_limit')) {
modules/node/node.module:        set_time_limit(240);
--
scripts/run-tests.sh-if (!ini_get('safe_mode')) {
scripts/run-tests.sh:  set_time_limit(0);

I think we need a drupal_set_time_limit($time_limit) in common.inc which checks for
- safe mode
- disabled functions list
- ensure we don't accidentally reduce the time limit

The latest patch at #174617: Environment::setTimeLimit() ignores current value, can reduce timeout value does some of this, probably needs merging with this.

Here's an attempt. Given that this defines a new drupal_set_time_limit() function in common.inc, seemed easiest to use this everywhere for consistency. Includes tests, which will actually fail if safe_mode is on or if set_time_limit is disabled.

Trying again, can't seem to change the patch on #24 except by deleting it :P

[update] OK still no joy, try a different filename! Finger trouble methinks.. Hope that's got it.. :D

C'mon bot!

Marked #174617: Environment::setTimeLimit() ignores current value, can reduce timeout value as duplicate. Raised level to critical like that issue was.

Reroll of patch also to make testbot happy.

Chasing HEAD

The comment "+ * Note that a time limit of 0 means unlimited." seems like it's in a strange place. It should probably be on the @param.

All the limit testing seem totally unnecessary... Why go to all the trouble?

I think we still need to use @set_time_limit() to suppress the errors. function_exists returns false when the function is disabled using disabled_functions, but not when its disabled by suhoshin. So the method call may still fail on some hosts.

@34: Is there any way of checking if set_time_limit() has been disabled by suhosin?

Maybe sufficient just to check suhosin.executor.func.blacklist via ini_get() ???

@32: Agree re. the @param.

Re. the limit testing, see the original post at http://drupal.org/node/174617, and also #9. Without the limit testing then Drupal might reduce the time limit (actually, it currently will if the default is > 240; this is probably not uncommon on larger sites).

Have also included check for suhosin.executor.func.blacklist. As I understand it, use of error-suppression operator @ in Drupal is something of a last resort?

Re. the limit testing, see the original post at http://drupal.org/node/174617, and also #9. Without the limit testing then Drupal might reduce the time limit (actually, it currently will if the default is > 240; this is probably not uncommon on larger sites).

Actually, that's a bit more trickier that this.

Please see the description of 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.

In other words: you increase the timeout as long as you set a duration > as the one remaining. But of course, there is not get_time_limit() call...

It would be better to just call set_time_limit() at regular interval during a long processing (every 10,000 entries processed or something like this).

i'd like to see the tests drupal_set_time_limit() broken into multiple lines. it seems like way to much code to be run in two lines.

what would ini_get('max_execution_time') return that's non numeric?

the phpdocs still need work they don't follow the conventions in the rest of core. (int) isn't the way we typically do it, it's usually a complete sentence. Perhaps something more like: "An integer specifying the desired time limit, in seconds. A value of 0 indicates no time limit should be enforced."

Oh one other aspect that hasn't been addressed is you don't document that you only increase the limit. What if i wanted to set it low so there's a quick time out? The PHPDocs don't make it clear that this function won't do it--and I actually think that it's a bad idea to break from PHP's default behavior if you're going to name this function so it looks like it's just a wrapper on the PHP function.

Looks like I cross posted with Damien Tournoud. He's correct about the operation of the function, I should have referred back to the docs.

@38: Very true, but the set_time_limit is hit early in the request processing in the instances in core so the time already consumed will be near to 0. Agree that repeatedly calling set_time_limit() would be a better bet for some operations; I guess that's partly why we ended up with a Batch API. The function should allow the time limit to be "re"-set to its current value, to allow for repeated calls, which it currently doesn't.

@39: Agree the numeric check is probably unnecessary plus other points.

@40: The docs twice mention that the wrapper will only increase the time limit, but maybe drupal_increase_time_limit() would be clearer (there's a similar suggestion at #13 above).

Subscribing. I think this issue should be addressed... most shared hosting have set_time_limit disabled.

After reading all the comments above, I modified the patch in #37 and tested it.

• I removed the tests to avoid "decreasing" execution time (also removed the assertions in the test class related to that). It simply cannot happen, by design, with set_time_limit (maybe with a time machine ;))
• Slightly modified the way to detect safe_mode.
• Adapted function name, comments as suggested.
• I did not put the if condition on multiple line as suggested, I tired to find coding standards about this in the documentation, I did not find anything and looking at few core files I have seen many places where conditions are longer and not splited.

Hmm.. I suspect a problem with the test bot... my bad if it's not and I'd be happy to know what really failed ;)

OK then.. I suppose I'll have to rely on someone to explain this to me then :S

I'm setting the priotity right, according to guidlines and give it a better title. This is just a PHP warning, nothing that will prevent the cron from running, but still it is annoying as hell when it comes to read the logged events (in Drupal 6 ! In HEAD it is "fixed" by a -not so clean- error control operator).

Ok, after some reading about SimpleTest (sorry, I'm pretty new to patching against drupal-HEAD). I found the error in the common.test file... there was an unescaped single quote. Let's hope this one works :)

@44: http://uk.php.net/set_time_limit *can* be used to reduce the time limit (per #40). Also the way the patch at #37 checked for safe_mode was modeled on the checks for register_globals (perhaps the code comments needed to say something about this..)

With your previous way to check if the value was "decreasing" the max_execution_time here is an example (with arbitrary values) of what could have happend : set_time_limit is called after 30 seconds of execution with 35 as new time limit to set and the current max_execution_time is set at 40; your function won't allow this new limit because this new time limit is inferior to the max_execution_time and thus prevent developers to give 35 more seconds. I do not see how it is better; unless we would check the elapsed time running the script, this verification type of verification not efficient and it is meaningless to compute elapsed time only to "try" to increase the timelimit. We should just use this function as a pure wrapper and let developers figure out how PHP's set_time_limit is working and which best value they should use in their environement.

About safe_mode verification, you are right, we should stay consistant.

About the test class, we should discuss its utility now, because without testing if decreasing is possible or not, we are just testing the environement and not the unit. Testing this unit seems useless, since with this patch, it would be only a wrapper.

Here's the corrected patch for this.

Ok, I corrected the patch, since it failed because of the duplicate that got commited "thanks" to this issue : #375578: Cron: guarantee each cron implementation the same share of execution time

Only issue left I see is to determin if those test cases are useful, I don't think they are, they test only the environement, not the unti itself.

Let's just ignore the warning. The condition for this is very complex, involves string parsing, and I suspect it doesn't even covers all possible configuration cases.

if (function_exists('set_time_limit')) {
  @set_time_limit($limit);
}

Fine by me too, I also think the Suoshin check is too much (it is not in core of PHP). But this issue reports a precise warning that is dealt with "function_exsits" (this solves the warning caused by disable_function directive in php.ini), to my knowledge within PHP core there is only another case when set_time_limit can be "disabled" and raise a warning it is with when safe_mode is enabled, so why not treat it here too ? These kinds of warnings are likely to be raised on shared hosts which use both those ways to "secure" their ressources.

Now, I think that Drupal is not here to babysit the server administrators so we should just use !ini_get('safe_mode'). If administrators want to set "safe_mode" to weird values which won't produce a boolean as described in the manual (as quoted below, from ini_get PHP manual page), they should just not expect everything to work fine :

Note: When querying boolean values
A boolean ini value of off will be returned as an empty string or "0" while a boolean ini value of on will be returned as "1". The function can also return the literal string of INI value.

I also removed the test cases, which aren't testing the unit. I really see no point in testing the environment.

Please hide the warning with "@", there is no way to consistently avoid it. And you can probably also drop the check for ini_get('safe_mode') as serves no real purpose.

The function_exists() call is useful, because it avoids a PHP fatal error on some hosts.

I do not think it is a good practice to use the control operator that way. You want to use it to avoid all the warnings, but the ones that are caused by PHP core can be handled easily as I proposed and the rest of warning should be raised (why shouldn't they ?). Using the control operator that way is an excellent way to prevent easy debugging in case this portion of code fails at some point which is not related to PHP core.

**EDIT**
On a side note, as far as I know, the "Fatal error" when "set_time_limit" is disabled (either by safe_mode or disable_functions) was happening on PHP4 hostings, and I remember reading Drupal 7.x won't be supporting PHP version below PHP 5.2 anymore so why bother checking if "function_exists" in this case ;).

That's true that it is not generally a good practice, but I do know that we use the suppression character in http://api.drupal.org/api/function/drupal_cron_run/7 right now, so there's precedent.

In that particular case, it makes a lot more sense to simply ignore a warning if it is raised, rather then add a complex (and not full proof) logic to try to prevent it.

OK, there's no real point to argue. Although a precedent does not mean it was a good use back then either. Sure it is a lot easier (not really faster because "@" is costly in terms of performance, but this function is not called that many times anyways), it might just skip errors you wanted to see in the log (errors which stand outside the usual way PHP core is working... like suhoshin). That is why I also updated the new function comments accordingly.
So here's the amended patch.

By the way, I do not see why this condition should be fool proof, it's an ideal we want to reach, but all your solution does here is avoid all warnings and even fatal errors... warnings/errors that might be useful for server admins, this does not sound like "fool proof" to me... maybe as bad as the first solution, but not better in my opinion ;)

Running in safe mode is not an error, especially when the configuration is not under your control (shared host - quite common case). Attempting forbidden function on such a config is an error. But the user is not responsible for that either (more or less - this is debatable), since it is official Drupal code, and hacking core is not recommended. Leaving the user with error messages he can't fix creates a strong feeling of Drupal code being unstable/unreliable.

I don't see, how the messages are supposed to be useful (unless you run your own server [having access to the config], and yet don't know whether it's in safe mode, and/or how to test it with a one-liner testing script - I don't think this is a common case).

All I said is that this approach was not better in the sense that in case of error at this function call, the user is left in the dark, especially in case of Fatal Error. function_exists should avoid it, but it is not "fool proof" either, read #34 (my understanding is that with suhoshin, for example, there is a fatal error, the function exists and is not disabled by PHP core settings, so function_exists return TRUE). So the script might end abruptly there, without any easy way to debug (no error message, no line number, etc.). Leaving users in the dark is not a better approach, no matter if they have power or not on the server.

In order, the productive approach would be to test this patch which follows the current consensus (using '@'). Then maybe debate if, without using '@', we could treat the common PHP core behaviours, avoid the call if it we know it is not going to work (and maybe log it) and for other undetermined errors, leave the usual error reporting process do its job and join this to a Drupal setting allowing to disable call to this function (for people on shared hosts, which cannot change their server settings). This would be even easier now that we would have a wrapper, this would be a potential feature request to create when this patch finally gets tested and reviewed :P

Oops, there was a mistake in the comments ("in" -> "by") of the wrapper function.

I don't know why the test bot couldn't install this to HEAD (I could do it, of course there were offsets), anyways here's an updated patch, again... maybe somebody will care about testing it...
(Content of the patch does not change, only offsets ... and dates of course)
** EDIT ** Previous patch was working... the testing bot was just behaving ;)

Not bad.

-  // If not in 'safe mode', increase the maximum execution time.
-  if (!ini_get('safe_mode')) {
-    set_time_limit(240);
-  }
...
-  if (ini_get('max_execution_time') < 240) {
-    @set_time_limit(240);
-  }

2 required, but missing safety checks though.

Additionally, we can remove the error suppression, because that function should check all requirements (and statically cache them).

Furthermore, it would be nice to report back whether the change was successful. Modules may be able to react on that. For example, queuing longer jobs alternatively.

sun: The verifications you mention are not reliable. The first one because it is not possible to check efficiently the environment (safe_mode is only one of the cases where the function might be "disabled") so we reached the compromise that we would only check if the function exists (if it exists but it is disabled then the produced warning or error is intercepted by the error control operator, @ ... which is the only point I do not agree, but I am will to fix this issue and I'd say it is a necessary evil.. which might get fixed some day).
The second one because set_time_limit is not working the way this condition supposes, it resets the timer when you call it and thus the moment when it is called is important and we cannot detect if we are decreasing the max_execution_time since we do know/track the time when this function is called, so this "security" check is also unreliable.
All this is explained in more details throughout the issue.

Good summary. I should have mentioned that I didn't read the issue, only the patch. Now we can use #73 as base for reworking and adding explanations for why things are done this way, but also why it is not done differently - like people would obviously expect. Clarifying that drupal_set_time_limit() does not set, but rather reset/append to any existing time limit would be good, too.

The only other point I figure we could "rework" is the error suppression, but Damien Tournoud and webchick condone the use of this error control operator (@), based on precedent uses of it (which were commited) and also because they seem to think that only warnings can be issued by this function (which might not always be true, see #34 ... maybe it is just a warning with suoshin too, but if it is a Fatal error from another PHP security extension then I predict hell for the person who would have to debug this WSOD ;)). Since they are both more invested in Drupal than I am and since I really want to stop these PHP warning errors to scatter Drupal's log on most shared hostings, I did not see any point in arguing and produced the amended patch, so people could test this implementation... but it seems that testers aren't really attracted by this issue ;)
So I think we should concentrate on the functionality now that we reached a compromise

About improving the comments why not, here's a patch (only comments changed)... although, it says that the function is a wrapper around set_time_limit and it would better not to duplicate the documentation of PHP, it could get difficult to maintain the comments if we'd do that too often, but let's say this is a special case.

Probably #174617: Environment::setTimeLimit() ignores current value, can reduce timeout value is no longer a duplicate, since the issue at hand is now only dealing with the problem on the tin: "warning: set_time_limit() has been disabled for security reasons".

Also noting that for cron, the "decreasing time limit" problem was addressed (incorrectly) in #375578: Cron: guarantee each cron implementation the same share of execution time, and remains unaddressed in _locale_import_po() (whether this matters I've no idea).

Perhaps we ultimately need drupal_increase_time_limit() as a wrapper round drupal_set_time_limit()... in a separate issue.. ;)

This one looks good to go.

Well why not address this small issue here... we should not set a new time limit if the current max_execution_time is already set to 0 (unlimited)... for other decreasing case, as stated before, it is not possible to reliably check if we are decreasing, unless we track the time already spent running the script... and tracking this seems pretty useless if it is just for this. I also added a line in the comments of the function : "We only try to set the new time limit if it is not already unlimited.".

>for other decreasing case, as stated before, it is not possible to reliably check if we are decreasing
The issue with, for example, cron, is that we don't want to decrease a time limit of 2,400 to 240. In http://api.drupal.org/api/function/drupal_cron_run the test is in any case made early in the page request so considerations of time already spent are not worth bothering with.

I won't address this, it was demonstrated that the solution you did offer prevent the script to get more time in many cases too I do not see any reason to discuss this further.

spatz4000: So you're ready to code something to know how much time was spent in the script ? Other solutions are not reliable. I won't code unreliably just to satisfy this requirement which is by the way not part of the intial issue, so please do not change the status without discussing it before.

I don't see which "User registration" test this patch could make fail... so let's re-test this ;)

It's time for this issue to get a title reflecting what we are trying to do.

 /**
+ * Attempts to set the PHP maximum execution time.
+ *
+ * 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. It also means that, unless we
+ * monitor the time spent running the current script, it is not possible to
+ * prevent this function to decrease the time limit set by default or manually
+ * at the beginning of the script.
+ * Before calling set_time_limit, we check if this function is available
+ * because it could be disabled by the server administrator. We also hide all
+ * the errors that could occur when calling set_time_limit because it is not
+ * possible to reliably ensure that PHP or a security PHP extension will not
+ * issue a warning error when preventing the use of this function.
+ * We only try to set the new time limit if it is not already unlimited.
+ *
+ * @param $time_limit
+ *   An integer specifying the new time limit, in seconds. A value of 0
+ *   indicates unlimited execution time.
+ */
+function drupal_set_time_limit($time_limit) {
+  if (function_exists('set_time_limit') && ini_get('max_execution_time') > 0) {
+    @set_time_limit($time_limit);
+  }
+}

- can we please have some more line breaks in the description?

- following the discussion here and in #375578: Cron: guarantee each cron implementation the same share of execution time, I think we should remove the check on the max_execution_time. After all, it defeats the purpose of setting the time limit.

I can agree to both points, I'd add that changing the behavior of the function would make drupal_set_time_limit more than just a wrapper ... so basically, my bad for re-introducing this max_execution_time check. Here's the amended patch.

There's a subtle difference between the max_execution_time check in this issue and the other one. Here, it checks for the special value "0", which means there is no limitation, and if that is the case, set_time_limit() shouldn't invoked at all. Like http://de.php.net/manual/en/info.configuration.php#ini.max-execution-time states, 0 is the default value for PHP running on the command line, which means it is possibly the default for drupal.sh as well.

If it's a wrapper, I do not think we are supposed to change the way the original function would behave with the same parameters, so this check should be indeed removed. I agreed to change the patch on this term.
Although if we end up keeping this verification, which in the case of Drupal would make some sense in my opinion, we should say it in the comment and stop calling it a wrapper in there too.

This looks like a nice clean up, thanks TheRec. Let's get this in.

Corrected patch, EOL were not UNIX as required. My bad.

+++ includes/common.inc	3 Aug 2009 15:03:35 -0000
@@ -2295,6 +2295,35 @@
+ * execution a call such as set_time_limit(20) is made, the script will run for

This PHPDoc line seems to exceed 80 chars.

+++ includes/common.inc	3 Aug 2009 15:03:35 -0000
@@ -2295,6 +2295,35 @@
+ * set_time_limit() restarts the timeout counter from zero. In other words, 
...
+ * 
...
+ * 

Trailing white-space here.

+++ includes/common.inc	3 Aug 2009 15:03:35 -0000
@@ -2295,6 +2295,35 @@
+ * the errors that could occur when calling set_time_limit because it is not

Missing braces after "set_time_limit" and I think there should be a comma before "because".

+++ includes/common.inc	3 Aug 2009 15:03:35 -0000
@@ -3555,10 +3584,8 @@
+  // Try to restart the timeout counter from zero and set a sufficient new time limit.
+  drupal_set_time_limit(240);
+++ includes/locale.inc	3 Aug 2009 15:03:35 -0000
@@ -1167,10 +1167,8 @@
+  // Try to restart the timeout counter from zero and set a sufficient new time limit.
+  drupal_set_time_limit(240);
+++ modules/node/node.module	3 Aug 2009 15:03:35 -0000
@@ -2646,10 +2646,9 @@
+      // Try to restart the timeout counter from zero and set a sufficient new time limit.
+      drupal_set_time_limit(240);

All these inline comments do not wrap at 80 chars. Also not sure whether the comment must be so verbose; at least, they should be replaced with comments that explain why set_time_limit() is invoked here at all (instead of describing what drupal_set_time_limit() does).

+++ scripts/run-tests.sh	3 Aug 2009 15:03:36 -0000
@@ -70,10 +70,8 @@
+// Try to set an unlimited execution time 

Trailing white-space and missing period (full-stop) here.

This review is powered by Dreditor.

Thank you for the code review sun.

The first comment was not exceeding the 80 characters limit, maybe this is an issue with "Dreditor" ? Anyways I also reformatted this line so it's not so close to the limit ;)

Missing braces after "set_time_limit" and I think there should be a comma before "because".

Do you mean "parentheses" when you write "braces" ? If so, then I corrected that (plus another same mistake in the previous sentence).

I corrected the inline comments to match each situation where drupal_set_time_limit is called.

Thanks for the great discussion here!

I agree that @set_time_limit() is not ideal (the notes about debugging a complete WTF WSOD are very valid), but it strikes me as even less ideal that we try and code conditions for all of the various PHP security extensions. If one particularly bad one seems to rear its ugly head often, we can always adjust this logic later to take it into account.

I'm still concerned about sun's point in #87 with this possibly monkeying with command-line scripts. But since we're keeping this as a simple wrapper for now, and since now all such calls are centralized, that too can be revisited at a later time.

In any case, this fixes the problem we inadvertently introduced in trying to NOT get it to reduce the time allowed. :P

Couple of quick comment fixes and then this is good:

"prevent this function to decrease the time limit" -> from decreasing

+ * It also means that, unless we monitor the time spent running the current
+ * script, it is not possible to reliably prevent this function to decrease
+ * the time limit set by default or manually at the beginning of the script.

Can we add another few words here to explain how that would happen? I think basically "if the original max_execution_time variable was set to a higher value or unlimited" but I'm not sure.

"issue a warning error when preventing the use of this function." -> issue a warning, preventing the use...)

This change needs to be reflected in the 6.x => 7.x module upgrade page.

Thank you webchick.
Corrected the English errors and tried to clarify how this should be used. I do not want to write more comments, it's almost more descriptive than the PHP manual itself about set_time_limit.
Might 13 be the lucky number... this seems like an endless loop of patch corrections for a pretty trivial problem, mostly due to the fact that people does not know how set_time_limit works and how it should be used... this should not be the role of Drupal documentation to teach this, but it seems that it is required for this case.

If this does not fit anyone could always remove most the comments and just leave a description saying it is a wrapper and point to the official documentation... this might be wiser, but when there was fewer comment, someone requested more explanations.

Minor corrections in the comments of the function : removed a redundant "PHP", added a "/" between "warning" and "error", rephrased the first sentence ("Wrapper around" -> "This function is a wrapper around").

This patch is now even better, thanks TheRec. Let's get this little one in.

Great! Committed to HEAD.

This can't really be solved in the same way in D6, because this is an API change.

Marking "Needs work" for documentation. Please update the 6.x => 7.x modules page.

Since we cannot change the API for Drupal 6, we have to repeat the code if we want the same behavior and while we're at it let's update the comments as we did for D7.

Now documented in Converting 6.x modules to 7.x.
Still needs review for the D6 patch, the status got changed by test bot because I forgot to change the version when posting the ported patch :)

in drupal 6.13 without patch

warning: set_time_limit() has been disabled for security reasons in /home/primopos/public_html/includes/common.inc on line 2633.
-----------Cron run failed.------------

(But cron ran after clearing view cache)

Yes, hopefully someone will take time to evaluate if commiting this to DRUPAL-6 is ok, after D7 code freeze maybe :) If you are interested in having a better use of set_time_limit() in Drupal 7 (and maybe it will get back-ported) you should review : #375578: Cron: guarantee each cron implementation the same share of execution time

Thanks, committed to Drupal 6 too.

D6 Regression #580248: Drupal 6.14: Links not checked, 'max_execution_time' is overriden to "undefined" by drupal_cron_run(), $time_limit is undefined!

See my answer in the other issue. This is a bug in the link checker module.

No. Linkchecker, job queue and others are now broken

I opened #582070: D6.14: Job queue should not use ini_get('max_execution_time'), but call drupal_set_time_limit() itself for Job queue.

Yup... seems like the review process for D6 was kinda skipped ;) Back to a litteral value (240)... There was the same copy/paste error in node.module.

Damien Tournoud> These calls are indeed wrong... so let's make it right ... undefinied variable are never good... by the way, that's where @ comes in the way of good debugging, I guess I wasn't so wrong about it.

@Damien: I do not really understand how I should use drupal_set_timeout here. Logic wise the while() runs as long as the half cron time limit is reached. There is no global variable we can ask for how long the page can run. This is a major problem and the (ini_get('max_execution_time') / 2) seems to be a good idea to solve this problem. If you have a better idea, please share. drupal_set_time_limit() is only available in D7 and SET a time, but we need the maximum allowed page runtime here!

  $result = db_query_range("SELECT * FROM {linkchecker_links} WHERE last_checked < %d AND status = %d ORDER BY last_checked, lid ASC", time() - $check_links_interval, 1, 0, $check_links_max_per_cron_run);
  while ($link = db_fetch_object($result)) {
    // Fetch URL.
    $response = drupal_http_request($link->url, array('User-Agent' => 'User-Agent: ' . $useragent), $link->method, NULL, 1);
    _linkchecker_status_handling($link, $response);

    if ((timer_read('page') / 1000) > (ini_get('max_execution_time') / 2)) {
      break; // Stop once we have used over half of the maximum execution time.
    }
  }

#111: looks code wise good to me for D6. Doesn't such a bug that makes cron running *unlimited* not prove to build a new release - 6.15?

Patched one production machine with patch in #111. linkchecker seems to work as before.

I've taken a look to the drupal_set_time_limit() documentation in D7. Would you really suggest that we run jobs until they are finished? I could run linkchecker and use drupal_set_time_limit(60) in the while()...

   // Let's go take the millions of links from the table at once - drupal_set_time_limit() gives us unlimited time for link checks *G*.
   $check_links_max_per_cron_run = 1000000;
   $result = db_query_range("SELECT * FROM {linkchecker_links} WHERE last_checked < %d AND status = %d ORDER BY last_checked, lid ASC", time() - $check_links_interval, 1, 0, $check_links_max_per_cron_run);
  while ($link = db_fetch_object($result)) {
    // Fetch URL.
    drupal_set_time_limit(60); // value MAY NOT ENOUGH for a link check.
    $response = drupal_http_request($link->url, array('User-Agent' => 'User-Agent: ' . $useragent), $link->method, NULL, 1);
    _linkchecker_status_handling($link, $response);
  }

But this makes the cron running hours or days if I select enough URLs from the linkchecker table... Do you really think this is a good idea for poormanscron enabled sites? I believe there are cheap hosters that may hard limit CPU processing time... in such a case cron will fail. There may be hosters that do not allow to override or use the 'set_time_limit', see comment #104. What will happen in such a case? I think this is why job_queue module have used ini_get('max_execution_time') and I took this approach over to linkchecker as it looked reliable to me to prevent crashes on very long running tasks!?

Stale or endless running processes, eating all memory may be the result... a cron using batch API may be a better approach.

sub

Agree with @TheRec (#111).

Drupal 6.14 breaks cron time handling for modules like FeedAPI or Notifications. Plus it really changes cron behavior for what was suppossed to be a stable core version...

Committed the fix to Drupal 6, thanks. Will be out with the next stable release.

How many stable modules need to be broken for a new core release? :-) With the list from Jose we are aware about 4 broken modules...

@hass: you can help out at #575796: OpenID XRI test violates the spec. and #302240: button broken due to fix various problems when using form storage if you'd like to see a new D6 release sooner.

Although setting the time limit to 0 (i.e. unlimited) was accidental in the patch at #99, I'd agree that modules should not rely on the max_execution_time PHP setting being strictly >0, since 0 is a valid value (even if it is not sensible in most environments), and may be encountered when running Drupal from the command line. Although I can also see that given 240 was hard-coded into drupal_cron_run() it probably seemed a very reasonable to assume that this would not change!

Original issue was against 5.3.

Lowering priority since this was marked critical with respect to fixing the faulty patch at #99 that was committed to 6.x.

How many stable modules need to be broken for a new core release? :-)

Subscriptions is broken, too.

@hass in #113:

    if ((timer_read('page') / 1000) > (ini_get('max_execution_time') / 2)) {
      break; // Stop once we have used over half of the maximum execution time.
    }

If the modules that ran before you have already eaten up half of the max_execution_time, then you won't do anything, even though the second half may go unused.

Subscriptions lets the administrator set a percentage of the total cron time, independently of what happens before and after our cron job:

  // Strategy for cron:
  // Use a defined percentage of the total cron time (default: 50%), but leave at least 5s.
  $total_seconds = ini_get('max_execution_time');
  if (empty($total_seconds)) {
    $total_seconds = 240;  // work-around for broken D6.14
  }
  $lost_seconds = timer_read('page')/1000;
  $available_seconds = $total_seconds - $lost_seconds;
  $usable_seconds = min(array($available_seconds - 5, $total_seconds*subscriptions_mail_get_cron_percentage()/100));
  if ($usable_seconds <= 0) {
    // put a warning into the watchdog log
  }
  while (timer_read('page')/1000 < $lost_seconds + $usable_seconds) {
    ...
  }
  // report the $total_seconds, $available_seconds, and $used_seconds in the watchdog log

Only the administrator knows which other modules run before and after Subscriptions and how much time they need / may use, and he should be able to schedule each module according to his priorities.

Any responsible cron client will take care not to use up all available cron time; breaking ini_get('max_execution_time') is a nasty bug. Another nasty is the DISTINCT bug with Views and node access. I would also like to see D6.15 released as soon as that one is fixed, too.

If the modules that ran before you have already eaten up half of the max_execution_time, then you won't do anything, even though the second half may go unused.

I know - but no problem for linkchecker, can also run on next cron :-)

subs

Tagging.

Hi guys,

I have posted a little something on this issue: #547998: Make cron time limit a variable

In version 6.14 you use a variable which is NOT set (i.e. $time_limit). Looking at this very issue, I see that this variable is expected to be the one passed to a sub-function which I guess was not created... However, that means the variable is NOT defined and thus the timing is removed altogether (i.e. it will run until finished or an out of memory occurs.)

I'm not too sure what you have in mind at this time, but I guess I'd have a different suggestion than what I made in the other issue. I'd simply use 3500 and that's it. Then the rest takes over as if nothing had changed.

  if (function_exists('set_time_limit')) {
    @set_time_limit(3500);
  }

This is until you get something better... which for CRON is probably not really a big deal. (i.e. why add all the complexity of checking the timer, etc. when anyway it will be automatically killed by the server.)

Note that yes... this means many people may run in all sorts of problems because multiple CRON run in parallel... (remember that you kill the semaphore in some cases, so you CAN get multiple CRON running in parallel, especially if someone did setup their CRON to run once a minute.)

Thank you.
Alexis Wilke

P.S. this is in common.inc and node.module, also in node.module you may want to use a smaller number since to reach that location and the generation of a node may not take 1 whole hour?! The one in locale.inc still uses 240.

@Alexis: the D6.14 patch is in #111.

hass,

That would have been quite many entries to read! 8-)

So... only 240 seconds?! Hmmm....

Thank you!
Alexis

subscribe

already fixed in DRUPAL-6

Let's not loose that tag anyway, so we have a good overview of progress. Otherwise we only see a depressing list of items to do :)

I applied the patch in #111 to 6.14 but no change -- cron.php still does not run automatically yet works fine manually.

That means you haven't properly set up your cron job (outside of Drupal). It has nothing to do with this issue.

You said "cron.php still does not run automatically", not I.

This issue has a long history, but the most recent problem that we dealt with was a bug that was introduced in Drupal 6.14 and that causes various modules to not do anything because they think they don't have any time left (independently of whether cron was started externally or internally). The bug fix for this was committed in #106. That bug does not manifest itself in "cron.php still does not run automatically yet works fine manually" but in cron-using modules failing.

If "cron.php still does not run automatically yet works fine manually" for you, then you have a different problem. Please open a new issue and do not try to hijack this one.

.

Note that this does not fix the 240s in #174617 : the hard-limit is still there in 6.15 for node_access_rebuild() line 2323 of node.module.

I suspect most uses of the non-batch mode for node_access_rebuild() are for CLI, and by default the execution time is set from the max_execution_time option, so it needn't be extended in such cases. How about modifying the text like:

// currently:
      if (function_exists('set_time_limit')) {
        @set_time_limit(240);
      }
// suggested
      if (function_exists('set_time_limit') && php_sapi_name() != 'cli') {
        @set_time_limit(240);
      }

#86 is not relevant any more. Resetting status.

D5 needs a backport of #99 and incorporating the fixes in #111.

Marking as won't fix as d5 is end of life.