-
Improvement
-
Resolution: Fixed
-
Major
-
4.3
-
MOODLE_403_STABLE
-
MOODLE_403_STABLE
-
MDL-78467-master -
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.