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

CommentFileSizeAuthor
#21 2233929-drupal-set-time-limit-increase-only-D7.patch1.62 KBDave Reid
#15 drupal-set-time-limit-2233929.15.patch478 bytesBerdir
#7 2233929.7.patch1.64 KBalexpott
#7 5-7-interdiff.txt1.17 KBalexpott
#6 2233929.5.patch552 bytesalexpott
#2 2233929.2.patch564 bytesalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Active » Needs review
FileSize
2233929.2.patch564 bytes

How about this.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs review » Closed (works as designed)

alexpott is wrong.

http://3v4l.org/i7XsL

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Closed (works as designed) » Needs review

Well now I definitely am wrong lol

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

YesCT CreditAttribution: YesCT commented
FileSize
2233929.5.patch1.82 KB
interdiff-2-5.txt1.42 KB

docs update for change in #2

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
2233929.5.patch552 bytes

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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
5-7-interdiff.txt1.17 KB
2233929.7.patch1.64 KB

Improved the docs.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Status: Needs review » Reviewed & tested by the community

ok. new resolution still allows decreasing the limit.

seems reasonable, docs updated nicely.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue summary: View changes

put the fail this fixes in issue summary.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Looks good to me as well.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Related issues: +#2234159: Fix config importing through the UI and change ConfigImportAll test using the UI, +#1744302: [meta] Resolve known performance regressions in Drupal 8

making that test take less time is related.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Priority: Normal » Critical
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
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

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Fixed » Needs review
FileSize
drupal-set-time-limit-2233929.15.patch478 bytes

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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs review » Reviewed & tested by the community

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

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
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

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
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

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Priority: Critical » Normal
Status: Patch (to be ported) » Needs review
FileSize
2233929-drupal-set-time-limit-increase-only-D7.patch1.62 KB
manuelBS’s picture

manuelBS CreditAttribution: manuelBS commented

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

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: -Needs backport to 7.x +Needs backport to D7
fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
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

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
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.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Title: drupal_set_time_limit should only be able to increase the time limit » drupal_set_time_limit should not be able to change the time limit if it's already unlimited
Status: Reviewed & tested by the community » Fixed
Issue tags: +7.40 release notes

Yeah, I guess this makes sense since pretty much any time any Drupal code uses this it's with the intention of increasing the time limit.

Committed to 7.x - thanks!

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid at Lullabot commented

Status: Fixed » Closed (fixed)

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

func0der’s picture

func0der CreditAttribution: func0der commented
Parent issue: » #2183483: Cron lock time can be outrun by max execution time
Related issues: +#2183483: Cron lock time can be outrun by max execution time

Practially the cause of what I have said here three months earlier, but okay. Take it.