Moodle
  1. Moodle
  2. MDL-37545

If a cache is used during the initialisation or updating of the cache config file you get a fatal error

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.4.1, 2.5
    • Fix Version/s: 2.4.2
    • Component/s: Caching
    • Labels:
    • Testing Instructions:
      Hide
      1. Upgrade a site.
      2. Perform a fresh install.
      3. Repeat both through CLI.
      4. Run unit tests.
      5. Apply the patch attached to this file called "testing.patch"
      6. Browse to your site.
      7. You'll get a debug notice about missing definition and you should have bumped version. Ignore that.
      8. On the first page you browse you should get a "adhoc/plugintypes"
      9. Refresh the page.
      10. Make sure you get a "core/plugintypes"
      11. Make sure you get no errors.
      Show
      Upgrade a site. Perform a fresh install. Repeat both through CLI. Run unit tests. Apply the patch attached to this file called "testing.patch" Browse to your site. You'll get a debug notice about missing definition and you should have bumped version. Ignore that. On the first page you browse you should get a "adhoc/plugintypes" Refresh the page. Make sure you get a "core/plugintypes" Make sure you get no errors.
    • Workaround:
      Hide

      Dance like you want to.

      Show
      Dance like you want to.
    • Difficulty:
      Difficult
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      wip-MDL-37545-m25

      Description

      As quoted from David Mudrak's comment on MDL-34224:

      Sam, I have a question for you. While working on MDL-34401 I am trying to implement a cache to be used inside get_plugin_list(). According to my experiments, it decreases the Wall time spent in this function (as measured by xhprof) significantly. Using an ad-hoc cache worked great. So I wanted to convert it to a cache with proper definition. But ... The problem is that get_plugin_list() is called very early in the setup process and I can't find a way how to detect the case that the cache's description has not been registered yet. I am suspecting we will need to completely disable the whole MUC immediately after we realize that
      if ($version > $CFG->version) { // upgrade
      in admin/index.php.
      In other words, my problem is that I have a code
      $cache = cache::make('core', 'pluginlist');
      that gets called before the core registers the description of this cache (simply because the cache API uses this function while it's looking for descriptions...).
      Any ideas?

      This highlights an issue within the cache API.
      Internally it uses the plugin API in order to find definitions presented by plugins.
      A very necessary procedure.

      David above was looking to implement a plugins cache, a cache that would be very worth while. Because the cache uses the plugins API internally during initialisation unfortunately he ends up in a state where the cache get stuck in a loop continually trying to reinitialise itself.
      The problem comes because part way through the initialisation the plugins API is called, the plugins API makes a cache and as part of that process we try to initialise again.

      The solution to this I believe is to make use of an ad-hoc cache if a cache has been requested that we don't know about while the cache API itself is currently initialising.
      In order to implement this we'll need a new state "updating" and better handling of requests to create a cache (this is where initialisation starts to happen).

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Sam Hemelryk added a comment -

            Marked as a blocker as its hindering David's work

            Show
            Sam Hemelryk added a comment - Marked as a blocker as its hindering David's work
            Hide
            Sam Hemelryk added a comment -

            Putting this up for peer-review.

            David and Martin I've added you as watchers here.
            David perhaps you'd be so kind as to peer-review this. I'd be very interested to make sure it works with your development of the plugins cache as well.
            I had a look at your github in case it was up there already but as it wasn't (no probs at all) I created enough of a mock up to test this.

            Many thanks
            Sam

            Show
            Sam Hemelryk added a comment - Putting this up for peer-review. David and Martin I've added you as watchers here. David perhaps you'd be so kind as to peer-review this. I'd be very interested to make sure it works with your development of the plugins cache as well. I had a look at your github in case it was up there already but as it wasn't (no probs at all) I created enough of a mock up to test this. Many thanks Sam
            Hide
            Damyon Wiese added a comment -

            Reading the comments - I'll leave this for David to review with his plugin cache.

            Show
            Damyon Wiese added a comment - Reading the comments - I'll leave this for David to review with his plugin cache.
            Hide
            Damyon Wiese added a comment -

            Sorry for the noise - putting this back in "waiting for peer review" state.

            Show
            Damyon Wiese added a comment - Sorry for the noise - putting this back in "waiting for peer review" state.
            Hide
            David Mudrak added a comment -

            My pleasure to peer-review this.

            Show
            David Mudrak added a comment - My pleasure to peer-review this.
            Hide
            David Mudrak added a comment -

            Sam, I like the logic of the patch. I am going to cherry-pick it into my dev branch. I will let you know the results.

            Show
            David Mudrak added a comment - Sam, I like the logic of the patch. I am going to cherry-pick it into my dev branch. I will let you know the results.
            Hide
            David Mudrak added a comment -

            Yay! With the patch cherry-picked, I am getting the error no more. Just FYI I have not spotted any debugging message nor the adhoc cache usage before the upgrade but maybe I just overlooked something.

            Anyway, thanks for the patch! Please let me know if you are about to modify it yet so I would un-pick it from my branch before the rebase. And thanks for the attached testing.patch - it made me a bit more confident that I'm getting things right

            Show
            David Mudrak added a comment - Yay! With the patch cherry-picked, I am getting the error no more. Just FYI I have not spotted any debugging message nor the adhoc cache usage before the upgrade but maybe I just overlooked something. Anyway, thanks for the patch! Please let me know if you are about to modify it yet so I would un-pick it from my branch before the rebase. And thanks for the attached testing.patch - it made me a bit more confident that I'm getting things right
            Hide
            Sam Hemelryk added a comment -

            Thanks for reviewing and testing it with your work David, most appreciated!

            Re: not seeing the ad-hoc or a developer warning I suppose it is worth pointing out that ad-hoc would have appeared on the very first page that you viewed with both this patch and your patch applied and only if your patch had never been applied before.
            After the first load it will have the definition and will never be shown again.
            Perhaps you were using a site where your patch had already been applied, or just missed seeing it on the first page... hehe either way sounds like a success here

            Putting this up for integration now.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Thanks for reviewing and testing it with your work David, most appreciated! Re: not seeing the ad-hoc or a developer warning I suppose it is worth pointing out that ad-hoc would have appeared on the very first page that you viewed with both this patch and your patch applied and only if your patch had never been applied before. After the first load it will have the definition and will never be shown again. Perhaps you were using a site where your patch had already been applied, or just missed seeing it on the first page... hehe either way sounds like a success here Putting this up for integration now. Cheers Sam
            Hide
            David Mudrak added a comment -

            For the record, I was finally able to see both developer warning and the ad-hoc temporary cache created. Works like a charm. Very elegant solution, Sam. Thanks.

            Show
            David Mudrak added a comment - For the record, I was finally able to see both developer warning and the ad-hoc temporary cache created. Works like a charm. Very elegant solution, Sam. Thanks.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Integrated (24 & master), thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Integrated (24 & master), thanks!
            Hide
            David Monllaó added a comment -

            It works as expected, tested in master with define('MDL_PERF*, true);

            Show
            David Monllaó added a comment - It works as expected, tested in master with define('MDL_PERF*, true);
            Hide
            Eloy Lafuente (stronk7) added a comment -

            A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed.

            (and won't be revisiting it unless some regression is found)

            Thanks and ciao

            Show
            Eloy Lafuente (stronk7) added a comment - A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed. (and won't be revisiting it unless some regression is found) Thanks and ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: