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