Moodle
  1. Moodle
  2. MDL-38303

MUC: Session cache is not changed when user logs in or out

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.4.3
    • Component/s: Caching
    • Labels:
    • Rank:
      48167

      Description

      Data is session-level cache remains the same when user loggs in/out

      I had a similar unit test for it in https://github.com/marinaglancy/moodle/compare/master...wip-MDL-38165-master
      function test_session_cache()

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          Thanks for picking up this issue Marina, I've pushed a couple of branches up for peer-review now.
          The solution is have the session cache track the userid its interacting with and if at any point it changes purge the cache and update itself.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Thanks for picking up this issue Marina, I've pushed a couple of branches up for peer-review now. The solution is have the session cache track the userid its interacting with and if at any point it changes purge the cache and update itself. Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          Putting this up for integration review now.

          Show
          Sam Hemelryk added a comment - Putting this up for integration review now.
          Hide
          Damyon Wiese added a comment -

          The patch looks good, the unit tests pass. I looked for anything that might be using a session cache and switching users - but there is hardly any use of session cache at all right now (none in 2.4) so this seems safe.

          Show
          Damyon Wiese added a comment - The patch looks good, the unit tests pass. I looked for anything that might be using a session cache and switching users - but there is hardly any use of session cache at all right now (none in 2.4) so this seems safe.
          Hide
          Damyon Wiese added a comment -

          Hi Sam,

          I just want some clarification on something (I am not sure I follow the code 100%). If a the default cache storage for MODE_SESSION cache is (for example) a file_store instead of a session_store - there is nothing that ties the cache instance to a specific session. So - if this code detects a userid change, it will expire the session cache for all users. (Sorry if the names are confusing in that sentence).

          This also means that the way I used this cache in mod_assign is incorrect - I should have included the current userid in the name of the cache to make it user specific.

          • Damyon
          Show
          Damyon Wiese added a comment - Hi Sam, I just want some clarification on something (I am not sure I follow the code 100%). If a the default cache storage for MODE_SESSION cache is (for example) a file_store instead of a session_store - there is nothing that ties the cache instance to a specific session. So - if this code detects a userid change, it will expire the session cache for all users. (Sorry if the names are confusing in that sentence). This also means that the way I used this cache in mod_assign is incorrect - I should have included the current userid in the name of the cache to make it user specific. Damyon
          Hide
          Damyon Wiese added a comment -

          Thanks Sam,

          Pushed to master and 24.

          I'll follow up with you next week about whether the session cache is supposed to be "per user".

          Show
          Damyon Wiese added a comment - Thanks Sam, Pushed to master and 24. I'll follow up with you next week about whether the session cache is supposed to be "per user".
          Hide
          Damyon Wiese added a comment -

          Unit tests pass so passing this issue.

          Show
          Damyon Wiese added a comment - Unit tests pass so passing this issue.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Luckily we don't have any cache definition working under session mode. Because I agree with Damyon that it's not clear how it's going to supposedly work using, for example, one memcached store (that supports the session mode).

          I've been following code and it's not clear for me how that memcached store would behave when the cache (session_cache) detects a change of user. Purging the whole store? ohoh. Perhaps, after all, the "shared-mem stores" are only suitable for application_cache, and not for session_cache.

          Something to review and fix before any session_cache mode lands to core...

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Luckily we don't have any cache definition working under session mode. Because I agree with Damyon that it's not clear how it's going to supposedly work using, for example, one memcached store (that supports the session mode). I've been following code and it's not clear for me how that memcached store would behave when the cache (session_cache) detects a change of user. Purging the whole store? ohoh. Perhaps, after all, the "shared-mem stores" are only suitable for application_cache, and not for session_cache. Something to review and fix before any session_cache mode lands to core... Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

          Thanks!

          PS: Yay, legacy template messages. Yes, you're ok, we don't have CVS anymore!

          Show
          Eloy Lafuente (stronk7) added a comment - This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks! PS: Yay, legacy template messages. Yes, you're ok, we don't have CVS anymore!
          Hide
          Damyon Wiese added a comment -

          Just confirmed on chat with Sam that when the mode is MODE_SESSION - the cache is supposed to include the userid in the keys - it doesn't look like it is currently and so is a bug for any session store other than the "session session store". Will raise an issue for this.

          Show
          Damyon Wiese added a comment - Just confirmed on chat with Sam that when the mode is MODE_SESSION - the cache is supposed to include the userid in the keys - it doesn't look like it is currently and so is a bug for any session store other than the "session session store". Will raise an issue for this.
          Hide
          Damyon Wiese added a comment -

          Just one note - there is a session cache usage in master (in assign) thats why I thought of it.

          Show
          Damyon Wiese added a comment - Just one note - there is a session cache usage in master (in assign) thats why I thought of it.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: