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

Policy discussion: Tidy up locking options for cache definitions

    XMLWordPrintable

Details

    • Improvement
    • Status: Open
    • Minor
    • Resolution: Unresolved
    • Future Dev
    • None
    • Policy

    Description

      Proposal

      Tidy up locking options for cache definitions.

      Background

      MDL-67020 introduced the new cache defintion option `requirelockingbeforewrite`. This checks that a lock already exists for a given cache key before setting the key is attempted. This was added because when rebuilding coursemodinfo cache, the expensive job that you want the lock to protect is rebuilding all the data to store in the cache, before `set()` is called. The existing `requirelockingwrite` only protected actually writing the data into the cache, which is relatively quick.

      Rationale

      1. `requirelockingwrite` may be worth applying when a cache contains large amounts of data in a single key that may take a long time to write, but in that case it is also likely to take a long time to construct, so `requirelockingbeforewrite` is applicable. Even if the data is quick to construct but slow to write, `requirelockingbeforewrite` can be used in this case to ensure a lock is obtained before `set()` is called.

      2. `requirelockingwrite` is not used in any cache definitions in Moodle core. As a result, it is poorly tested, and I have found at least one case where it does not work correctly (`cache_application::set_many`).

      3. `requirelockingread` is not implemented consistently. `cache_application::get_many` creates a lock for each key, whereas `cache_application::get_implementation` expects a lock to exist already, as in `requirelockingbeforewrite`. It is also not implemented in any core caches.

      Options

      1. Create a new `requirelockingbeforeread` option which mirrors `requirelockingbeforewrite` exactly, then deprecate `requirelockingread` and `requirelockingwrite`. This gives us the flexibility we need in the cache API, while minimising the number of options to maintain. Plugin maintainers who have implemented the existing options will need to migrate to the new ones.
      2. Create `requirelockingbeforeread`, make `requirelockingread` behave consistently, and fix the bugs with `requirelockingwrite`. Maintain all four options going forwards. This gives us maximum flexibility and convenience for plugin maintainers, but some duplication of functionality. Plugins written to expect the current behaviours will need updating.
      3. Fix the issues with `requirelockingwrite` and `requirelockingread` so that they behave correctly and consistently, but don't add `requirelockingbeforewrite`. Plugins written to expect the current behaviours will need updating.
      4. Leave everything as it is.

      I favour option 1, to give us fewer options to maintain going forward.

      Attachments

        Issue Links

          Activity

            People

              Unassigned Unassigned
              marxjohnson Mark Johnson
              David Woloszyn, Huong Nguyen, Jake Dallimore, Meirza, Michael Hawkins, Raquel Ortega, Safat Shahin, Stevani Andolo
              Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

                Created:
                Updated: