Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-78467

Cache: Improve cache locking API

XMLWordPrintable

    • MOODLE_403_STABLE
    • MOODLE_403_STABLE
    • MDL-78467-master
    • Hide

      There is no intended change in behaviour as a result of these changes, so we should do a basic test of the caching including updating the modinfo cache:

      1. Go to any course page. Verify there are no errors.
      2. Edit settings for the course, change the course name, and save changes. Verify there are no errors.

      The detail of these API changes should be covered by unit tests.

      Show
      There is no intended change in behaviour as a result of these changes, so we should do a basic test of the caching including updating the modinfo cache: Go to any course page. Verify there are no errors. Edit settings for the course, change the course name, and save changes. Verify there are no errors. The detail of these API changes should be covered by unit tests.

      Following on from MDL-78092 we have noticed that it is confusing that the cache acquire_lock API just returns false if it can't acquire a lock.

      In that issue, and also in some of the cache code before the old requirelockingread/write features were removed in MDL-78109, there were places which didn't handle this correctly - i.e. they assume that a lock has been acquired after acquire_lock returns. IMO this is understandable and the function name kind of sounds like that too: it's not called 'see_if_we_can_acquire_lock'.

      I propose that we tidy up the cache API with regard to locking, specifically:

      • Change acquire_lock so that it throws an exception if a lock cannot be acquired after the underlying locking mechanism's timeout, instead of just returning false. The type signature can be backwards-compatible because we can make it always return true.
      • Ensure that locks are always released (as much as possible) by using try-finally blocks.
      • Ensure that requirelockingbeforewrite works consistently by throwing exceptions in all cases where there isn't a lock on the key you're trying to set or delete (previously set_many output debugging messages instead of an exception, and delete/delete_many didn't check for the lock).

      Because of the change to make 'delete' check locks we will also need to change the one place that actually uses 'requirelockingbeforewrite', i.e. modinfo cache. The modinfo cache gets deleted in some places, which is not necessary now because we are using the versioned API. (If the version increases, any old data will not be used and will soon be overwritten by a new version, so there is no need to actually delete it.)

      Brendan Heywood wrote a nice explanation in a comment in MDL-78109 explaining the reason delete is not needed for modinfo cache:

      Lets say you have cache values which are 10 meg and you have 100 courses, and you have 10 front ends and each has a local cache and there is a shared global cache. Once the system has warmed up the total cache usage will be static and all of them will store 1 gig of cache data. Even in the face of a steady stream of changes each cache store at any point in the system will have a max of 1 gig of storage, each new version isn't taking up additional room its replacing the version before it. When I say you can't delete across a distributed cache I simply mean it's not instant nor guaranteed, but practically speaking as the 10 front ends serve up content it will propagate fully across them all and the total storage won't have changed at all, during or after the propagation.

      To be clear for anyone else reading, this only applies to caches which used versioned values using set_versioned. This does NOT apply if you are using versioned keys in which case new versions will take up additional room eg htmlpurifier.

            quen Sam Marshall
            quen Sam Marshall
            Andrew Lyons Andrew Lyons
            Jun Pataleta Jun Pataleta
            Ron Carl Alfon Yu Ron Carl Alfon Yu
            Votes:
            1 Vote for this issue
            Watchers:
            7 Start watching this issue

              Created:
              Updated:
              Resolved:

                Estimated:
                Original Estimate - 0 minutes
                0m
                Remaining:
                Remaining Estimate - 0 minutes
                0m
                Logged:
                Time Spent - 2 hours, 58 minutes
                2h 58m

                  Error rendering 'clockify-timesheets-time-tracking-reports:timer-sidebar'. Please contact your Jira administrators.