Moodle
  1. Moodle
  2. MDL-36466

Ballooning memory on install when dataroot not writeable

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Caching
    • Labels:
    • Testing Instructions:
      Hide
      1. Perform a fresh install through your browser.
      2. Perform a fresh install through CLI.
      3. Upgrade a 2.3 site in your browser.
      4. Run unit tests.
      5. Prepare to install a site (but don't run installation yet)
      6. Create the directory moodledata/muc and remove all permissions (sudo chmod -R 000 muc)
      7. Attempt the install.
      8. Check you get an error and not an timeout/max memory exceeded/max recursion error.
      Show
      Perform a fresh install through your browser. Perform a fresh install through CLI. Upgrade a 2.3 site in your browser. Run unit tests. Prepare to install a site (but don't run installation yet) Create the directory moodledata/muc and remove all permissions (sudo chmod -R 000 muc) Attempt the install. Check you get an error and not an timeout/max memory exceeded/max recursion error.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      wip-MDL-36466-m24
    • Rank:
      45302

      Description

      On Barbaras machine trying to do a fresh install:

      11:15 Fatal error: Allowed memory size of 4294967296 bytes exhausted (tried to allocate 71 bytes) in /Users/barbararamiro/Sites/integration/cache/locallib.php on line 68

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          Caused because dataroot was not writable. Still very much a blocker, I'll work on it shortly.

          Show
          Sam Hemelryk added a comment - Caused because dataroot was not writable. Still very much a blocker, I'll work on it shortly.
          Hide
          Barbara Ramiro added a comment -

          As Sam suggested, the problem came from a wrong permission in dataroot. For some reason the 'muc' directory had no write access. I did not create it manually so I am not sure of the reason...

          drwxrwsrwx  7 barbararamiro  staff  238  9 Nov 11:02 .
          drwxrwxrwx  4 barbararamiro  staff  136  8 Nov 17:06 ..
          -rw-rw-rw-  1 _www           staff  128  9 Nov 11:02 .htaccess
          drwxrwxrwx  2 _www           staff   68  9 Nov 11:02 cache
          drwxrwxrwx  2 _www           staff   68  9 Nov 11:02 lang
          d---------  2 _www           staff   68  8 Nov 17:14 muc
          drwxrwxrwx  2 _www           staff   68  9 Nov 11:02 temp
          

          Fred (using Barbie's computer)

          Show
          Barbara Ramiro added a comment - As Sam suggested, the problem came from a wrong permission in dataroot. For some reason the 'muc' directory had no write access. I did not create it manually so I am not sure of the reason... drwxrwsrwx 7 barbararamiro staff 238 9 Nov 11:02 . drwxrwxrwx 4 barbararamiro staff 136 8 Nov 17:06 .. -rw-rw-rw- 1 _www staff 128 9 Nov 11:02 .htaccess drwxrwxrwx 2 _www staff 68 9 Nov 11:02 cache drwxrwxrwx 2 _www staff 68 9 Nov 11:02 lang d--------- 2 _www staff 68 8 Nov 17:14 muc drwxrwxrwx 2 _www staff 68 9 Nov 11:02 temp Fred (using Barbie's computer)
          Hide
          Frédéric Massart added a comment -

          Actually, that was my mistake. Or pretty sure it was!

          When there was this issue with cache/lib.php not being included. I manually hacked the code and added a require_once() within the function that created the cache object. Which means the CFG was not set. I had then set the main variables manually and try again. What would have happened is that the directory had been created using $CFG->dirpermissions which was 0. Hence this weird permission...

          Sorry for wasting your time...

          Show
          Frédéric Massart added a comment - Actually, that was my mistake. Or pretty sure it was! When there was this issue with cache/lib.php not being included. I manually hacked the code and added a require_once() within the function that created the cache object. Which means the CFG was not set. I had then set the main variables manually and try again. What would have happened is that the directory had been created using $CFG->dirpermissions which was 0. Hence this weird permission... Sorry for wasting your time...
          Hide
          Sam Hemelryk added a comment -

          No probs Fred, it turned up exception handling issues in the cache API that we may not have found before release otherwise!

          I'll be implementing a two part fix on this issue:

          1. Better exception handling within cache initialisation.
          2. Disable cache during installation.
          Show
          Sam Hemelryk added a comment - No probs Fred, it turned up exception handling issues in the cache API that we may not have found before release otherwise! I'll be implementing a two part fix on this issue: Better exception handling within cache initialisation. Disable cache during installation.
          Hide
          Michael de Raadt added a comment -

          Closing this as it was a bug that arose when trying to overcome another bug.

          Show
          Michael de Raadt added a comment - Closing this as it was a bug that arose when trying to overcome another bug.
          Hide
          Aparup Banerjee added a comment -

          MDr: i don't think the linked bug will actually resolve this.

          Show
          Aparup Banerjee added a comment - MDr: i don't think the linked bug will actually resolve this.
          Hide
          Dan Poltawski added a comment -

          There is still a bug here!

          Show
          Dan Poltawski added a comment - There is still a bug here!
          Hide
          Sam Hemelryk added a comment -

          Hehe was just about to reopen to that effect with a comment explaining it

          While this issue came about because of another issue it has turned up an entirely new issue to do with the exception handling within cache.
          The new issue is leading to an infinite loop situation:

          1. Cache starts initialisation.
          2. Cache throws exception.
          3. Exception handler calls get_string for the message.
          4. get_string call the cache to initialise.
          5. Goto 1.
          Show
          Sam Hemelryk added a comment - Hehe was just about to reopen to that effect with a comment explaining it While this issue came about because of another issue it has turned up an entirely new issue to do with the exception handling within cache. The new issue is leading to an infinite loop situation: Cache starts initialisation. Cache throws exception. Exception handler calls get_string for the message. get_string call the cache to initialise. Goto 1.
          Hide
          Sam Hemelryk added a comment -

          Putting this up for peer-review.
          There are five commits making the following changes:

          1. improved handling of exceptions during initialisation.
          2. renamed define NO_CACHE_STORES to CACHE_DISABLE_STORES
          3. renamed disable to disable_stores and added tests
          4. implemented functionality to disable the bulk of the cache API
          5. disabled caching during installation and upgrade

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Putting this up for peer-review. There are five commits making the following changes: improved handling of exceptions during initialisation. renamed define NO_CACHE_STORES to CACHE_DISABLE_STORES renamed disable to disable_stores and added tests implemented functionality to disable the bulk of the cache API disabled caching during installation and upgrade Cheers Sam
          Hide
          Martin Dougiamas added a comment -

          Petr can you please peer review this?

          Show
          Martin Dougiamas added a comment - Petr can you please peer review this?
          Hide
          Petr Škoda added a comment -

          starting...

          Show
          Petr Škoda added a comment - starting...
          Hide
          Petr Škoda added a comment -

          Hi, I do not understand what you are trying to do with cache_factory::disable() in installer and upgrade code because I think we should use constants like CACHE_DISABLE_STORES before we require config.php or any other libraries - I can not see any other safe way to disable everything. cache_factory::disable() does not kill/disable existing instances, right?

          I expected to see 3 options:
          1/ CACHE_DISABLE_STORES which is used only before config.php, expected to be used in CLI installer, CLI upgrade scripts and first part of web installer
          2/ cache_factory::disable_all_stores() - disable all already created cache instances and new instances will be disabled
          3/ cache_factory::enable_all_stores() - enable all previously disabled stores and create any new stores the normal way

          The admin/index.php is tricky because it does installation, upgrade and normal UI at the same time. Maybe it is time to split the upgrade and installation to separate scripts (with redirects). For now cache_factory::disable_all_stores() could do the trick, but I think it would be better to execute lib/setup.php and friends without any caching once we start hacking the config tables.

          I am puzzled by the persistent stores, that requirement seems to collide with any disabling of such stores. Or is it all or nothing guaranteed to be stored? Another potential problem is request stores, previously we kept them working even in upgrade and install scripts for performance reasons. Maybe there could be some extra info in definition describing what should be done when disabling stores to let developers decide.

          Sorry for this general feedback, I hope it is not totally wrong or off-topic, I guess I need more time to understand all the potential problems here and the current design. I want to spend all time on this today and tomorrow if necessary. Please ping me once you get online on Monday, ciao.

          Show
          Petr Škoda added a comment - Hi, I do not understand what you are trying to do with cache_factory::disable() in installer and upgrade code because I think we should use constants like CACHE_DISABLE_STORES before we require config.php or any other libraries - I can not see any other safe way to disable everything. cache_factory::disable() does not kill/disable existing instances, right? I expected to see 3 options: 1/ CACHE_DISABLE_STORES which is used only before config.php, expected to be used in CLI installer, CLI upgrade scripts and first part of web installer 2/ cache_factory::disable_all_stores() - disable all already created cache instances and new instances will be disabled 3/ cache_factory::enable_all_stores() - enable all previously disabled stores and create any new stores the normal way The admin/index.php is tricky because it does installation, upgrade and normal UI at the same time. Maybe it is time to split the upgrade and installation to separate scripts (with redirects). For now cache_factory::disable_all_stores() could do the trick, but I think it would be better to execute lib/setup.php and friends without any caching once we start hacking the config tables. I am puzzled by the persistent stores, that requirement seems to collide with any disabling of such stores. Or is it all or nothing guaranteed to be stored? Another potential problem is request stores, previously we kept them working even in upgrade and install scripts for performance reasons. Maybe there could be some extra info in definition describing what should be done when disabling stores to let developers decide. Sorry for this general feedback, I hope it is not totally wrong or off-topic, I guess I need more time to understand all the potential problems here and the current design. I want to spend all time on this today and tomorrow if necessary. Please ping me once you get online on Monday, ciao.
          Hide
          Sam Hemelryk added a comment -

          Thanks Petr.

          We talked this morning I'm going to action the discussed points and get this ready for your review and testing again, hopefully in the next 24 hours.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Petr. We talked this morning I'm going to action the discussed points and get this ready for your review and testing again, hopefully in the next 24 hours. Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          Hi Petr,
          I've an updated branch available now.
          The following changes have been made:

          1. disable method is now protected and used only for unit tests.
          2. new define CACHE_DISABLE_ALL, disables everything, unused presently
          3. install and upgrade methods now disable stores before they start and then reset things once they're done.

          Keen to see what you think.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Petr, I've an updated branch available now. The following changes have been made: disable method is now protected and used only for unit tests. new define CACHE_DISABLE_ALL, disables everything, unused presently install and upgrade methods now disable stores before they start and then reset things once they're done. Keen to see what you think. Cheers Sam
          Hide
          Petr Škoda added a comment -

          Hi, I like the API much better now, thanks a lot. I was also playing with the installer and upgrades, borking permissions, deleting things randomly, etc. - so far works fine for me.

          +1

          Show
          Petr Škoda added a comment - Hi, I like the API much better now, thanks a lot. I was also playing with the installer and upgrades, borking permissions, deleting things randomly, etc. - so far works fine for me. +1
          Hide
          Sam Hemelryk added a comment -

          Awesome, thanks for looking at that Petr its much appreciated.
          Putting this up for integration now.

          Show
          Sam Hemelryk added a comment - Awesome, thanks for looking at that Petr its much appreciated. Putting this up for integration now.
          Hide
          Dan Poltawski added a comment -

          Integrated, thanks Sam.

          Show
          Dan Poltawski added a comment - Integrated, thanks Sam.
          Hide
          Rossiani Wijaya added a comment -

          Running Unit test for the site returned the following failures:

          There were 3 failures:
          
          1) dml_testcase::test_unique_index_collation_trouble
          Unique index is accent insensitive, this may cause problems for non-ascii languages. This is usually caused by accent insensitive default collation.
          
          /integration/cli/master/lib/dml/tests/dml_test.php:3633
          
          To re-run:
           /usr/bin/phpunit dml_testcase lib/dml/tests/dml_test.php
          
          2) dml_testcase::test_sql_binary_equal
          SQL operator "=" is expected to be case sensitive
          Failed asserting that 1 matches expected 2.
          
          /integration/cli/master/lib/dml/tests/dml_test.php:3661
          
          To re-run:
           /usr/bin/phpunit dml_testcase lib/dml/tests/dml_test.php
          
          3) grading_manager_testcase::test_tokenize
          A test using UTF-8 characters has failed. Consider updating PHP and PHP's PCRE or INTL extensions (MDL-30494)
          Failed asserting that false is true.
          
          /integration/cli/master/grade/grading/tests/lib_test.php:105
          /integration/cli/master/lib/phpunit/classes/advanced_testcase.php:76
          
          To re-run:
           /usr/bin/phpunit grading_manager_testcase grade/grading/tests/lib_test.php
          
          FAILURES!
          Tests: 1510, Assertions: 28313, Failures: 3.
          
          
          Show
          Rossiani Wijaya added a comment - Running Unit test for the site returned the following failures: There were 3 failures: 1) dml_testcase::test_unique_index_collation_trouble Unique index is accent insensitive, this may cause problems for non-ascii languages. This is usually caused by accent insensitive default collation. /integration/cli/master/lib/dml/tests/dml_test.php:3633 To re-run: /usr/bin/phpunit dml_testcase lib/dml/tests/dml_test.php 2) dml_testcase::test_sql_binary_equal SQL operator "=" is expected to be case sensitive Failed asserting that 1 matches expected 2. /integration/cli/master/lib/dml/tests/dml_test.php:3661 To re-run: /usr/bin/phpunit dml_testcase lib/dml/tests/dml_test.php 3) grading_manager_testcase::test_tokenize A test using UTF-8 characters has failed. Consider updating PHP and PHP's PCRE or INTL extensions (MDL-30494) Failed asserting that false is true . /integration/cli/master/grade/grading/tests/lib_test.php:105 /integration/cli/master/lib/phpunit/classes/advanced_testcase.php:76 To re-run: /usr/bin/phpunit grading_manager_testcase grade/grading/tests/lib_test.php FAILURES! Tests: 1510, Assertions: 28313, Failures: 3.
          Hide
          Rossiani Wijaya added a comment -

          For step 8: I received "Unable to save the cache config to file." message.

          I think that is the correct behavior.

          Hi Sam,

          Could you verify the error for phpunit test and let me know if it should be fixed on different issue?

          Thanks

          Show
          Rossiani Wijaya added a comment - For step 8: I received "Unable to save the cache config to file." message. I think that is the correct behavior. Hi Sam, Could you verify the error for phpunit test and let me know if it should be fixed on different issue? Thanks
          Hide
          Matteo Scaramuccia added a comment -

          Hi,
          IMHO those errors come from using a MySQL database with a collation like utf8_unicode_ci.

          HTH,
          Matteo

          Show
          Matteo Scaramuccia added a comment - Hi, IMHO those errors come from using a MySQL database with a collation like utf8_unicode_ci . HTH, Matteo
          Hide
          Dan Poltawski added a comment -

          Agreed about unit tests, please see: http://docs.moodle.org/dev/Common_unit_test_problems

          (The CI servers checked the tests and all looking good there).

          Show
          Dan Poltawski added a comment - Agreed about unit tests, please see: http://docs.moodle.org/dev/Common_unit_test_problems (The CI servers checked the tests and all looking good there).
          Hide
          Sam Hemelryk added a comment -

          Hi Rosie,

          That is the expected result for step 8.
          Certainly those unit tests are as Matteo and Dan have pointed out due to other factors.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Rosie, That is the expected result for step 8. Certainly those unit tests are as Matteo and Dan have pointed out due to other factors. Many thanks Sam
          Hide
          Rossiani Wijaya added a comment -

          Thank you for the feedback guys.

          I'm passing this test.

          Test passed.

          Show
          Rossiani Wijaya added a comment - Thank you for the feedback guys. I'm passing this test. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Y E S !

          Closing as fixed, many thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Y E S ! Closing as fixed, many thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: