Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-56748

Memory leak in MUC reset

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 3.0.6, 3.1.2, 3.2
    • Fix Version/s: 3.0.7, 3.1.3
    • Component/s: Caching
    • Labels:
    • Testing Instructions:
      Hide
      All branches

      You will need to compare the results both before, and after this patch is applied.

      1. Open cache/classes/factory.php in your editor and find the create_store_from_config function. Add the following immediately before the return:

        error_log("\nCount: " . array_sum(array_map('count', $this->definitionstores)));
        

      2. Run unit tests and compare
        1. The 'before' run has a constantly increasing count, whilst the 'after' count resets regularly
      Master only

      You will need to compare the results both before, and after this patch is applied.
      Note, you may not be able to complete the tests before the patch is applied.

      1. Start a redis-server and note the PID
      2. Ensure you have the redis extension for php installed
      3. Add the following to your config.php:

        define('TEST_CACHESTORE_REDIS_TESTSERVERS', '127.0.0.1');
        define('TEST_CACHE_USING_APPLICATION_STORE' , 'redis');
        

      4. Perform a complete unit test run
      5. While it is running, find the pid of the PHP process, and run the following loop:

        // Find the pid:
        ps -ef | grep phpunit | grep -v grep
        // Keep an eye on open files:
        while true; do echo "==="; lsof -p $pid_redis | wc -l; lsof -p $pid_phpunit | wc -l; sleep 1; done
        

        1. Confirm that the 'after' run stays reasonably stable whilst the 'before' run goes crazy
      Show
      All branches You will need to compare the results both before, and after this patch is applied. Open cache/classes/factory.php in your editor and find the create_store_from_config function. Add the following immediately before the return : error_log("\nCount: " . array_sum(array_map('count', $this->definitionstores))); Run unit tests and compare The 'before' run has a constantly increasing count, whilst the 'after' count resets regularly Master only You will need to compare the results both before, and after this patch is applied. Note, you may not be able to complete the tests before the patch is applied. Start a redis-server and note the PID Ensure you have the redis extension for php installed Add the following to your config.php: define('TEST_CACHESTORE_REDIS_TESTSERVERS', '127.0.0.1'); define('TEST_CACHE_USING_APPLICATION_STORE' , 'redis'); Perform a complete unit test run While it is running, find the pid of the PHP process, and run the following loop: // Find the pid: ps -ef | grep phpunit | grep -v grep // Keep an eye on open files: while true; do echo "==="; lsof -p $pid_redis | wc -l; lsof -p $pid_phpunit | wc -l; sleep 1; done Confirm that the 'after' run stays reasonably stable whilst the 'before' run goes crazy
    • Affected Branches:
      MOODLE_30_STABLE, MOODLE_31_STABLE, MOODLE_32_STABLE
    • Fixed Branches:
      MOODLE_30_STABLE, MOODLE_31_STABLE
    • Pull Master Branch:
      MDL-56748-master

      Description

      While testing MDL-48468, it was observed that there was some memory leak.
      After debugging Andrew Nicols found that the cache store instances were not been cleaned while purging cache.

      Comments from MDL-48468:
      Essentially it boils down to the fact that the our unit testing framework purges the cache, and it also removes the cache definitions, but it does not actually remove the cache store instances for those definitions.
      Every time a cache is created (e.g. via cache::make), an instance of a cache_store is created and stored (technically it's cloned, but that's not the point).
      The newly created cache_store instance is then stored in an anonymous array at cache_factory::$definitionstores.
      This is only used to facilitate fetching of all definition stores via get_store_instances_in_use. Items are never fetched out of the array again and there are no uses of this function in core.
      Between unit tests we purge all caches (cache_helper::purge_all()) and we then reset the cache (cache_factory::reset().
      The cache_helper::purge_all() function purges the caches, but does nothing more.
      The cache_factory::reset() function empties all of the base cache_store instances, the definitions, the config, the actual caches from definitions, but it does not remove the definition stores.
      As a result, on the next test after a purge, we create a new store, and a new definition store, based on the definition. Since we cannot actually fetch the store from cache_factory::$definitionstores, we append the new store to the array.
      As a result cache_factory::$definitionstores grows in size every time a test requires resetAfterTest and the number of items in the array hit a total of 23,511 in my testing.
      From my local testing of my solution to this issue, phpunit memory consumption on php7 drops by 70MB.

        Attachments

          Issue Links

            Activity

              People

              • Assignee:
                dobedobedoh Andrew Nicols
                Reporter:
                rajeshtaneja Rajesh Taneja
                Peer reviewer:
                Rajesh Taneja
                Integrator:
                David Monllaó
                Tester:
                Jun Pataleta
                Participants:
                Component watchers:
                Matteo Scaramuccia, Amaia Anabitarte, Bas Brands, Carlos Escobedo, Sara Arjona (@sarjona), Víctor Déniz Falcón
              • Votes:
                2 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/Nov/16