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:

      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()

        Gliffy Diagrams

          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: