From f4cec2ec875708d5fd9f1164465589eeaf7959a4 Mon Sep 17 00:00:00 2001 From: Matteo Scaramuccia Date: Sat, 17 Nov 2012 23:13:22 +0100 Subject: [PATCH 1/4] MDL-36322 Errors when updating a Moodle instance with some misconfigured cache stores --- cache/stores/memcache/lib.php | 19 +++++++++++++------ cache/stores/memcached/lib.php | 29 +++++++++++++++++++---------- cache/stores/mongodb/lib.php | 18 +++++++++++++----- cache/stores/session/lib.php | 2 ++ cache/stores/static/lib.php | 1 + 5 files changed, 48 insertions(+), 21 deletions(-) diff --git a/cache/stores/memcache/lib.php b/cache/stores/memcache/lib.php index d25b1c046d8..f83d45f1e9f 100644 --- a/cache/stores/memcache/lib.php +++ b/cache/stores/memcache/lib.php @@ -106,7 +106,15 @@ class cachestore_memcache extends cache_store implements cache_is_configurable { $this->servers[] = $server; } - $this->isready = true; + $this->connection = new Memcache; + foreach ($this->servers as $server) { + $this->connection->addServer($server[0], $server[1], true, $server[2]); + // Test the connection to this server. + if (@$this->connection->set("$server[0]:$server[1]:$server[2]", 'ping', MEMCACHE_COMPRESSED, 1)) { + // We can connect at least to this server. + $this->isready = true; + } + } } /** @@ -121,10 +129,6 @@ class cachestore_memcache extends cache_store implements cache_is_configurable { throw new coding_exception('This memcache instance has already been initialised.'); } $this->definition = $definition; - $this->connection = new Memcache; - foreach ($this->servers as $server) { - $this->connection->addServer($server[0], $server[1], true, $server[2]); - } } /** @@ -276,7 +280,10 @@ class cachestore_memcache extends cache_store implements cache_is_configurable { * @return boolean True on success. False otherwise. */ public function purge() { - $this->connection->flush(); + if ($this->isready) { + $this->connection->flush(); + } + return true; } diff --git a/cache/stores/memcached/lib.php b/cache/stores/memcached/lib.php index 4370a6d84b1..8f2a59ad388 100644 --- a/cache/stores/memcached/lib.php +++ b/cache/stores/memcached/lib.php @@ -127,7 +127,21 @@ class cachestore_memcached extends cache_store implements cache_is_configurable $this->options[Memcached::OPT_HASH] = $hashmethod; $this->options[Memcached::OPT_BUFFER_WRITES] = $bufferwrites; - $this->isready = true; + $this->connection = new Memcached(crc32($this->name)); + $servers = $this->connection->getServerList(); + if (empty($servers)) { + foreach ($this->options as $key => $value) { + $this->connection->setOption($key, $value); + } + $this->connection->addServers($this->servers); + foreach ($this->servers as $server) { + // Test the connection to this server. + if (@$this->connection->set("$server[0]:$server[1]:$server[2]", 'ping', MEMCACHE_COMPRESSED, 1)) { + // We can connect at least to this server. + $this->isready = true; + } + } + } } /** @@ -142,14 +156,6 @@ class cachestore_memcached extends cache_store implements cache_is_configurable throw new coding_exception('This memcached instance has already been initialised.'); } $this->definition = $definition; - $this->connection = new Memcached(crc32($this->name)); - $servers = $this->connection->getServerList(); - if (empty($servers)) { - foreach ($this->options as $key => $value) { - $this->connection->setOption($key, $value); - } - $this->connection->addServers($this->servers); - } } /** @@ -302,7 +308,10 @@ class cachestore_memcached extends cache_store implements cache_is_configurable * @return boolean True on success. False otherwise. */ public function purge() { - $this->connection->flush(); + if ($this->isready) { + $this->connection->flush(); + } + return true; } diff --git a/cache/stores/mongodb/lib.php b/cache/stores/mongodb/lib.php index cefc299f4b1..36aed49eae0 100644 --- a/cache/stores/mongodb/lib.php +++ b/cache/stores/mongodb/lib.php @@ -130,7 +130,13 @@ class cachestore_mongodb extends cache_store implements cache_is_configurable { $this->extendedmode = $configuration['extendedmode']; } - $this->isready = self::are_requirements_met(); + try { + $this->connection = new Mongo($this->server, $this->options); + $this->database = $this->connection->selectDB($this->databasename); + $this->isready = true; + } catch (Exception $e) { + // Tipically, a MongoConnectionException. + } } /** @@ -176,8 +182,6 @@ class cachestore_mongodb extends cache_store implements cache_is_configurable { throw new coding_exception('This mongodb instance has already been initialised.'); } $this->definitionhash = $definition->generate_definition_hash(); - $this->connection = new Mongo($this->server, $this->options); - $this->database = $this->connection->selectDB($this->databasename); $this->collection = $this->database->selectCollection($this->definitionhash); $this->collection->ensureIndex(array('key' => 1), array( 'safe' => $this->usesafe, @@ -366,8 +370,12 @@ class cachestore_mongodb extends cache_store implements cache_is_configurable { * @return boolean True on success. False otherwise. */ public function purge() { - $this->collection->drop(); - $this->collection = $this->database->selectCollection($this->definitionhash); + if ($this->isready) { + $this->collection->drop(); + $this->collection = $this->database->selectCollection($this->definitionhash); + } + + return true; } /** diff --git a/cache/stores/session/lib.php b/cache/stores/session/lib.php index 5a2b9a09d00..383dae17c8a 100644 --- a/cache/stores/session/lib.php +++ b/cache/stores/session/lib.php @@ -361,6 +361,8 @@ class cachestore_session extends session_data_store implements cache_is_key_awar */ public function purge() { $this->store = array(); + + return true; } /** diff --git a/cache/stores/static/lib.php b/cache/stores/static/lib.php index 6b4ab4ed58d..4ac32de7448 100644 --- a/cache/stores/static/lib.php +++ b/cache/stores/static/lib.php @@ -358,6 +358,7 @@ class cachestore_static extends static_data_store implements cache_is_key_aware public function purge() { $this->flush_store_by_id($this->storeid); $this->store = &self::register_store_id($this->storeid); + return true; } /** From 65b3edc4502540b5b092ee4c2ae680f9146003a3 Mon Sep 17 00:00:00 2001 From: Sam Hemelryk Date: Thu, 24 Jan 2013 15:41:06 +1300 Subject: [PATCH 2/4] MDL-36322 cache: cache stores now test connections during construction --- cache/stores/memcache/lib.php | 14 +++++++++----- cache/stores/memcached/lib.php | 17 +++++++++-------- cache/stores/mongodb/lib.php | 6 +++--- cache/stores/session/lib.php | 1 - 4 files changed, 21 insertions(+), 17 deletions(-) diff --git a/cache/stores/memcache/lib.php b/cache/stores/memcache/lib.php index f83d45f1e9f..e9c6e1d32b6 100644 --- a/cache/stores/memcache/lib.php +++ b/cache/stores/memcache/lib.php @@ -69,6 +69,12 @@ class cachestore_memcache extends cache_store implements cache_is_configurable { */ protected $isready = false; + /** + * Set to true once this store instance has been initialised. + * @var bool + */ + protected $isinitialised = false; + /** * The cache definition this store was initialised for. * @var cache_definition @@ -110,10 +116,7 @@ class cachestore_memcache extends cache_store implements cache_is_configurable { foreach ($this->servers as $server) { $this->connection->addServer($server[0], $server[1], true, $server[2]); // Test the connection to this server. - if (@$this->connection->set("$server[0]:$server[1]:$server[2]", 'ping', MEMCACHE_COMPRESSED, 1)) { - // We can connect at least to this server. - $this->isready = true; - } + $this->isready = @$this->connection->set("ping", 'ping', MEMCACHE_COMPRESSED, 1); } } @@ -129,6 +132,7 @@ class cachestore_memcache extends cache_store implements cache_is_configurable { throw new coding_exception('This memcache instance has already been initialised.'); } $this->definition = $definition; + $this->isinitialised = true; } /** @@ -137,7 +141,7 @@ class cachestore_memcache extends cache_store implements cache_is_configurable { * @return bool */ public function is_initialised() { - return ($this->connection !== null); + return ($this->isinitialised); } /** diff --git a/cache/stores/memcached/lib.php b/cache/stores/memcached/lib.php index 8f2a59ad388..4cad20cb72b 100644 --- a/cache/stores/memcached/lib.php +++ b/cache/stores/memcached/lib.php @@ -74,6 +74,12 @@ class cachestore_memcached extends cache_store implements cache_is_configurable */ protected $isready = false; + /** + * Set to true when this store instance has been initialised. + * @var bool + */ + protected $isinitialised = false; + /** * The cache definition this store was initialised with. * @var cache_definition @@ -134,13 +140,7 @@ class cachestore_memcached extends cache_store implements cache_is_configurable $this->connection->setOption($key, $value); } $this->connection->addServers($this->servers); - foreach ($this->servers as $server) { - // Test the connection to this server. - if (@$this->connection->set("$server[0]:$server[1]:$server[2]", 'ping', MEMCACHE_COMPRESSED, 1)) { - // We can connect at least to this server. - $this->isready = true; - } - } + $this->isready = @$this->connection->set("ping", 'ping', 1); } } @@ -156,6 +156,7 @@ class cachestore_memcached extends cache_store implements cache_is_configurable throw new coding_exception('This memcached instance has already been initialised.'); } $this->definition = $definition; + $this->isinitialised = true; } /** @@ -164,7 +165,7 @@ class cachestore_memcached extends cache_store implements cache_is_configurable * @return bool */ public function is_initialised() { - return ($this->connection !== null); + return ($this->isinitialised); } /** diff --git a/cache/stores/mongodb/lib.php b/cache/stores/mongodb/lib.php index 36aed49eae0..2385d5e5682 100644 --- a/cache/stores/mongodb/lib.php +++ b/cache/stores/mongodb/lib.php @@ -132,10 +132,9 @@ class cachestore_mongodb extends cache_store implements cache_is_configurable { try { $this->connection = new Mongo($this->server, $this->options); - $this->database = $this->connection->selectDB($this->databasename); $this->isready = true; - } catch (Exception $e) { - // Tipically, a MongoConnectionException. + } catch (MongoConnectionException $e) { + // We only want to catch MongoConnectionExceptions here. } } @@ -181,6 +180,7 @@ class cachestore_mongodb extends cache_store implements cache_is_configurable { if ($this->is_initialised()) { throw new coding_exception('This mongodb instance has already been initialised.'); } + $this->database = $this->connection->selectDB($this->databasename); $this->definitionhash = $definition->generate_definition_hash(); $this->collection = $this->database->selectCollection($this->definitionhash); $this->collection->ensureIndex(array('key' => 1), array( diff --git a/cache/stores/session/lib.php b/cache/stores/session/lib.php index 383dae17c8a..8cb42f0d0e0 100644 --- a/cache/stores/session/lib.php +++ b/cache/stores/session/lib.php @@ -361,7 +361,6 @@ class cachestore_session extends session_data_store implements cache_is_key_awar */ public function purge() { $this->store = array(); - return true; } From 5dfa3031a80d8378d04a1c16672ff42620943548 Mon Sep 17 00:00:00 2001 From: Sam Hemelryk Date: Wed, 30 Jan 2013 13:39:45 +1300 Subject: [PATCH 3/4] MDL-36322 cache: clarified docs for initialise and construct --- cache/classes/store.php | 18 ++++++++++++++++-- cache/stores/mongodb/lib.php | 14 ++++++++++++-- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/cache/classes/store.php b/cache/classes/store.php index 7cbb069dd95..4130b590f25 100644 --- a/cache/classes/store.php +++ b/cache/classes/store.php @@ -128,7 +128,16 @@ abstract class cache_store implements cache_store_interface { /** * Constructs an instance of the cache store. * - * This method should not create connections or perform and processing, it should be used + * The constructor should be responsible for creating anything needed by the store that is not + * specific to a definition. + * Tasks such as opening a connection to check it is available are best done here. + * Tasks that are definition specific such as creating a storage area for the definition data + * or creating key tables and indexs are best done within the initialise method. + * + * Once a store has been constructed the cache API will check it is ready to be intialised with + * a definition by called $this->is_ready(). + * If the setup of the store failed (connection could not be established for example) then + * that method should return false so that the store instance is not selected for use. * * @param string $name The name of the cache store * @param array $configuration The configuration for this store instance. @@ -144,7 +153,12 @@ abstract class cache_store implements cache_store_interface { /** * Initialises a new instance of the cache store given the definition the instance is to be used for. * - * This function should prepare any given connections etc. + * This function should be used to run any definition specific setup the store instance requires. + * Tasks such as creating storage areas, or creating indexes are best done here. + * + * Its important to note that the initialise method is expected to always succeed. + * If there are setup tasks that may fail they should be done within the __construct method + * and should they fail is_ready should return false. * * @param cache_definition $definition */ diff --git a/cache/stores/mongodb/lib.php b/cache/stores/mongodb/lib.php index 2385d5e5682..39a180683c7 100644 --- a/cache/stores/mongodb/lib.php +++ b/cache/stores/mongodb/lib.php @@ -100,7 +100,17 @@ class cachestore_mongodb extends cache_store implements cache_is_configurable { protected $definitionhash = null; /** - * Constructs a new instance of the Mongo store but does not connect to it. + * Set to true once this store is ready to be initialised and used. + * @var bool + */ + protected $isready = false; + + /** + * Constructs a new instance of the Mongo store. + * + * Noting that this function is not an initialisation. It is used to prepare the store for use. + * The store will be initialised when required and will be provided with a cache_definition at that time. + * * @param string $name * @param array $configuration */ @@ -171,7 +181,7 @@ class cachestore_mongodb extends cache_store implements cache_is_configurable { /** * Initialises the store instance for use. * - * This function is reponsible for making the connection. + * Once this has been done the cache is all set to be used. * * @param cache_definition $definition * @throws coding_exception From 79c55ca34472ffc3f1aaf3d30e951903cf9177cb Mon Sep 17 00:00:00 2001 From: Sam Hemelryk Date: Wed, 30 Jan 2013 13:59:17 +1300 Subject: [PATCH 4/4] MDL-36322 cache: implemented cache_store::create_clone --- cache/classes/factory.php | 8 +++++++- cache/classes/store.php | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/cache/classes/factory.php b/cache/classes/factory.php index c23a147c6a8..f3d520c51af 100644 --- a/cache/classes/factory.php +++ b/cache/classes/factory.php @@ -263,7 +263,13 @@ class cache_factory { if (!$store->is_ready() || !$store->is_supported_mode($definition->get_mode())) { return false; } - $store = clone($this->stores[$name]); + // We always create a clone of the original store. + // If we were to clone a store that had already been initialised with a definition then + // we'd run into a myriad of issues. + // We use a method of the store to create a clone rather than just creating it ourselves + // so that if any store out there doesn't handle cloning they can override this method in + // order to address the issues. + $store = $this->stores[$name]->create_clone($details); $store->initialise($definition); return $store; } diff --git a/cache/classes/store.php b/cache/classes/store.php index 4130b590f25..5bb26e4d4fd 100644 --- a/cache/classes/store.php +++ b/cache/classes/store.php @@ -306,4 +306,22 @@ abstract class cache_store implements cache_store_interface { public function supports_native_ttl() { return $this::get_supported_features() & self::SUPPORTS_NATIVE_TTL; } + + /** + * Creates a clone of this store instance ready to be initialised. + * + * This method is used so that a cache store needs only be constructed once. + * Future requests for an instance of the store will be given a cloned instance. + * + * If you are writing a cache store that isn't compatible with the clone operation + * you can override this method to handle any situations you want before cloning. + * + * @param array $details An array containing the details of the store from the cache config. + * @return cache_store + */ + public function create_clone(array $details = array()) { + // By default we just run clone. + // Any stores that have an issue with this will need to override the create_clone method. + return clone($this); + } }