Moodle
  1. Moodle
  2. MDL-38205

memcache plugin should prefix a moodle specific tag to the keys before sending and retrieving.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.4.1, 2.5
    • Fix Version/s: 2.4.2
    • Component/s: Caching
    • Labels:
    • Rank:
      48041

      Description

      This is a memcache specific issue as memcached provides the prefix option.

      There have been reports of our cache keys colliding with Drupal cache keys when both are set to use the same memcache instance.
      Without confirming it I believe it makes good sense to include a Moodle specific prefix to avoid such collision with any software.
      We hash our keys as do many others so no doubt there is the chance for collisions.

        Activity

        Hide
        Sam Hemelryk added a comment -

        Putting this straight up for peer-review now.

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

        Actually changed my mind straight up for integration review, I'm confident with this fix and no one is looking at the peer-review queue presently.

        Show
        Sam Hemelryk added a comment - Actually changed my mind straight up for integration review, I'm confident with this fix and no one is looking at the peer-review queue presently.
        Hide
        Sam Hemelryk added a comment -

        Integrators while this could be viewed as in improvement I think it should be considered a bug and backported.
        Such a change ensures we don't get crazy reports about Moodle messing with the cache data of other software and vice-versa.

        Show
        Sam Hemelryk added a comment - Integrators while this could be viewed as in improvement I think it should be considered a bug and backported. Such a change ensures we don't get crazy reports about Moodle messing with the cache data of other software and vice-versa.
        Hide
        Dan Poltawski added a comment -

        Sam, I just saw this and had some comments - is there a max memcached key length.. maybe we should make this configurable?

        Show
        Dan Poltawski added a comment - Sam, I just saw this and had some comments - is there a max memcached key length.. maybe we should make this configurable?
        Hide
        Sam Hemelryk added a comment -

        Hi Dan,

        This is the situation with the key length for memcache.

        • The Memcache backend allows a max key length of 250 chars - configurable to 255.
        • The memcache php extension silently truncates any key to 250 chars. Epically evil if we get there.
        • The memcached extension fails to set an item with a key longer than 250.

        As for how we handle things the length of the key depends upon whether the definition uses simplekeys or not.
        If the definition doesn't use simple keys (the default) we use sha1 to hash the key before sending it to the backend. The key length would be 40 + the prefix added by this patch. Resulting key length of 43.
        If the definition does use simple keys then the key length given to our memcache plugins would be variable, it would be 33 + the length of the key. As the memcache plugin would now add a prefix as well that would be 36 + the length of the key. Any keys more then 214 chars long would not work.

        There is improvements that could be made there, really it would be nice to handle the key length in the backend plugins so that both our memcache and memcached plugins would deal with any keys over the length of 250.
        I'll patch this on both branches now.

        As for the setting it would be no trouble at all to turn it into a settings. I'll make that change now on the master branch only.

        Many thanks
        Sam

        Show
        Sam Hemelryk added a comment - Hi Dan, This is the situation with the key length for memcache. The Memcache backend allows a max key length of 250 chars - configurable to 255. The memcache php extension silently truncates any key to 250 chars. Epically evil if we get there. The memcached extension fails to set an item with a key longer than 250. As for how we handle things the length of the key depends upon whether the definition uses simplekeys or not. If the definition doesn't use simple keys (the default) we use sha1 to hash the key before sending it to the backend. The key length would be 40 + the prefix added by this patch. Resulting key length of 43. If the definition does use simple keys then the key length given to our memcache plugins would be variable, it would be 33 + the length of the key. As the memcache plugin would now add a prefix as well that would be 36 + the length of the key. Any keys more then 214 chars long would not work. There is improvements that could be made there, really it would be nice to handle the key length in the backend plugins so that both our memcache and memcached plugins would deal with any keys over the length of 250. I'll patch this on both branches now. As for the setting it would be no trouble at all to turn it into a settings. I'll make that change now on the master branch only. Many thanks Sam
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Warn, conflicting!

        Show
        Eloy Lafuente (stronk7) added a comment - Warn, conflicting!
        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
        Adrian Greeve added a comment -

        Tested on integration master and 2.4.
        No errors found.
        Test passed.

        Show
        Adrian Greeve added a comment - Tested on integration master and 2.4. No errors found. Test passed.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities.

        Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied).

        Thanks, closing as fixed!

        Show
        Eloy Lafuente (stronk7) added a comment - This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities. Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied). Thanks, closing as fixed!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: