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

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

    Details

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

      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.

        Gliffy Diagrams

          Activity

          Hide
          samhemelryk Sam Hemelryk added a comment -

          Putting this straight up for peer-review now.

          Show
          samhemelryk Sam Hemelryk added a comment - Putting this straight up for peer-review now.
          Hide
          samhemelryk 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
          samhemelryk 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
          samhemelryk 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
          samhemelryk 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
          poltawski 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
          poltawski 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
          samhemelryk 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
          samhemelryk 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
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Warn, conflicting!

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

          Integrated (24 & master), thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (24 & master), thanks!
          Hide
          abgreeve Adrian Greeve added a comment -

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

          Show
          abgreeve Adrian Greeve added a comment - Tested on integration master and 2.4. No errors found. Test passed.
          Hide
          stronk7 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
          stronk7 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:
                Fix Release Date:
                11/Mar/13