Moodle
  1. Moodle
  2. MDL-38165

MUC: Session cache is not purged for another user

    Details

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

      Description

      I was trying to create a unit test for this situation but could not emulate different sessions.

      There is a session cache
      $cache = cache::make_from_params(cache_store::MODE_SESSION, ...);

      I have two users simultaneously working with a website. One of them updates information and purges the cache. The second one still sees the same data it was before purging. If the second user logs out and in, he can see the updated data.

      I would expect that when cache is purged, it is purged for all users.

      Also I wrote several unit tests that fail:
      https://github.com/marinaglancy/moodle/compare/master...wip-MDL-38165-master

        Issue Links

          Activity

          Hide
          Marina Glancy added a comment -
          marina@MarinaThinkPad:~/www/master-pg$ ~/pear/bin/phpunit cache_phpunit_tests cache/tests/cache_test.php 
          PHPUnit 3.7.13 by Sebastian Bergmann.
          
          Configuration read from /home/marina/repositories/master-pg/moodle/phpunit.xml
          
          ....................EFFF
          
          Time: 8 seconds, Memory: 67.50Mb
          
          There was 1 error:
          
          1) cache_phpunit_tests::test_false_value
          coding_exception: Coding error detected, it must be fixed by a programmer: Failed to unserialise data from file. Either failed to read, or failed to write.
          
          /home/marina/repositories/master-pg/moodle/cache/stores/file/lib.php:437
          /home/marina/repositories/master-pg/moodle/cache/stores/file/lib.php:339
          /home/marina/repositories/master-pg/moodle/cache/classes/loaders.php:293
          /home/marina/repositories/master-pg/moodle/cache/classes/loaders.php:1299
          /home/marina/repositories/master-pg/moodle/cache/tests/cache_test.php:774
          /home/marina/repositories/master-pg/moodle/lib/phpunit/classes/advanced_testcase.php:76
          
          To re-run:
           /home/marina/pear/bin/phpunit cache_phpunit_tests cache/tests/cache_test.php
          
          --
          
          
          There were 3 failures:
          
          1) cache_phpunit_tests::test_session_cache
          Failed asserting that true is false.
          
          /home/marina/repositories/master-pg/moodle/cache/tests/cache_test.php:799
          /home/marina/repositories/master-pg/moodle/lib/phpunit/classes/advanced_testcase.php:76
          
          To re-run:
           /home/marina/pear/bin/phpunit cache_phpunit_tests cache/tests/cache_test.php
          
          2) cache_phpunit_tests::test_purge_all_caches
          Failed asserting that true is false.
          
          /home/marina/repositories/master-pg/moodle/cache/tests/cache_test.php:821
          /home/marina/repositories/master-pg/moodle/lib/phpunit/classes/advanced_testcase.php:76
          
          To re-run:
           /home/marina/pear/bin/phpunit cache_phpunit_tests cache/tests/cache_test.php
          
          3) cache_phpunit_tests::test_definition_ttl2
          Failed asserting that true is false.
          
          /home/marina/repositories/master-pg/moodle/cache/tests/cache_test.php:849
          /home/marina/repositories/master-pg/moodle/lib/phpunit/classes/advanced_testcase.php:76
          
          To re-run:
           /home/marina/pear/bin/phpunit cache_phpunit_tests cache/tests/cache_test.php
          
          FAILURES!
          Tests: 24, Assertions: 471, Failures: 3, Errors: 1.
          
          Show
          Marina Glancy added a comment - marina@MarinaThinkPad:~/www/master-pg$ ~/pear/bin/phpunit cache_phpunit_tests cache/tests/cache_test.php PHPUnit 3.7.13 by Sebastian Bergmann. Configuration read from /home/marina/repositories/master-pg/moodle/phpunit.xml ....................EFFF Time: 8 seconds, Memory: 67.50Mb There was 1 error: 1) cache_phpunit_tests::test_false_value coding_exception: Coding error detected, it must be fixed by a programmer: Failed to unserialise data from file. Either failed to read, or failed to write. /home/marina/repositories/master-pg/moodle/cache/stores/file/lib.php:437 /home/marina/repositories/master-pg/moodle/cache/stores/file/lib.php:339 /home/marina/repositories/master-pg/moodle/cache/classes/loaders.php:293 /home/marina/repositories/master-pg/moodle/cache/classes/loaders.php:1299 /home/marina/repositories/master-pg/moodle/cache/tests/cache_test.php:774 /home/marina/repositories/master-pg/moodle/lib/phpunit/classes/advanced_testcase.php:76 To re-run: /home/marina/pear/bin/phpunit cache_phpunit_tests cache/tests/cache_test.php -- There were 3 failures: 1) cache_phpunit_tests::test_session_cache Failed asserting that true is false . /home/marina/repositories/master-pg/moodle/cache/tests/cache_test.php:799 /home/marina/repositories/master-pg/moodle/lib/phpunit/classes/advanced_testcase.php:76 To re-run: /home/marina/pear/bin/phpunit cache_phpunit_tests cache/tests/cache_test.php 2) cache_phpunit_tests::test_purge_all_caches Failed asserting that true is false . /home/marina/repositories/master-pg/moodle/cache/tests/cache_test.php:821 /home/marina/repositories/master-pg/moodle/lib/phpunit/classes/advanced_testcase.php:76 To re-run: /home/marina/pear/bin/phpunit cache_phpunit_tests cache/tests/cache_test.php 3) cache_phpunit_tests::test_definition_ttl2 Failed asserting that true is false . /home/marina/repositories/master-pg/moodle/cache/tests/cache_test.php:849 /home/marina/repositories/master-pg/moodle/lib/phpunit/classes/advanced_testcase.php:76 To re-run: /home/marina/pear/bin/phpunit cache_phpunit_tests cache/tests/cache_test.php FAILURES! Tests: 24, Assertions: 471, Failures: 3, Errors: 1.
          Hide
          Marina Glancy added a comment -

          To reproduce my problem:
          Put sessioncache.php to the moodle homedir https://gist.github.com/marinaglancy/5011144
          Open it in two browsers logged in as two different users.

          1. As user one set cache
          2. As user two set cache
          3. Refresh both pages, make sure the cache contents is ok
          4. As user one purge cache
          5. As user two refresh page, the cached value is still there
          Show
          Marina Glancy added a comment - To reproduce my problem: Put sessioncache.php to the moodle homedir https://gist.github.com/marinaglancy/5011144 Open it in two browsers logged in as two different users. As user one set cache As user two set cache Refresh both pages, make sure the cache contents is ok As user one purge cache As user two refresh page, the cached value is still there
          Hide
          Marina Glancy added a comment -

          ... continuing the last test - try purging all caches as any user - the session cache is just glued there until user logs out

          Show
          Marina Glancy added a comment - ... continuing the last test - try purging all caches as any user - the session cache is just glued there until user logs out
          Hide
          Sam Hemelryk added a comment - - edited

          Thanks for the report Marina, indeed there is an issue here.
          It's a big no-no to modify other users session directly, instead what is required is an event driven invalidation.
          We have such a system for application caches whereby you can define invalidation events for them.
          However I've just tested now and that is broken.

          In order to solve this issue I shall get that working for session caches.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - - edited Thanks for the report Marina, indeed there is an issue here. It's a big no-no to modify other users session directly, instead what is required is an event driven invalidation. We have such a system for application caches whereby you can define invalidation events for them. However I've just tested now and that is broken. In order to solve this issue I shall get that working for session caches. Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          Alrighty, I have a patch to fix an issue with event invalidation in both the session and application caches.

          The bug was that purging all entries in a cache with a subsequent request wasn't functioning.
          For the session cache that only the cache of the current user would have been purged.
          In the case of an application cache this means that anyone using a back end set up that makes use of replication would have only seen the cache purged from nodes they were currently connected to.

          As for your code Marina there is one thing that you will need to change.
          Calling purge on a session will only purge the current users session, after thinking about it I've decided this is the best approach. Modifying other users session is generally a bad idea and introduces many complexities to the session system should we try to implement it.
          The event invalidation method gets around this by flagging an event. When a user first interacts with a cache the cache looks to see if the definition has any events and if so checks when the events were last fired. If the events were last fired between the time of the last check and the now the cache API re-runs the purge/invalidation at that point in time.

          For your code this means two things. First you will need to use a definition and set a invalidation event.

          $definitions = array(
              'mycache' => array(
                  'mode' => cache_store::MODE_SESSION,
                  'invalidationevents' => array(
                      'somethinghaschanged'
                  )
              )
          );
          

          Then when you want to purge the cache for all users use this code:

          cache_helper::purge_by_event('somethinghaschanged');
          

          Could you please have a look at this and see if you are happy with all of this and perhaps peer-review the changes.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Alrighty, I have a patch to fix an issue with event invalidation in both the session and application caches. The bug was that purging all entries in a cache with a subsequent request wasn't functioning. For the session cache that only the cache of the current user would have been purged. In the case of an application cache this means that anyone using a back end set up that makes use of replication would have only seen the cache purged from nodes they were currently connected to. As for your code Marina there is one thing that you will need to change. Calling purge on a session will only purge the current users session, after thinking about it I've decided this is the best approach. Modifying other users session is generally a bad idea and introduces many complexities to the session system should we try to implement it. The event invalidation method gets around this by flagging an event. When a user first interacts with a cache the cache looks to see if the definition has any events and if so checks when the events were last fired. If the events were last fired between the time of the last check and the now the cache API re-runs the purge/invalidation at that point in time. For your code this means two things. First you will need to use a definition and set a invalidation event. $definitions = array( 'mycache' => array( 'mode' => cache_store::MODE_SESSION, 'invalidationevents' => array( 'somethinghaschanged' ) ) ); Then when you want to purge the cache for all users use this code: cache_helper::purge_by_event('somethinghaschanged'); Could you please have a look at this and see if you are happy with all of this and perhaps peer-review the changes. Many thanks Sam
          Hide
          Marina Glancy added a comment -

          Sam, please look at my commit in https://github.com/marinaglancy/moodle/compare/master...wip-MDL-38165-master2
          Test does not pass Maybe I'm doing something wrong?

          Show
          Marina Glancy added a comment - Sam, please look at my commit in https://github.com/marinaglancy/moodle/compare/master...wip-MDL-38165-master2 Test does not pass Maybe I'm doing something wrong?
          Hide
          Marina Glancy added a comment -

          Found it.
          If I remove 'persistent' from cache definitions - everything works. But you have just suggested me to use persistent.... So - can I have it all?

          Show
          Marina Glancy added a comment - Found it. If I remove 'persistent' from cache definitions - everything works. But you have just suggested me to use persistent.... So - can I have it all?
          Hide
          Sam Hemelryk added a comment -

          Oh good spotting, now that is a fun issue!

          The problem is stemming from the instance of the store used for the event handling being purged but that purge request not being passed to the other instances of the same store.
          Normally caches are interacted with at the loader level which handles this situation, however the event invalidation code is trying to handle it at a lower level causing the issue.
          I'll look into that right now.

          Show
          Sam Hemelryk added a comment - Oh good spotting, now that is a fun issue! The problem is stemming from the instance of the store used for the event handling being purged but that purge request not being passed to the other instances of the same store. Normally caches are interacted with at the loader level which handles this situation, however the event invalidation code is trying to handle it at a lower level causing the issue. I'll look into that right now.
          Hide
          Sam Hemelryk added a comment -

          Ok I've updated the branches now Marina.

          This is the first use of the event invalidation within core so its great to hit these issues now.
          I've added two more commits, the first introduces more unit tests (making use of persistent caches when testing events) and the second fixes the issue with only the active instance being purged.

          Would you mind having a look again. I've made this a blocker now.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Ok I've updated the branches now Marina. This is the first use of the event invalidation within core so its great to hit these issues now. I've added two more commits, the first introduces more unit tests (making use of persistent caches when testing events) and the second fixes the issue with only the active instance being purged. Would you mind having a look again. I've made this a blocker now. Many thanks Sam
          Hide
          Marina Glancy added a comment -

          yay! All works great for me. Thanks!

          Show
          Marina Glancy added a comment - yay! All works great for me. Thanks!
          Hide
          Sam Hemelryk added a comment -

          Thanks Marina - putting this up for integration now.

          Show
          Sam Hemelryk added a comment - Thanks Marina - putting this up for integration now.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (24 & master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (24 & master), thanks!
          Hide
          David Monllaó added a comment -

          Passing as running in the CI server

          Show
          David Monllaó added a comment - Passing as running in the CI server
          Hide
          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
          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: