Moodle
  1. Moodle
  2. MDL-31553

max_execution_time lowered during command line upgrade

    Details

    • Database:
      MySQL
    • Testing Instructions:
      Hide

      Create a test script in the root of the Moodle installation with the following code. Run it with the PHP command line interpreter. Should output "Pass" if the upgrade_set_timeout function does not alter the default max_execution_time.

      <?php
      define('CLI_SCRIPT', true);
      require_once 'config.php';
      $CFG->upgraderunning = time() + 3600;
      upgrade_set_timeout();
      echo ini_get('max_execution_time') == 0 ? "Pass" : "Fail";
      unset_config('upgraderunning'); // Clean up

      Show
      Create a test script in the root of the Moodle installation with the following code. Run it with the PHP command line interpreter. Should output "Pass" if the upgrade_set_timeout function does not alter the default max_execution_time. <?php define('CLI_SCRIPT', true); require_once 'config.php'; $CFG->upgraderunning = time() + 3600; upgrade_set_timeout(); echo ini_get('max_execution_time') == 0 ? "Pass" : "Fail"; unset_config('upgraderunning'); // Clean up
    • Workaround:
      Hide

      Running command line script with set_time_limit disabled may help:

      php -d disable_functions=set_time_limit upgrade.php

      Show
      Running command line script with set_time_limit disabled may help: php -d disable_functions=set_time_limit upgrade.php
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w17_MDL-31553_m23_clitimeout
    • Rank:
      38108

      Description

      During long-running command line upgrades (particularly from Moodle 1.9 to 2) it is possible to get the following error: "Fatal error: Maximum execution time of 300 seconds exceeded". This should not happen as the PHP CLI interface is hard-coded to set max_execution_time to 0 (unlimited) regardless of the php.ini setting.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting that and providing a patch.

          We might want to check the constant is defined instead of, or at least before, checking its value.

          Show
          Michael de Raadt added a comment - Thanks for reporting that and providing a patch. We might want to check the constant is defined instead of, or at least before, checking its value.
          Hide
          Michael Aherne added a comment -

          Thanks for your comment, Michael. I did originally use "defined", but afterwards noticed that it's done this way elsewhere in the same file, so changed it for consistency.

          I wonder if this should be raised as a separate issue? A quick search of the codebase gives 25 occurrences of "if (CLI_SCRIPT", but only 1 of "defined('CLI_SCRIPT" so it seems to be standard in Moodle to do it this way.

          Show
          Michael Aherne added a comment - Thanks for your comment, Michael. I did originally use "defined", but afterwards noticed that it's done this way elsewhere in the same file, so changed it for consistency. I wonder if this should be raised as a separate issue? A quick search of the codebase gives 25 occurrences of "if (CLI_SCRIPT", but only 1 of "defined('CLI_SCRIPT" so it seems to be standard in Moodle to do it this way.
          Hide
          Petr Škoda added a comment -

          ah, I did not notice your patch and created a new one that explicitly set the timeout to 0 for CLI scripts, thanks for the report and patch!!

          to integrators: please cherry pick to supported stable branches

          Show
          Petr Škoda added a comment - ah, I did not notice your patch and created a new one that explicitly set the timeout to 0 for CLI scripts, thanks for the report and patch!! to integrators: please cherry pick to supported stable branches
          Hide
          Dan Poltawski added a comment -

          Nice improvement.

          Show
          Dan Poltawski added a comment - Nice improvement.
          Hide
          Dan Poltawski added a comment -

          Petr,

          this does not cleanly cherry-pick to other branches, please could produce branches, thanks!

          Show
          Dan Poltawski added a comment - Petr, this does not cleanly cherry-pick to other branches, please could produce branches, thanks!
          Hide
          Petr Škoda added a comment -

          oops, I moved the file last week, fixed!

          Show
          Petr Škoda added a comment - oops, I moved the file last week, fixed!
          Hide
          Dan Poltawski added a comment -

          Thanks, this has been integrated now!

          Show
          Dan Poltawski added a comment - Thanks, this has been integrated now!
          Hide
          Michael Aherne added a comment - - edited

          Hi Petr & Dan. Thanks for fixing this!

          There's a bit of a problem though - there's another bit of code in the same function which appears to be "manually" timing out even though max_execution_time is set to zero (wish I'd noticed this when I first reported the issue!). I've reported it as MDL-32607.

          Show
          Michael Aherne added a comment - - edited Hi Petr & Dan. Thanks for fixing this! There's a bit of a problem though - there's another bit of code in the same function which appears to be "manually" timing out even though max_execution_time is set to zero (wish I'd noticed this when I first reported the issue!). I've reported it as MDL-32607 .
          Hide
          Rossiani Wijaya added a comment -

          This is working great.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working great. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This has been near becoming rejected, because it's not the best code you are able to produce.

          But, luckily, at the end, it has landed and has been spread to all repos out there.

          Many thanks and, don't forget it, keep improving your skills, you can!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This has been near becoming rejected, because it's not the best code you are able to produce. But, luckily, at the end, it has landed and has been spread to all repos out there. Many thanks and, don't forget it, keep improving your skills, you can! Closing, ciao

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: