Moodle
  1. Moodle
  2. MDL-38387

Caching of the site identifier is not robust enough quite yet.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.4.2, 2.5
    • Fix Version/s: 2.4.4
    • Component/s: Caching
    • Labels:
    • Rank:
      48297

      Description

      The cache now uses the site identifier as part of the hash prefixed to all keys.
      Within its handling it attempts to load the site identifier from the cache config. If its not there then the cache config tries to load it from the database config.
      The issue arises that if you write a cache definition that is used before the database cache then you can find yourself in a situation where the cache ends up in an infinite loop while trying to initialise.
      The cause of this is that the cache initialisation is trying to use the cache API. That's a no-no the cache API cannot use caches or we risk the infinite initalisation issue.

      There's a simple fix for this luckily.

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          Putting this up for peer-review now.

          Show
          Sam Hemelryk added a comment - Putting this up for peer-review now.
          Hide
          Sam Hemelryk added a comment -

          Hi David,

          I've added you as a watcher, the patch attached to this issue should solve the issues you have implementing the plugins cache presently.
          Would you mind giving it a shot.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi David, I've added you as a watcher, the patch attached to this issue should solve the issues you have implementing the plugins cache presently. Would you mind giving it a shot. Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          Submitting for integration, landing this sooner removes a roadblock.

          Show
          Sam Hemelryk added a comment - Submitting for integration, landing this sooner removes a roadblock.
          Hide
          Aparup Banerjee added a comment -

          not seeing the m25 branch

          Show
          Aparup Banerjee added a comment - not seeing the m25 branch
          Hide
          Aparup Banerjee added a comment -

          Thanks, thats been integrated.

          I did have one itch about the use of 'unknown'. I think it would've been better if it was a constant or something more globally recognisable as "i've no idea". or a boolean but i'm guessing there might've been a problem with that (mixed) ?

          Show
          Aparup Banerjee added a comment - Thanks, thats been integrated. I did have one itch about the use of 'unknown'. I think it would've been better if it was a constant or something more globally recognisable as "i've no idea". or a boolean but i'm guessing there might've been a problem with that (mixed) ?
          Hide
          Aparup Banerjee added a comment -

          btw, this could do with a unit test.
          ie: a test that revolves around stopping infinite loopy loops.

          Show
          Aparup Banerjee added a comment - btw, this could do with a unit test. ie: a test that revolves around stopping infinite loopy loops.
          Hide
          David Mudrak added a comment -

          Hi Sam. Thanks for working on this. I can roughly understand the situation, although I haven't looked too deep into the patch. I just commented in MDL-34401 that I was not actually able to reproduce the endless loop problem experienced there. For me, upgrading from master to MDL-34401-muc-pluginlib worked well even without this patch. So let us hope that Dan and/or David Mo will be able to pass their tests this time.

          Show
          David Mudrak added a comment - Hi Sam. Thanks for working on this. I can roughly understand the situation, although I haven't looked too deep into the patch. I just commented in MDL-34401 that I was not actually able to reproduce the endless loop problem experienced there. For me, upgrading from master to MDL-34401 -muc-pluginlib worked well even without this patch. So let us hope that Dan and/or David Mo will be able to pass their tests this time.
          Hide
          Michael de Raadt added a comment -

          I encountered some file errors related to caching, but I think these have been reported before...

          Warning: rmdir(D:\xampp\Data\master_integration_MySQL/cache/cachestore_file/default_application/core_config): Directory not empty in D:\xampp\htdocs\master_integration\lib\moodlelib.php on line 10750
          
          Warning: rmdir(D:\xampp\Data\master_integration_MySQL/cache/cachestore_file/default_application): Directory not empty in D:\xampp\htdocs\master_integration\lib\moodlelib.php on line 10750
          
          Warning: rmdir(D:\xampp\Data\master_integration_MySQL/cache/cachestore_file): Directory not empty in D:\xampp\htdocs\master_integration\lib\moodlelib.php on line 10750
          
          Show
          Michael de Raadt added a comment - I encountered some file errors related to caching, but I think these have been reported before... Warning: rmdir(D:\xampp\Data\master_integration_MySQL/cache/cachestore_file/default_application/core_config): Directory not empty in D:\xampp\htdocs\master_integration\lib\moodlelib.php on line 10750 Warning: rmdir(D:\xampp\Data\master_integration_MySQL/cache/cachestore_file/default_application): Directory not empty in D:\xampp\htdocs\master_integration\lib\moodlelib.php on line 10750 Warning: rmdir(D:\xampp\Data\master_integration_MySQL/cache/cachestore_file): Directory not empty in D:\xampp\htdocs\master_integration\lib\moodlelib.php on line 10750
          Hide
          Michael de Raadt added a comment -

          The warnings above have been reported in MDL-38350 and MDL-38273. I assume they are unrelated. I manually deleted the cachestore_file directory to be sure.

          Show
          Michael de Raadt added a comment - The warnings above have been reported in MDL-38350 and MDL-38273 . I assume they are unrelated. I manually deleted the cachestore_file directory to be sure.
          Hide
          Michael de Raadt added a comment - - edited

          I encountered this Moodle error after applying David's patch. Actually, this was on the fresh installation, so I'm not sure what it means.

          Cache definitions reparsed causing cache reset in order to locate definition. You should bump the version number to ensure definitions are reprocessed.
          
              line 411 of \cache\classes\factory.php: call to debugging()
              line 181 of \cache\classes\factory.php: call to cache_factory->create_definition()
              line 169 of \cache\classes\loaders.php: call to cache_factory->create_cache_from_definition()
              line 8186 of \lib\moodlelib.php: call to cache::make()
              line 8008 of \lib\moodlelib.php: call to get_plugin_types()
              line 8048 of \lib\moodlelib.php: call to get_plugin_directory()
              line 469 of \lib\outputrequirementslib.php: call to get_component_directory()
              line 528 of \lib\outputrequirementslib.php: call to page_requirements_manager->find_module()
              line 251 of \lib\outputrequirementslib.php: call to page_requirements_manager->js_module()
              line 600 of \lib\pagelib.php: call to page_requirements_manager->__construct()
              line 717 of \lib\pagelib.php: call to moodle_page->magic_get_requires()
              line 2762 of \lib\outputrenderers.php: call to moodle_page->__get()
              line 2737 of \lib\outputrenderers.php: call to core_renderer->render_custom_menu()
              line 13 of \theme\base\layout\general.php: call to core_renderer->custom_menu()
              line 835 of \lib\outputrenderers.php: call to include()
              line 765 of \lib\outputrenderers.php: call to core_renderer->render_page_layout()
              line ? of unknownfile: call to core_renderer->header()
              line 234 of \lib\outputrenderers.php: call to call_user_func_array()
              line 146 of \admin\renderer.php: call to plugin_renderer_base->__call()
              line 146 of \admin\renderer.php: call to core_admin_renderer->header()
              line 220 of \admin\index.php: call to core_admin_renderer->upgrade_confirm_page()
          
          Show
          Michael de Raadt added a comment - - edited I encountered this Moodle error after applying David's patch. Actually, this was on the fresh installation, so I'm not sure what it means. Cache definitions reparsed causing cache reset in order to locate definition. You should bump the version number to ensure definitions are reprocessed. line 411 of \cache\classes\factory.php: call to debugging() line 181 of \cache\classes\factory.php: call to cache_factory->create_definition() line 169 of \cache\classes\loaders.php: call to cache_factory->create_cache_from_definition() line 8186 of \lib\moodlelib.php: call to cache::make() line 8008 of \lib\moodlelib.php: call to get_plugin_types() line 8048 of \lib\moodlelib.php: call to get_plugin_directory() line 469 of \lib\outputrequirementslib.php: call to get_component_directory() line 528 of \lib\outputrequirementslib.php: call to page_requirements_manager->find_module() line 251 of \lib\outputrequirementslib.php: call to page_requirements_manager->js_module() line 600 of \lib\pagelib.php: call to page_requirements_manager->__construct() line 717 of \lib\pagelib.php: call to moodle_page->magic_get_requires() line 2762 of \lib\outputrenderers.php: call to moodle_page->__get() line 2737 of \lib\outputrenderers.php: call to core_renderer->render_custom_menu() line 13 of \theme\base\layout\general.php: call to core_renderer->custom_menu() line 835 of \lib\outputrenderers.php: call to include() line 765 of \lib\outputrenderers.php: call to core_renderer->render_page_layout() line ? of unknownfile: call to core_renderer->header() line 234 of \lib\outputrenderers.php: call to call_user_func_array() line 146 of \admin\renderer.php: call to plugin_renderer_base->__call() line 146 of \admin\renderer.php: call to core_admin_renderer->header() line 220 of \admin\index.php: call to core_admin_renderer->upgrade_confirm_page()
          Hide
          Michael de Raadt added a comment -

          I tested this in 2.4 and master.

          I'm not sure if David's patch made a difference.

          After the /dataroot/muc/config.php file was deleted, it was regenerated.

          I also commented out the site_identifier member variable from the configuration array in /dataroot/muc/config.php and the site kept functioning.

          I assume this constitutes a pass, but I hope Sam will get online to verify this. In the meantime I will leave this in the hands of Apu.

          Show
          Michael de Raadt added a comment - I tested this in 2.4 and master. I'm not sure if David's patch made a difference. After the /dataroot/muc/config.php file was deleted, it was regenerated. I also commented out the site_identifier member variable from the configuration array in /dataroot/muc/config.php and the site kept functioning. I assume this constitutes a pass, but I hope Sam will get online to verify this. In the meantime I will leave this in the hands of Apu.
          Hide
          David Monllaó added a comment -

          I've just finished running the testing instructions, following the testing instructions I can't see any issue; if I begin the installation process with MDL-34401 integrated I receive the Maximum function nesting level of '100' reached; using Xdebug v2.2.1 and PHP 5.4.12

          Show
          David Monllaó added a comment - I've just finished running the testing instructions, following the testing instructions I can't see any issue; if I begin the installation process with MDL-34401 integrated I receive the Maximum function nesting level of '100' reached ; using Xdebug v2.2.1 and PHP 5.4.12
          Hide
          David Mudrak added a comment -

          Michael: that "Cache definitions reparsed ..." message is not an error but debugging message IIRC. It is expected to see it AFAIK.

          David: Do you mean that if both this MDL-38387 and MDL-34401 are integrated, you still experience the problem? Or does it happen only without this MDL-38387?

          Show
          David Mudrak added a comment - Michael: that "Cache definitions reparsed ..." message is not an error but debugging message IIRC. It is expected to see it AFAIK. David: Do you mean that if both this MDL-38387 and MDL-34401 are integrated, you still experience the problem? Or does it happen only without this MDL-38387 ?
          Hide
          David Monllaó added a comment -

          David: yes, with MDL-38387 and MDL-34401 both integrated when I install from scratch (pgsql) I experience the problem. I haven't tried without MDL-38387

          Show
          David Monllaó added a comment - David: yes, with MDL-38387 and MDL-34401 both integrated when I install from scratch (pgsql) I experience the problem. I haven't tried without MDL-38387
          Hide
          David Mudrak added a comment -

          David: well, I think you tried MDL-34401 without MDL-38387 as it was your and Dan's fail test report in MDL-34401 that led to working on this MDL-38387.

          Crap, so we still have this issue. We need Sam Hemelryk here. If David Monllaó is right, the caching of site identifier was not the real cause of our troubles here.

          Show
          David Mudrak added a comment - David: well, I think you tried MDL-34401 without MDL-38387 as it was your and Dan's fail test report in MDL-34401 that led to working on this MDL-38387 . Crap, so we still have this issue. We need Sam Hemelryk here. If David Monllaó is right, the caching of site identifier was not the real cause of our troubles here.
          Hide
          Aparup Banerjee added a comment -

          failing this as it does effectively still block MDL-34401

          Show
          Aparup Banerjee added a comment - failing this as it does effectively still block MDL-34401
          Hide
          Aparup Banerjee added a comment -

          Hm, so David, you're saying the patch is good but the issue still exists ? what if we leave the patch in integration (as it helps) but leave this issue open? (+1s?)

          Show
          Aparup Banerjee added a comment - Hm, so David, you're saying the patch is good but the issue still exists ? what if we leave the patch in integration (as it helps) but leave this issue open? (+1s?)
          Hide
          David Monllaó added a comment -

          Apu: Sorry, I haven't gone into the code, I don't know the internals of each patch

          • What worked for me: The testing instructions followed step by step, applying the MDL-34401 after upgrading/installing
          • What didn't worked for me: Applying MDL-34401 on top of integration and installing from there

          I just repeated the same steps, attaching screenshot

          Show
          David Monllaó added a comment - Apu: Sorry, I haven't gone into the code, I don't know the internals of each patch What worked for me: The testing instructions followed step by step, applying the MDL-34401 after upgrading/installing What didn't worked for me: Applying MDL-34401 on top of integration and installing from there I just repeated the same steps, attaching screenshot
          Hide
          David Mudrak added a comment -

          Well. I believe that this MDL-38387 may pass testing as no problems related to it were found (see Michael de Raadt's comment above). It is actually MDL-34401 causing the problem, if I understand David Monllaó correctly. IMHO we can pass this and wait with MDL-34401 for Sam Hemelryk.

          Show
          David Mudrak added a comment - Well. I believe that this MDL-38387 may pass testing as no problems related to it were found (see Michael de Raadt 's comment above). It is actually MDL-34401 causing the problem, if I understand David Monllaó correctly. IMHO we can pass this and wait with MDL-34401 for Sam Hemelryk .
          Hide
          Aparup Banerjee added a comment -

          i don't see how the patch @ MDL-34401 is the culprit here. imo cache should prevent any loop caused by anywhere calling it. but yes, will wait for more input.
          +1 to keep patch in integration (its good to me).

          Show
          Aparup Banerjee added a comment - i don't see how the patch @ MDL-34401 is the culprit here. imo cache should prevent any loop caused by anywhere calling it. but yes, will wait for more input. +1 to keep patch in integration (its good to me).
          Hide
          Aparup Banerjee added a comment -

          sorry, i should clarify which David in my comments!

          Show
          Aparup Banerjee added a comment - sorry, i should clarify which David in my comments!
          Hide
          Michael de Raadt added a comment -

          Good work all.

          Show
          Michael de Raadt added a comment - Good work all.
          Hide
          Damyon Wiese added a comment - - edited

          I had a look at this to see if we could add a simple unit test to catch this case and prove the patch (I failed).

          Applying MDL-34401 triggered the error when installing behat.

          Summary from long investigation is:

          This code does not handle the case when $CFG->siteidentifier is false
          and
          the static class variable $siteidentifier in cache/classes/helper.php is NULL.

          I think in cache/classes/helper.php line 551 should be changed from:

                      // It's very important here that we don't use get_config. We don't want an endless cache loop!
                      if (isset($CFG->siteidentifier)) {
                          self::$siteidentifier = self::update_site_identifier($CFG->siteidentifier);
          

          to

                      // It's very important here that we don't use get_config. We don't want an endless cache loop!
                      if (!empty($CFG->siteidentifier)) {
                          self::$siteidentifier = self::update_site_identifier($CFG->siteidentifier);
          

          But I also think Sam should confirm this so I think we should fail and revert this for this week.

          Show
          Damyon Wiese added a comment - - edited I had a look at this to see if we could add a simple unit test to catch this case and prove the patch (I failed). Applying MDL-34401 triggered the error when installing behat. Summary from long investigation is: This code does not handle the case when $CFG->siteidentifier is false and the static class variable $siteidentifier in cache/classes/helper.php is NULL. I think in cache/classes/helper.php line 551 should be changed from: // It's very important here that we don't use get_config. We don't want an endless cache loop! if (isset($CFG->siteidentifier)) { self::$siteidentifier = self::update_site_identifier($CFG->siteidentifier); to // It's very important here that we don't use get_config. We don't want an endless cache loop! if (!empty($CFG->siteidentifier)) { self::$siteidentifier = self::update_site_identifier($CFG->siteidentifier); But I also think Sam should confirm this so I think we should fail and revert this for this week.
          Hide
          Aparup Banerjee added a comment -

          Thanks Damyon, that made this revert easier

          Show
          Aparup Banerjee added a comment - Thanks Damyon, that made this revert easier
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          David Mudrak added a comment -

          Just wondering - are we sure the Behat integration is clean? Such a question must come on table given that the patch seems to cause problems with behat only now...

          Show
          David Mudrak added a comment - Just wondering - are we sure the Behat integration is clean? Such a question must come on table given that the patch seems to cause problems with behat only now...
          Hide
          David Monllaó added a comment -

          David, I haven't tried with behat installer, with MDL-34401 and MDL-38387 applied I receive the segmentation fault (infinite loop) installing moodle through web UI and also through admin/cli/install_database.php

          Show
          David Monllaó added a comment - David, I haven't tried with behat installer, with MDL-34401 and MDL-38387 applied I receive the segmentation fault (infinite loop) installing moodle through web UI and also through admin/cli/install_database.php
          Hide
          Damyon Wiese added a comment -

          Just pinging Sam - as this issue is blocking some others and I would like his feedback on the suggested fix (especially as this issue includes 24).

          Show
          Damyon Wiese added a comment - Just pinging Sam - as this issue is blocking some others and I would like his feedback on the suggested fix (especially as this issue includes 24).
          Hide
          Sam Hemelryk added a comment -

          Thanks for all the work on this guys.

          The fix you've noted Damyon is an improvement but isn't the solution to the issue David experienced that I was able to replicate after a few attempts today.
          I've just pushed up a fixed branch and have asked David to test again.

          I've introduced another patch to remove a call to the plugins API from within the guts of the cache API.
          It was trying to load plugin information during initialisation and causing a loop situation.

          Hopefully all fixed now, will wait for David's findings.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Thanks for all the work on this guys. The fix you've noted Damyon is an improvement but isn't the solution to the issue David experienced that I was able to replicate after a few attempts today. I've just pushed up a fixed branch and have asked David to test again. I've introduced another patch to remove a call to the plugins API from within the guts of the cache API. It was trying to load plugin information during initialisation and causing a loop situation. Hopefully all fixed now, will wait for David's findings. Many thanks Sam
          Hide
          David Monllaó added a comment -

          Hi,

          Applying now git://github.com/samhemelryk/moodle.git wip-MDL-38387-m24 and git://github.com/mudrd8mz/moodle.git MDL-34401-muc-pluginlib with the same PHP parameters and extensions the installation process finishes as expected

          Show
          David Monllaó added a comment - Hi, Applying now git://github.com/samhemelryk/moodle.git wip- MDL-38387 -m24 and git://github.com/mudrd8mz/moodle.git MDL-34401 -muc-pluginlib with the same PHP parameters and extensions the installation process finishes as expected
          Hide
          Sam Hemelryk added a comment -

          Thanks for checking that out David, I've put it up for integration review now

          Show
          Sam Hemelryk added a comment - Thanks for checking that out David, I've put it up for integration review now
          Hide
          Damyon Wiese added a comment -

          Thanks Sam,

          Integrated to 2.4 and 2.5. I noticed about 1 second after I pushed it that the 25 branch had one commit with the wrong tracker code (MDL-38350).

          Show
          Damyon Wiese added a comment - Thanks Sam, Integrated to 2.4 and 2.5. I noticed about 1 second after I pushed it that the 25 branch had one commit with the wrong tracker code ( MDL-38350 ).
          Hide
          David Monllaó added a comment -

          It passes with with MDL-34401 also integrated.

          • Fresh 2.4 install
          • Upgrade from 2.4 to master
          Show
          David Monllaó added a comment - It passes with with MDL-34401 also integrated. Fresh 2.4 install Upgrade from 2.4 to master
          Hide
          Damyon Wiese added a comment -

          This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

          Thanks for your contributions!

          Show
          Damyon Wiese added a comment - This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads). Thanks for your contributions!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: