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:
    • Testing Instructions:
      Hide
      1. Run unit tests.
      2. Run them again - there are lots!
      3. Upgrade a site
      4. Perform a fresh install
      5. Log in as an admin and set up the front page to display all options for all users.
      6. Test the front page as a guest, student, and admin.
      7. Test switching users.
      8. Run performance tests if you can.
      Show
      Run unit tests. Run them again - there are lots! Upgrade a site Perform a fresh install Log in as an admin and set up the front page to display all options for all users. Test the front page as a guest, student, and admin. Test switching users. Run performance tests if you can.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      wip-MDL-38565-m25

      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

        Gliffy Diagrams

          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: