Moodle
  1. Moodle
  2. MDL-38565

When a cache store is set to MODE_SESSION it should include the userid in the keys.

    Details

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

      Description

      Otherwise a site configured with a shared mem storage for the session cache will have a shared keyspace for all users which is not expected.

      Comments from Sam:
      It should be in the loader
      (10:30:09) samhemelryk@moodle.org: cache_session::parse_key should be doing it, but the code is not there ...

      ... write unit tests to test the stores explicitly for collision across all modes is perhaps the best way

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          Mega epic blocker - thanks for the report Damyon.

          Show
          Sam Hemelryk added a comment - Mega epic blocker - thanks for the report Damyon.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I believe I've got a solution for this now.
          Re-designed the session loader pretty much but it all makes more sense now I think.
          Patch is rather large Eloy or Damyon, if pos could you take a look?

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I believe I've got a solution for this now. Re-designed the session loader pretty much but it all makes more sense now I think. Patch is rather large Eloy or Damyon, if pos could you take a look? Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          I've added a 24 branch as well. Simple cherry-pick with two minor collisions, cache/tests/cache_test.php and cache/README.php.
          Both due to new features in 2.5.

          Show
          Sam Hemelryk added a comment - I've added a 24 branch as well. Simple cherry-pick with two minor collisions, cache/tests/cache_test.php and cache/README.php. Both due to new features in 2.5.
          Hide
          Damyon Wiese added a comment - - edited

          Hi Sam,

          Thanks for the patch (lots of reading). Your performance tests show that this new code is faster which is great.

          Minor things:

          1. double ;; in cache/classes/definition.php

          2. In session_cache::delete and session_cache::delete_many there is a slight difference to parent class (!empty($this->get_loader()) verses $this->get_loader() !== false (I prefer !empty)

          3. (mentioned on chat) MUST_EXUST in cache_phpunit_tests (kiwi accent typo)

          4. whitespace in lib/db/upgrade.php

          Improvement suggestions:

          would prefer the function call before the foreach here (cache/classes/factory.php):
          + // Initialise all of the stores used for that definition.
          + foreach (self::initialise_cachestore_instances($stores, $definition) as $store) {

          session_cache has a few functions (get and delete in particular) that are 95% same as the parent class - it would be nice to remove that duplication if possible (and reasonable).

          Other things:

          (Discussed on chat about adding sessionid to the keys for the session cache and modifying check_tracked_user to only purge the cache of keys matching the session id).
          Note: I tried modifying a unit test to detect this but couldn't because it requires separate sessions.

          Show
          Damyon Wiese added a comment - - edited Hi Sam, Thanks for the patch (lots of reading). Your performance tests show that this new code is faster which is great. Minor things: 1. double ;; in cache/classes/definition.php 2. In session_cache::delete and session_cache::delete_many there is a slight difference to parent class (!empty($this->get_loader()) verses $this->get_loader() !== false (I prefer !empty) 3. (mentioned on chat) MUST_EXUST in cache_phpunit_tests (kiwi accent typo) 4. whitespace in lib/db/upgrade.php Improvement suggestions: would prefer the function call before the foreach here (cache/classes/factory.php): + // Initialise all of the stores used for that definition. + foreach (self::initialise_cachestore_instances($stores, $definition) as $store) { session_cache has a few functions (get and delete in particular) that are 95% same as the parent class - it would be nice to remove that duplication if possible (and reasonable). Other things: (Discussed on chat about adding sessionid to the keys for the session cache and modifying check_tracked_user to only purge the cache of keys matching the session id). Note: I tried modifying a unit test to detect this but couldn't because it requires separate sessions.
          Hide
          Sam Hemelryk added a comment -

          Thanks for looking this over Damyon, I've made the changes noted and am pushing this up for integration now.

          For the integrator - performance comparisons can be found here:
          Comparison of master vs this patch applied with default settings: http://moodev.com/?w=300&h=150&before=master-2013-04-18.01&after=MDL-38565-2013-04-18.01&x=1&o=filesincluded
          Comparison of master vs this patch applied and a file store being used to store session data: http://moodev.com/?w=300&h=150&before=master-2013-04-18.01&after=MDL-38565-alt-2013-04-18.01&o=filesincluded

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Thanks for looking this over Damyon, I've made the changes noted and am pushing this up for integration now. For the integrator - performance comparisons can be found here: Comparison of master vs this patch applied with default settings: http://moodev.com/?w=300&h=150&before=master-2013-04-18.01&after=MDL-38565-2013-04-18.01&x=1&o=filesincluded Comparison of master vs this patch applied and a file store being used to store session data: http://moodev.com/?w=300&h=150&before=master-2013-04-18.01&after=MDL-38565-alt-2013-04-18.01&o=filesincluded Many thanks Sam
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Dan Poltawski added a comment -

          Integrated to master and 24, thanks Sam

          Show
          Dan Poltawski added a comment - Integrated to master and 24, thanks Sam
          Hide
          Adrian Greeve added a comment -

          Tested on 2.4 and master integration.
          I ran the unit tests multiple times, upgraded the sites, created new installs, enabled all of the front page settings, switched between different users, and ran performance tests.
          No problems found.
          Test passed.

          Show
          Adrian Greeve added a comment - Tested on 2.4 and master integration. I ran the unit tests multiple times, upgraded the sites, created new installs, enabled all of the front page settings, switched between different users, and ran performance tests. No problems found. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I feel myself really alone tonight! So was time to push your fixes upstream!

          "Lest we forget. We will remember them."

          Thanks and ciao!

          Show
          Eloy Lafuente (stronk7) added a comment - I feel myself really alone tonight! So was time to push your fixes upstream! "Lest we forget. We will remember them." Thanks and ciao!
          Hide
          Matteo Scaramuccia added a comment -

          Hi Sam,
          great work and nice improvement! I got in touch with it thanks to the Integration round 2013-04-25 post: I'll take the time to learn it.
          BTW, there 8 typos (the same pattern, 8 times) in PHPDoc, do you think they should be "fixed"? I'm not native English i.e. I'm not sure if they'll "hurt" someone...:

          diff --git a/cache/classes/interfaces.php b/cache/classes/interfaces.php
          index 24996ed..b950260 100644
          --- a/cache/classes/interfaces.php
          +++ b/cache/classes/interfaces.php
          @@ -108,7 +108,7 @@ interface cache_loader {
               public function set_many(array $keyvaluearray);
           
               /**
          -     * Test is a cache has a key.
          +     * Test if a cache has a key.
                *
                * The use of the has methods is strongly discouraged. In a high load environment the cache may well change between the
                * test and any subsequent action (get, set, delete etc).
          @@ -144,7 +144,7 @@ interface cache_loader {
               public function has_any(array $keys);
           
               /**
          -     * Test is a cache has all of the given keys.
          +     * Test if a cache has all of the given keys.
                *
                * It is strongly recommended to avoid the use of this function if not absolutely required.
                * In a high load environment the cache may well change between the test and any subsequent action (get, set, delete etc).
          @@ -287,7 +287,7 @@ interface cache_is_lockable {
           interface cache_is_key_aware {
           
               /**
          -     * Test is a cache has a key.
          +     * Test if a cache has a key.
                *
                * The use of the has methods is strongly discouraged. In a high load environment the cache may well change between the
                * test and any subsequent action (get, set, delete etc).
          @@ -323,7 +323,7 @@ interface cache_is_key_aware {
               public function has_any(array $keys);
           
               /**
          -     * Test is a cache has all of the given keys.
          +     * Test if a cache has all of the given keys.
                *
                * It is strongly recommended to avoid the use of this function if not absolutely required.
                * In a high load environment the cache may well change between the test and any subsequent action (get, set, delete etc).
          diff --git a/cache/classes/loaders.php b/cache/classes/loaders.php
          index 9f7dc79..5298ae3 100644
          --- a/cache/classes/loaders.php
          +++ b/cache/classes/loaders.php
          @@ -622,7 +622,7 @@ class cache implements cache_loader {
               }
           
               /**
          -     * Test is a cache has a key.
          +     * Test if a cache has a key.
                *
                * The use of the has methods is strongly discouraged. In a high load environment the cache may well change between the
                * test and any subsequent action (get, set, delete etc).
          @@ -678,7 +678,7 @@ class cache implements cache_loader {
               }
           
               /**
          -     * Test is a cache has all of the given keys.
          +     * Test if a cache has all of the given keys.
                *
                * It is strongly recommended to avoid the use of this function if not absolutely required.
                * In a high load environment the cache may well change between the test and any subsequent action (get, set, delete etc).
          @@ -1877,7 +1877,7 @@ class cache_session extends cache {
               }
           
               /**
          -     * Test is a cache has a key.
          +     * Test if a cache has a key.
                *
                * The use of the has methods is strongly discouraged. In a high load environment the cache may well change between the
                * test and any subsequent action (get, set, delete etc).
          @@ -1927,7 +1927,7 @@ class cache_session extends cache {
               }
           
               /**
          -     * Test is a cache has all of the given keys.
          +     * Test if a cache has all of the given keys.
                *
                * It is strongly recommended to avoid the use of this function if not absolutely required.
                * In a high load environment the cache may well change between the test and any subsequent action (get, set, delete etc).
          
          Show
          Matteo Scaramuccia added a comment - Hi Sam, great work and nice improvement! I got in touch with it thanks to the Integration round 2013-04-25 post: I'll take the time to learn it. BTW, there 8 typos (the same pattern, 8 times) in PHPDoc, do you think they should be "fixed"? I'm not native English i.e. I'm not sure if they'll "hurt" someone...: diff --git a/cache/classes/interfaces.php b/cache/classes/interfaces.php index 24996ed..b950260 100644 --- a/cache/classes/interfaces.php +++ b/cache/classes/interfaces.php @@ -108,7 +108,7 @@ interface cache_loader { public function set_many(array $keyvaluearray); /** - * Test is a cache has a key. + * Test if a cache has a key. * * The use of the has methods is strongly discouraged. In a high load environment the cache may well change between the * test and any subsequent action (get, set, delete etc). @@ -144,7 +144,7 @@ interface cache_loader { public function has_any(array $keys); /** - * Test is a cache has all of the given keys. + * Test if a cache has all of the given keys. * * It is strongly recommended to avoid the use of this function if not absolutely required. * In a high load environment the cache may well change between the test and any subsequent action (get, set, delete etc). @@ -287,7 +287,7 @@ interface cache_is_lockable { interface cache_is_key_aware { /** - * Test is a cache has a key. + * Test if a cache has a key. * * The use of the has methods is strongly discouraged. In a high load environment the cache may well change between the * test and any subsequent action (get, set, delete etc). @@ -323,7 +323,7 @@ interface cache_is_key_aware { public function has_any(array $keys); /** - * Test is a cache has all of the given keys. + * Test if a cache has all of the given keys. * * It is strongly recommended to avoid the use of this function if not absolutely required. * In a high load environment the cache may well change between the test and any subsequent action (get, set, delete etc). diff --git a/cache/classes/loaders.php b/cache/classes/loaders.php index 9f7dc79..5298ae3 100644 --- a/cache/classes/loaders.php +++ b/cache/classes/loaders.php @@ -622,7 +622,7 @@ class cache implements cache_loader { } /** - * Test is a cache has a key. + * Test if a cache has a key. * * The use of the has methods is strongly discouraged. In a high load environment the cache may well change between the * test and any subsequent action (get, set, delete etc). @@ -678,7 +678,7 @@ class cache implements cache_loader { } /** - * Test is a cache has all of the given keys. + * Test if a cache has all of the given keys. * * It is strongly recommended to avoid the use of this function if not absolutely required. * In a high load environment the cache may well change between the test and any subsequent action (get, set, delete etc). @@ -1877,7 +1877,7 @@ class cache_session extends cache { } /** - * Test is a cache has a key. + * Test if a cache has a key. * * The use of the has methods is strongly discouraged. In a high load environment the cache may well change between the * test and any subsequent action (get, set, delete etc). @@ -1927,7 +1927,7 @@ class cache_session extends cache { } /** - * Test is a cache has all of the given keys. + * Test if a cache has all of the given keys. * * It is strongly recommended to avoid the use of this function if not absolutely required. * In a high load environment the cache may well change between the test and any subsequent action (get, set, delete etc).

            People

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

              Dates

              • Created:
                Updated:
                Resolved: