Uploaded image for project: '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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore 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
            salvetore 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
            maherne 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
            maherne 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            poltawski Dan Poltawski added a comment -

            Nice improvement.

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

            Petr,

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

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

            oops, I moved the file last week, fixed!

            Show
            skodak Petr Skoda added a comment - oops, I moved the file last week, fixed!
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks, this has been integrated now!

            Show
            poltawski Dan Poltawski added a comment - Thanks, this has been integrated now!
            Hide
            maherne 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
            maherne 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
            rwijaya Rossiani Wijaya added a comment -

            This is working great.

            Test passed.

            Show
            rwijaya Rossiani Wijaya added a comment - This is working great. Test passed.
            Hide
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  14/May/12