From 9890ecfc25a61eb0248e03c836bbc70519237529 Mon Sep 17 00:00:00 2001 From: Sam Hemelryk Date: Wed, 16 Jan 2013 17:49:14 +1300 Subject: [PATCH 1/1] MDL-37545 cache: fixed bug during initialisation and updating of cache config --- cache/classes/factory.php | 103 ++++++++++++++++++++++++++++++++++---- cache/locallib.php | 5 +- 2 files changed, 96 insertions(+), 12 deletions(-) diff --git a/cache/classes/factory.php b/cache/classes/factory.php index 447ef30db19..c23a147c6a8 100644 --- a/cache/classes/factory.php +++ b/cache/classes/factory.php @@ -48,6 +48,8 @@ class cache_factory { const STATE_SAVING = 2; /** The cache is ready to use. */ const STATE_READY = 3; + /** The cache is currently updating itself */ + const STATE_UPDATING = 4; /** The cache encountered an error while initialising. */ const STATE_ERROR_INITIALISING = 9; /** The cache has been disabled. */ @@ -220,7 +222,12 @@ class cache_factory { */ public function create_cache(cache_definition $definition) { $class = $definition->get_cache_class(); - $stores = cache_helper::get_cache_stores($definition); + if ($this->is_initialising()) { + // Do nothing we just want the dummy store. + $stores = array(); + } else { + $stores = cache_helper::get_cache_stores($definition); + } if (count($stores) === 0) { // Hmm no stores, better provide a dummy store to mimick functionality. The dev will be none the wiser. $stores[] = $this->create_dummy_store($definition); @@ -333,21 +340,52 @@ class cache_factory { $id .= '::'.$aggregate; } if (!array_key_exists($id, $this->definitions)) { - $instance = $this->create_config_instance(); - $definition = $instance->get_definition_by_id($id); - if (!$definition) { - $this->reset(); - $instance = $this->create_config_instance(true); - $instance->update_definitions(); + // This is the first time this definition has been requested. + if ($this->is_initialising()) { + // We're initialising the cache right now. Don't try to create another config instance. + // We'll just use an ad-hoc cache for the time being. + $definition = cache_definition::load_adhoc(cache_store::MODE_REQUEST, $component, $area); + } else { + // Load all the known definitions and find the desired one. + $instance = $this->create_config_instance(); $definition = $instance->get_definition_by_id($id); if (!$definition) { - throw new coding_exception('The requested cache definition does not exist.'. $id, $id); + // Oh-oh the definition doesn't exist. + // There are several things that could be going on here. + // We may be installing/upgrading a site and have hit a definition that hasn't been used before. + // Of the developer may be trying to use a newly created definition. + if ($this->is_updating()) { + // The cache is presently initialising and the requested cache definition has not been found. + // This means that the cache initialisation has requested something from a cache (I had recursive nightmares about this). + // To serve this purpose and avoid errors we are going to make use of an ad-hoc cache rather than + // search for the definition which would possibly cause an infitite loop trying to initialise the cache. + $definition = cache_definition::load_adhoc(cache_store::MODE_REQUEST, $component, $area); + if ($aggregate !== null) { + // If you get here you deserve a warning. We have to use an ad-hoc cache here, so we can't find the definition and therefor + // can't find any information about the datasource or any of its aggregated. + // Best of luck. + debugging('An unknown cache was requested during development with an aggregate that could not be loaded. Ad-hoc cache used instead.', DEBUG_DEVELOPER); + $aggregate = null; + } + } else { + // Either a typo of the developer has just created the definition and is using it for the first time. + $this->reset(); + $instance = $this->create_config_instance(true); + $instance->update_definitions(); + $definition = $instance->get_definition_by_id($id); + if (!$definition) { + throw new coding_exception('The requested cache definition does not exist.'. $id, $id); + } else { + debugging('Cache definitions reparsed causing cache reset in order to locate definition. + You should bump the version number to ensure definitions are reprocessed.', DEBUG_DEVELOPER); + } + $definition = cache_definition::load($id, $definition, $aggregate); + } } else { - debugging('Cache definitions reparsed causing cache reset in order to locate definition. - You should bump the version number to ensure definitions are reprocessed.', DEBUG_DEVELOPER); + $definition = cache_definition::load($id, $definition, $aggregate); } } - $this->definitions[$id] = cache_definition::load($id, $definition, $aggregate); + $this->definitions[$id] = $definition; } return $this->definitions[$id]; } @@ -414,6 +452,29 @@ class cache_factory { return true; } + /** + * Informs the factory that the cache is currently updating itself. + * + * This forces the state to upgrading and can only be called once the cache is ready to use. + * Calling it ensure we don't try to reinstantite things when requesting cache definitions that don't exist yet. + */ + public function updating_started() { + if ($this->state !== self::STATE_READY) { + return false; + } + $this->state = self::STATE_UPDATING; + return true; + } + + /** + * Informs the factory that the upgrading has finished. + * + * This forces the state back to ready. + */ + public function updating_finished() { + $this->state = self::STATE_READY; + } + /** * Returns true if the cache API has been disabled. * @@ -423,6 +484,26 @@ class cache_factory { return $this->state === self::STATE_DISABLED; } + /** + * Returns true if the cache is currently initialising itself. + * + * This includes both initialisation and saving the cache config file as part of that initialisation. + * + * @return bool + */ + public function is_initialising() { + return $this->state === self::STATE_INITIALISING || $this->state === self::STATE_SAVING; + } + + /** + * Returns true if the cache is currently updating itself. + * + * @return bool + */ + public function is_updating() { + return $this->state === self::STATE_UPDATING; + } + /** * Disables as much of the cache API as possible. * diff --git a/cache/locallib.php b/cache/locallib.php index 76888b7e782..cd4407cf9f2 100644 --- a/cache/locallib.php +++ b/cache/locallib.php @@ -401,8 +401,11 @@ class cache_config_writer extends cache_config { * @param bool $coreonly If set to true only core definitions will be updated. */ public static function update_definitions($coreonly = false) { - $config = self::instance(); + $factory = cache_factory::instance(); + $factory->updating_started(); + $config = $factory->create_config_instance(true); $config->write_definitions_to_cache(self::locate_definitions($coreonly)); + $factory->updating_finished(); } /** -- 2.17.1