Updated: Comment #10

Problem/Motivation

The implementation of the `drupal_set_time_limit` PHP wrapper function actually inhibits the ability for long running processes when invoked the CLI (and potentially via HTTP, though this actually cause HTTP timeouts).

This is because when functions like `drupal_cron_run` are called, the current implementation of `drupal_set_time_limit` assumes that the specified $time_limit provides enough time to complete the execution of the process and when in fact it may not. More importantly, `drupal_set_time_limit` imposes the specified $time_limit when running via CLI and the CLI's environment has an unlimited execution time.
By default, PHP's `max_execution_time` is set to 0 for CLI and should be respected if it is set as such. If the environment's PHP CLI max_execution_time is in fact set to 0, `drupal_set_time_limit` will actually impose a max execution time and constrain the process itself from completing.

Proposed resolution

Adapt the implementation of the `drupal_set_time_limit` to perform a check on the currently set max_execution_time, and if the current maximum execution time is greater then zero, then perform the `@set_time_limit` in which the time limit can be imposed.

Remaining tasks

  1. Write test
CommentFileSizeAuthor
#6 2212033-6-drupal_set_time_limit_allow_unlimited-D8.patch624 byteshelmo
#6 2212033-6-drupal_set_time_limit_allow_unlimited-D7.patch605 byteshelmo
#4 2212033-drupal_set_time_limit_allow_unlimited-D7.patch605 bytesamcgowanca
#1 2212033-drupal_set_time_limit_allow_unlimited-D7.patch604 bytesamcgowanca

Comments

amcgowanca’s picture

amcgowanca commented
StatusFileSize
new 2212033-drupal_set_time_limit_allow_unlimited-D7.patch604 bytes
markpavlitski’s picture

markpavlitski commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: 2212033-drupal_set_time_limit_allow_unlimited-D7.patch, failed testing.

amcgowanca’s picture

amcgowanca commented
Status: Needs work » Needs review
StatusFileSize
new 2212033-drupal_set_time_limit_allow_unlimited-D7.patch605 bytes

Re-rolled patch with additional empty line at end of file.

amcgowanca’s picture

amcgowanca commented

I also think that this patch would apply to Drupal 8 as well, given that D8's implementation is identical.

helmo’s picture

helmo commented
Version: 7.x-dev » 8.x-dev
StatusFileSize
new 2212033-6-drupal_set_time_limit_allow_unlimited-D7.patch605 bytes
new 2212033-6-drupal_set_time_limit_allow_unlimited-D8.patch624 bytes

Then here's a D8 patch to get the ball rolling. As @amcgowanca mentioned the patch applied cleanly to D8, only a shift in line number.

I propose to write $current_time_limit > 0 instead of 0 < $current_time_limit ... but that's just a minor thing.

The last submitted patch, 6: 2212033-6-drupal_set_time_limit_allow_unlimited-D7.patch, failed testing.

amcgowanca’s picture

amcgowanca commented

@helmo thanks for submitting!

helmo’s picture

helmo commented
Related issues: +#174617: Environment::setTimeLimit() ignores current value, can reduce timeout value

I expect that this gets a Needs tests tag ... so how could we test this?

  1. set_time_limit(0)
  2. drupal_set_time_limit(240)
  3. Assert that the timelimit is stil 0

From a ... standpoint changing the function name to drupal_raise_time_limit() would make it more clear. I'm not aware of any usecase for lowering the time_limit but that could be a separate function.

markpavlitski’s picture

markpavlitski commented
Related issues: -#174617: Environment::setTimeLimit() ignores current value, can reduce timeout value

@helmo, at the moment this function doesn't ensure the limit is raised, only that it's not changing from 0 (unlimited) to something else. i.e.:

drupal_set_time_limit(300);
...
drupal_set_time_limit(30);

Would result in a limit of 30, not 300.

Also, what is the purpose of this check?

if (FALSE !== $current_time_limit) {
  ...
}
markpavlitski’s picture

markpavlitski commented
Related issues: +#174617: Environment::setTimeLimit() ignores current value, can reduce timeout value

Restoring related issues.

helmo’s picture

helmo commented
Issue summary: View changes
markpavlitski’s picture

markpavlitski commented
Issue tags: +Needs tests

The name change makes sense, as long as that's what it does. I.e. you can increase the limit or set to zero.

Agreed that test a test case should be included.

markpavlitski’s picture

markpavlitski commented
Status: Needs review » Needs work
damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs work » Closed (duplicate)

This is a duplicate of #174617: Environment::setTimeLimit() ignores current value, can reduce timeout value.