Updated: Comment #N

Problem/Motivation

We should only allow the limit to be increased otherwise we don't allow tests or users to increase the time limit to what they want.

// Try to allocate unlimited time to run the tests.
drupal_set_time_limit(0);

run-test2233929s.sh

    drupal_set_time_limit($this->timeLimit);

TestBase::prepareEnvironment()

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

cron.php

      // Try to allocate enough time to rebuild node grants
      drupal_set_time_limit(240);

node.module node_access_rebuild()

---
Causes fails like:

Drupal\config\Tests\ConfigImportAllTest->testInstallUninstall() failing

looked at the test results, in the error log and found:

Fatal error: Maximum execution time of 240 seconds exceeded in /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/yaml/Symfony/Component/Yaml/Parser.php on line 542
FATAL Drupal\config\Tests\ConfigImportAllTest: test runner returned a non-zero error code (255).

Proposed resolution

Fix code.

Remaining tasks

Review and agree patch

User interface changes

None

API changes

None

Files: 
CommentFileSizeAuthor
#21 2233929-drupal-set-time-limit-increase-only-D7.patch1.62 KBDave Reid
PASSED: [[SimpleTest]]: [MySQL] 41,138 pass(es).
[ View ]
#15 drupal-set-time-limit-2233929.15.patch478 bytesBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,556 pass(es).
[ View ]
#7 2233929.7.patch1.64 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,618 pass(es).
[ View ]
#7 5-7-interdiff.txt1.17 KBalexpott
#6 2233929.5.patch552 bytesalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,467 pass(es).
[ View ]
#2 2233929.2.patch564 bytesalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,545 pass(es).
[ View ]

Comments

alexpott’s picture

Issue summary:View changes
alexpott’s picture

Status:Active» Needs review
StatusFileSize
new564 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,545 pass(es).
[ View ]

How about this.

alexpott’s picture

Status:Needs review» Closed (works as designed)

alexpott is wrong.

http://3v4l.org/i7XsL

alexpott’s picture

Status:Closed (works as designed)» Needs review

Well now I definitely am wrong lol

<?php
print ini_get('max_execution_time') . "\n";
set_time_limit(0);
print
ini_get('max_execution_time') . "\n";
set_time_limit(10);
print
ini_get('max_execution_time') . "\n";
set_time_limit(50);
print
ini_get('max_execution_time') . "\n";
set_time_limit(10);
print
ini_get('max_execution_time') . "\n";
?>

Outputs

0
0
10
50
10

So actually I think we do have a problem here.

YesCT’s picture

StatusFileSize
new1.82 KB
new1.42 KB

docs update for change in #2

alexpott’s picture

StatusFileSize
new552 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,467 pass(es).
[ View ]

So given

* Attempts to set the PHP maximum execution time.
*
* 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.

Let's only allow setting the limit if it is not set to unlimited.

YesCT++ for making me read the docs!

alexpott’s picture

StatusFileSize
new1.17 KB
new1.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,618 pass(es).
[ View ]

Improved the docs.

YesCT’s picture

Status:Needs review» Reviewed & tested by the community

ok. new resolution still allows decreasing the limit.

seems reasonable, docs updated nicely.

YesCT’s picture

Issue summary:View changes

put the fail this fixes in issue summary.

sun’s picture

Looks good to me as well.

YesCT’s picture

sun’s picture

Priority:Normal» Critical
webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

  • Commit 7ba1d72 on 8.x by webchick:
    Issue #2233929 by alexpott: Drupal_set_time_limit should only be able to...
Berdir’s picture

Status:Fixed» Needs review
StatusFileSize
new478 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,556 pass(es).
[ View ]

Looks like this didn't fix anything after all :)

#2224887: Language configuration overrides should have their own storage was still failing with the 240s timeout (apparently makes it a bit slower) and when debugging, I found out that init_get() returns (string) "0", so the type safe check there fails.

Uploading the interdiff from the languag override issue as a patch...

alexpott’s picture

Status:Needs review» Reviewed & tested by the community

Confirmed the vli default is string(1) "0"

catch’s picture

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

  • Commit 8d373d0 on 8.x by catch:
    Issue #2233929 by alexpott, Berdir: drupal_set_time_limit() should only...

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Dave Reid’s picture

Version:8.0.x-dev» 7.x-dev
Issue summary:View changes
Status:Closed (fixed)» Patch (to be ported)
Issue tags:+Needs backport to 7.x

This is so eligible and needed for Drupal 7 backport.

Dave Reid’s picture

Priority:Critical» Normal
Status:Patch (to be ported)» Needs review
StatusFileSize
new1.62 KB
PASSED: [[SimpleTest]]: [MySQL] 41,138 pass(es).
[ View ]
manuelBS’s picture

Thanks for the patch. I tested it in two projects where I want to set the max execution time of cron runs to unlimited due to big data calculations and this patch helped to get it working. Thanks!

xjm’s picture

Issue tags:-Needs backport to 7.x+needs backport to D7
fgm’s picture

Version:7.x-dev» 8.0.x-dev
Status:Needs review» Needs work

Actually, this does not accomplish the stated goal : if the max_execution_time is set and nonzero, but higher than the hardcoded limit in Drupal, this will actually reduce the time limit (just had this running D8 tests).

So I think this needs to be changed again in 8.x (then backported) to only be able to increase the limit, hence changing status and resetting to 8.x. However, due to the lack of something like get_time_limit(), there is no apparent way to accomplish the stated goal in that case.

fgm’s picture

Version:8.0.x-dev» 7.x-dev
Status:Needs work» Reviewed & tested by the community

Resetting to 7.x, and marking RTBC as the patch looks goot to me, and applies on current 7.x HEAD.