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

Cache: Improve cache locking API

    XMLWordPrintable

Details

    • 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.

    Description

      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.

      Attachments

        1. ir-updates.patch
          3 kB
          Sam Marshall
        2. MDL-78467.png
          190 kB
          Ron Carl Alfon Yu

        Issue Links

          Activity

            People

              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

              Dates

                Created:
                Updated:
                Resolved:

                Time Tracking

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

                  Clockify

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