From 5d7411e92bab4bcd89402ca28a9a4a4533e5a9ac Mon Sep 17 00:00:00 2001 From: Matteo Scaramuccia Date: Sun, 2 Dec 2012 10:48:18 +0100 Subject: [PATCH 1/4] MDL-36363 Added cache_store::instance_created() and cache_store::instance_deleted() --- cache/classes/store.php | 22 ++++++++++++++++++++++ cache/locallib.php | 9 +++++++++ 2 files changed, 31 insertions(+) diff --git a/cache/classes/store.php b/cache/classes/store.php index dca65dafc8f..e3326774237 100644 --- a/cache/classes/store.php +++ b/cache/classes/store.php @@ -135,6 +135,13 @@ abstract class cache_store implements cache_store_interface { */ abstract public function __construct($name, array $configuration = array()); + /** + * Performs any necessary operation when the store instance has been created. + * + * @link http://tracker.moodle.org/browse/MDL-36363 + */ + public function instance_created() {} + /** * Returns the name of this store instance. * @return string @@ -225,9 +232,24 @@ abstract class cache_store implements cache_store_interface { /** * Performs any necessary clean up when the store instance is being deleted. + * + * Please note that if the store has been already initialised with + * a definition ({@link initialise()}), cleanup will be performed against the scope + * of that definition. + * + * @see instance_deleted() */ abstract public function cleanup(); + /** + * Performs any necessary operation when the store instance is being deleted, + * regardless the store being initialised with a definition ({@link initialise()}). + * + * @link http://tracker.moodle.org/browse/MDL-36363 + * @see cleanup() + */ + public function instance_deleted() {} + /** * Returns true if the user can add an instance of the store plugin. * diff --git a/cache/locallib.php b/cache/locallib.php index 76888b7e782..4982719d66d 100644 --- a/cache/locallib.php +++ b/cache/locallib.php @@ -178,6 +178,10 @@ class cache_config_writer extends cache_config { $this->configstores[$name]['lock'] = $configuration['lock']; unset($this->configstores[$name]['configuration']['lock']); } + // Call instance_created() + $store = new $class($name, $this->configstores[$name]['configuration']); + $store->instance_created(); + $this->config_save(); return true; } @@ -304,6 +308,11 @@ class cache_config_writer extends cache_config { throw new cache_exception('You cannot delete a cache store that has definition mappings.'); } } + // Call instance_deleted() + $class = 'cachestore_'.$this->configstores[$name]['plugin']; + $store = new $class($name, $this->configstores[$name]['configuration']); + $store->instance_deleted(); + unset($this->configstores[$name]); $this->config_save(); return true; From 233046ea5e6a94859cfe5959f043930f23c2069b Mon Sep 17 00:00:00 2001 From: Matteo Scaramuccia Date: Sun, 2 Dec 2012 11:40:35 +0100 Subject: [PATCH 2/4] MDL-36363 Adding/Removing a file store cache instance cleanups the folder too --- cache/classes/store.php | 10 ++++-- cache/stores/file/lib.php | 72 +++++++++++++++++++++++++++++++++------ 2 files changed, 68 insertions(+), 14 deletions(-) diff --git a/cache/classes/store.php b/cache/classes/store.php index e3326774237..219cda08b38 100644 --- a/cache/classes/store.php +++ b/cache/classes/store.php @@ -140,7 +140,9 @@ abstract class cache_store implements cache_store_interface { * * @link http://tracker.moodle.org/browse/MDL-36363 */ - public function instance_created() {} + public function instance_created() { + // By default, do nothing. + } /** * Returns the name of this store instance. @@ -235,7 +237,7 @@ abstract class cache_store implements cache_store_interface { * * Please note that if the store has been already initialised with * a definition ({@link initialise()}), cleanup will be performed against the scope - * of that definition. + * of that definition i.e. the result depends on the specific store implementation. * * @see instance_deleted() */ @@ -248,7 +250,9 @@ abstract class cache_store implements cache_store_interface { * @link http://tracker.moodle.org/browse/MDL-36363 * @see cleanup() */ - public function instance_deleted() {} + public function instance_deleted() { + // By default, do nothing + } /** * Returns true if the user can add an instance of the store plugin. diff --git a/cache/stores/file/lib.php b/cache/stores/file/lib.php index bcfbf409d45..40d71b1683f 100644 --- a/cache/stores/file/lib.php +++ b/cache/stores/file/lib.php @@ -46,10 +46,16 @@ class cachestore_file extends cache_store implements cache_is_key_aware, cache_i protected $name; /** - * The path to use for the file storage. + * The path to use for the file storage. False otherwise. * @var string */ - protected $path = null; + protected $filestorepath = false; + + /** + * The path to use when the store instance has been initialised. False otherwise. + * @var string + */ + protected $path = false; /** * Set to true when a prescan has been performed. @@ -129,7 +135,9 @@ class cachestore_file extends cache_store implements cache_is_key_aware, cache_i $path = make_cache_directory('cachestore_file/'.preg_replace('#[^a-zA-Z0-9\.\-_]+#', '', $name)); } $this->isready = $path !== false; + $this->filestorepath = $path; $this->path = $path; + // Check if we should prescan the directory. if (array_key_exists('prescan', $configuration)) { $this->prescan = (bool)$configuration['prescan']; @@ -146,6 +154,16 @@ class cachestore_file extends cache_store implements cache_is_key_aware, cache_i } } + /** + * Performs any necessary operation when the file store instance has been created. + */ + public function instance_created() { + if ($this->isready && !$this->prescan) { + // It is supposed the store instance to expect an empty folder. + $this->purge_all_definitions(); + } + } + /** * Returns true if this store instance is ready to be used. * @return bool @@ -214,7 +232,7 @@ class cachestore_file extends cache_store implements cache_is_key_aware, cache_i public function initialise(cache_definition $definition) { $this->definition = $definition; $hash = preg_replace('#[^a-zA-Z0-9]+#', '_', $this->definition->get_id()); - $this->path .= '/'.$hash; + $this->path = $this->filestorepath.'/'.$hash; make_writable_directory($this->path); if ($this->prescan && $definition->get_mode() !== self::MODE_REQUEST) { $this->prescan = false; @@ -485,21 +503,40 @@ class cachestore_file extends cache_store implements cache_is_key_aware, cache_i } /** - * Purges the cache deleting all items within it. + * Purges the cache definition deleting all the items within it. * * @return boolean True on success. False otherwise. */ public function purge() { - $files = glob($this->glob_keys_pattern(), GLOB_MARK | GLOB_NOSORT); - if (is_array($files)) { - foreach ($files as $filename) { - @unlink($filename); + if ($this->isready) { + $files = glob($this->glob_keys_pattern(), GLOB_MARK | GLOB_NOSORT); + if (is_array($files)) { + foreach ($files as $filename) { + @unlink($filename); + } } + $this->keys = array(); } - $this->keys = array(); + return true; } + /** + * Purges all the cache definitions deleting all items within them. + * + * @return boolean True on success. False otherwise. + */ + protected function purge_all_definitions() { + // Warning: limit the deletion to what file store is actually able + // to create using the internal {@link purge()} providing the + // {@link $path} with a wildcard to perform a purge action over all the definitions. + $currpath = $this->path; + $this->path = $this->filestorepath.'/*'; + $result = $this->purge(); + $this->path = $currpath; + return $result; + } + /** * Given the data from the add instance form this function creates a configuration array. * @@ -567,16 +604,29 @@ class cachestore_file extends cache_store implements cache_is_key_aware, cache_i } /** - * Performs any necessary clean up when the store instance is being deleted. + * Performs any necessary clean up when the file store instance is being deleted. * * 1. Purges the cache directory. - * 2. Deletes the directory we created for this cache instances data. + * 2. Deletes the directory we created for the given definition. */ public function cleanup() { $this->purge(); @rmdir($this->path); } + /** + * Performs any necessary operation when the file store instance is being deleted, + * regardless the file store being initialised with a definition ({@link initialise()}). + * + * @see cleanup() + */ + public function instance_deleted() { + if ($this->isready && !$this->prescan) { + // Remove the content related to this file store. + $this->purge_all_definitions(); + } + } + /** * Generates an instance of the cache store that can be used for testing. * From 86afa1d00fb457102eedee3faa1ace9c976f7930 Mon Sep 17 00:00:00 2001 From: Matteo Scaramuccia Date: Sun, 2 Dec 2012 21:35:21 +0100 Subject: [PATCH 3/4] MDL-36363 Override instance_deleted() for all cache stores performing just a cleanup() when this action successfully applies over all the definitions of the cache store --- cache/stores/memcache/lib.php | 11 +++++++++++ cache/stores/memcached/lib.php | 11 +++++++++++ cache/stores/mongodb/lib.php | 11 +++++++++++ cache/stores/session/lib.php | 11 +++++++++++ cache/stores/static/lib.php | 11 +++++++++++ 5 files changed, 55 insertions(+) diff --git a/cache/stores/memcache/lib.php b/cache/stores/memcache/lib.php index 80c5a33c391..044831580f2 100644 --- a/cache/stores/memcache/lib.php +++ b/cache/stores/memcache/lib.php @@ -323,6 +323,17 @@ class cachestore_memcache extends cache_store implements cache_is_configurable { $this->purge(); } + /** + * Performs any necessary operation when the store instance is being deleted, + * regardless the store being initialised with a definition ({@link initialise()}). + * + * @link http://tracker.moodle.org/browse/MDL-36363 + * @see cleanup() + */ + public function instance_deleted() { + $this->cleanup(); + } + /** * Generates an instance of the cache store that can be used for testing. * diff --git a/cache/stores/memcached/lib.php b/cache/stores/memcached/lib.php index 44a11b9ca6b..f49d7077248 100644 --- a/cache/stores/memcached/lib.php +++ b/cache/stores/memcached/lib.php @@ -405,6 +405,17 @@ class cachestore_memcached extends cache_store implements cache_is_configurable $this->purge(); } + /** + * Performs any necessary operation when the store instance is being deleted, + * regardless the store being initialised with a definition ({@link initialise()}). + * + * @link http://tracker.moodle.org/browse/MDL-36363 + * @see cleanup() + */ + public function instance_deleted() { + $this->cleanup(); + } + /** * Generates an instance of the cache store that can be used for testing. * diff --git a/cache/stores/mongodb/lib.php b/cache/stores/mongodb/lib.php index 4ee7752cd86..f82b94fbd36 100644 --- a/cache/stores/mongodb/lib.php +++ b/cache/stores/mongodb/lib.php @@ -443,6 +443,17 @@ class cachestore_mongodb extends cache_store implements cache_is_configurable { $this->purge(); } + /** + * Performs any necessary operation when the store instance is being deleted, + * regardless the store being initialised with a definition ({@link initialise()}). + * + * @link http://tracker.moodle.org/browse/MDL-36363 + * @see cleanup() + */ + public function instance_deleted() { + $this->cleanup(); + } + /** * Generates an instance of the cache store that can be used for testing. * diff --git a/cache/stores/session/lib.php b/cache/stores/session/lib.php index 2ec9a948faa..28b41d70a90 100644 --- a/cache/stores/session/lib.php +++ b/cache/stores/session/lib.php @@ -379,6 +379,17 @@ class cachestore_session extends session_data_store implements cache_is_key_awar $this->purge(); } + /** + * Performs any necessary operation when the store instance is being deleted, + * regardless the store being initialised with a definition ({@link initialise()}). + * + * @link http://tracker.moodle.org/browse/MDL-36363 + * @see cleanup() + */ + public function instance_deleted() { + $this->cleanup(); + } + /** * Generates an instance of the cache store that can be used for testing. * diff --git a/cache/stores/static/lib.php b/cache/stores/static/lib.php index 7281fad49b1..53b7734487b 100644 --- a/cache/stores/static/lib.php +++ b/cache/stores/static/lib.php @@ -376,6 +376,17 @@ class cachestore_static extends static_data_store implements cache_is_key_aware $this->purge(); } + /** + * Performs any necessary operation when the store instance is being deleted, + * regardless the store being initialised with a definition ({@link initialise()}). + * + * @link http://tracker.moodle.org/browse/MDL-36363 + * @see cleanup() + */ + public function instance_deleted() { + $this->cleanup(); + } + /** * Generates an instance of the cache store that can be used for testing. * From a50cdcc0fd8dd0de34220b78d21532408691f3c0 Mon Sep 17 00:00:00 2001 From: Sam Hemelryk Date: Thu, 24 Jan 2013 15:09:20 +1300 Subject: [PATCH 4/4] MDL-36363 cache: clean up pre-integration --- cache/classes/store.php | 41 ++++++++++++++++++---------------- cache/locallib.php | 1 + cache/stores/file/lib.php | 33 +++++++++------------------ cache/stores/memcache/lib.php | 23 +++++++++---------- cache/stores/memcached/lib.php | 27 ++++++++++++---------- cache/stores/mongodb/lib.php | 29 +++++++++++++----------- cache/stores/session/lib.php | 13 +---------- cache/stores/static/lib.php | 13 +---------- cache/testperformance.php | 6 ++--- cache/upgrade.txt | 8 +++++++ 10 files changed, 88 insertions(+), 106 deletions(-) create mode 100644 cache/upgrade.txt diff --git a/cache/classes/store.php b/cache/classes/store.php index 219cda08b38..7cbb069dd95 100644 --- a/cache/classes/store.php +++ b/cache/classes/store.php @@ -135,15 +135,6 @@ abstract class cache_store implements cache_store_interface { */ abstract public function __construct($name, array $configuration = array()); - /** - * Performs any necessary operation when the store instance has been created. - * - * @link http://tracker.moodle.org/browse/MDL-36363 - */ - public function instance_created() { - // By default, do nothing. - } - /** * Returns the name of this store instance. * @return string @@ -235,23 +226,35 @@ abstract class cache_store implements cache_store_interface { /** * Performs any necessary clean up when the store instance is being deleted. * - * Please note that if the store has been already initialised with - * a definition ({@link initialise()}), cleanup will be performed against the scope - * of that definition i.e. the result depends on the specific store implementation. - * - * @see instance_deleted() + * @deprecated since 2.5 */ - abstract public function cleanup(); + public function cleanup() { + debugging('This function has been renamed to instance_deleted. Please update your calls.', DEBUG_DEVELOPER); + } /** - * Performs any necessary operation when the store instance is being deleted, - * regardless the store being initialised with a definition ({@link initialise()}). + * Performs any necessary operation when the store instance has been created. * - * @link http://tracker.moodle.org/browse/MDL-36363 + * @since 2.5 + */ + public function instance_created() { + // By default, do nothing. + } + + /** + * Performs any necessary operation when the store instance is being deleted. + * + * This method may be called before the store has been initialised. + * + * @since 2.5 * @see cleanup() */ public function instance_deleted() { - // By default, do nothing + if (method_exists($this, 'cleanup')) { + // There used to be a legacy function called cleanup, it was renamed to instance delete. + // To be removed in 2.6. + $this->cleanup(); + } } /** diff --git a/cache/locallib.php b/cache/locallib.php index 4982719d66d..02dd03eab53 100644 --- a/cache/locallib.php +++ b/cache/locallib.php @@ -308,6 +308,7 @@ class cache_config_writer extends cache_config { throw new cache_exception('You cannot delete a cache store that has definition mappings.'); } } + // Call instance_deleted() $class = 'cachestore_'.$this->configstores[$name]['plugin']; $store = new $class($name, $this->configstores[$name]['configuration']); diff --git a/cache/stores/file/lib.php b/cache/stores/file/lib.php index 40d71b1683f..855a223c8c1 100644 --- a/cache/stores/file/lib.php +++ b/cache/stores/file/lib.php @@ -46,17 +46,17 @@ class cachestore_file extends cache_store implements cache_is_key_aware, cache_i protected $name; /** - * The path to use for the file storage. False otherwise. - * @var string - */ - protected $filestorepath = false; - - /** - * The path to use when the store instance has been initialised. False otherwise. + * The path used to store files for this store and the definition it was initialised with. * @var string */ protected $path = false; + /** + * The path in which definition specific sub directories will be created for caching. + * @var string + */ + protected $filestorepath = false; + /** * Set to true when a prescan has been performed. * @var bool @@ -136,6 +136,7 @@ class cachestore_file extends cache_store implements cache_is_key_aware, cache_i } $this->isready = $path !== false; $this->filestorepath = $path; + // This will be updated once the store has been initialised for a definition. $this->path = $path; // Check if we should prescan the directory. @@ -517,7 +518,6 @@ class cachestore_file extends cache_store implements cache_is_key_aware, cache_i } $this->keys = array(); } - return true; } @@ -609,22 +609,9 @@ class cachestore_file extends cache_store implements cache_is_key_aware, cache_i * 1. Purges the cache directory. * 2. Deletes the directory we created for the given definition. */ - public function cleanup() { - $this->purge(); - @rmdir($this->path); - } - - /** - * Performs any necessary operation when the file store instance is being deleted, - * regardless the file store being initialised with a definition ({@link initialise()}). - * - * @see cleanup() - */ public function instance_deleted() { - if ($this->isready && !$this->prescan) { - // Remove the content related to this file store. - $this->purge_all_definitions(); - } + $this->purge_all_definitions(); + @rmdir($this->filestorepath); } /** diff --git a/cache/stores/memcache/lib.php b/cache/stores/memcache/lib.php index 044831580f2..d25b1c046d8 100644 --- a/cache/stores/memcache/lib.php +++ b/cache/stores/memcache/lib.php @@ -319,19 +319,18 @@ class cachestore_memcache extends cache_store implements cache_is_configurable { /** * Performs any necessary clean up when the store instance is being deleted. */ - public function cleanup() { - $this->purge(); - } - - /** - * Performs any necessary operation when the store instance is being deleted, - * regardless the store being initialised with a definition ({@link initialise()}). - * - * @link http://tracker.moodle.org/browse/MDL-36363 - * @see cleanup() - */ public function instance_deleted() { - $this->cleanup(); + if ($this->connection) { + $connection = $this->connection; + } else { + $connection = new Memcache; + foreach ($this->servers as $server) { + $connection->addServer($server[0], $server[1], true, $server[2]); + } + } + $connection->flush(); + unset($connection); + unset($this->connection); } /** diff --git a/cache/stores/memcached/lib.php b/cache/stores/memcached/lib.php index f49d7077248..4370a6d84b1 100644 --- a/cache/stores/memcached/lib.php +++ b/cache/stores/memcached/lib.php @@ -401,19 +401,22 @@ class cachestore_memcached extends cache_store implements cache_is_configurable /** * Performs any necessary clean up when the store instance is being deleted. */ - public function cleanup() { - $this->purge(); - } - - /** - * Performs any necessary operation when the store instance is being deleted, - * regardless the store being initialised with a definition ({@link initialise()}). - * - * @link http://tracker.moodle.org/browse/MDL-36363 - * @see cleanup() - */ public function instance_deleted() { - $this->cleanup(); + if ($this->connection) { + $connection = $this->connection; + } else { + $connection = new Memcached(crc32($this->name)); + $servers = $connection->getServerList(); + if (empty($servers)) { + foreach ($this->options as $key => $value) { + $connection->setOption($key, $value); + } + $connection->addServers($this->servers); + } + } + $connection->flush(); + unset($connection); + unset($this->connection); } /** diff --git a/cache/stores/mongodb/lib.php b/cache/stores/mongodb/lib.php index f82b94fbd36..cefc299f4b1 100644 --- a/cache/stores/mongodb/lib.php +++ b/cache/stores/mongodb/lib.php @@ -67,7 +67,7 @@ class cachestore_mongodb extends cache_store implements cache_is_configurable { * The Connection object * @var Mongo */ - protected $connection; + protected $connection = false; /** * The Database Object @@ -439,19 +439,22 @@ class cachestore_mongodb extends cache_store implements cache_is_configurable { /** * Performs any necessary clean up when the store instance is being deleted. */ - public function cleanup() { - $this->purge(); - } - - /** - * Performs any necessary operation when the store instance is being deleted, - * regardless the store being initialised with a definition ({@link initialise()}). - * - * @link http://tracker.moodle.org/browse/MDL-36363 - * @see cleanup() - */ public function instance_deleted() { - $this->cleanup(); + // We can't use purge here that acts upon a collection. + // Instead we must drop the named database. + if ($this->connection) { + $connection = $this->connection; + } else { + $connection = new Mongo($this->server, $this->options); + } + $database = $connection->selectDB($this->databasename); + $database->drop(); + $connection = null; + $database = null; + // Explicitly unset things to cause a close. + $this->collection = null; + $this->database = null; + $this->connection = null; } /** diff --git a/cache/stores/session/lib.php b/cache/stores/session/lib.php index 28b41d70a90..5a2b9a09d00 100644 --- a/cache/stores/session/lib.php +++ b/cache/stores/session/lib.php @@ -375,19 +375,8 @@ class cachestore_session extends session_data_store implements cache_is_key_awar /** * Performs any necessary clean up when the store instance is being deleted. */ - public function cleanup() { - $this->purge(); - } - - /** - * Performs any necessary operation when the store instance is being deleted, - * regardless the store being initialised with a definition ({@link initialise()}). - * - * @link http://tracker.moodle.org/browse/MDL-36363 - * @see cleanup() - */ public function instance_deleted() { - $this->cleanup(); + $this->purge(); } /** diff --git a/cache/stores/static/lib.php b/cache/stores/static/lib.php index 53b7734487b..6b4ab4ed58d 100644 --- a/cache/stores/static/lib.php +++ b/cache/stores/static/lib.php @@ -372,19 +372,8 @@ class cachestore_static extends static_data_store implements cache_is_key_aware /** * Performs any necessary clean up when the store instance is being deleted. */ - public function cleanup() { - $this->purge(); - } - - /** - * Performs any necessary operation when the store instance is being deleted, - * regardless the store being initialised with a definition ({@link initialise()}). - * - * @link http://tracker.moodle.org/browse/MDL-36363 - * @see cleanup() - */ public function instance_deleted() { - $this->cleanup(); + $this->purge(); } /** diff --git a/cache/testperformance.php b/cache/testperformance.php index 26796c1afcc..a4704dc25c8 100644 --- a/cache/testperformance.php +++ b/cache/testperformance.php @@ -100,7 +100,7 @@ foreach (get_plugin_list_with_file('cachestore', 'lib.php', true) as $plugin => } $result[5] = sprintf('%01.4f', microtime(true) - $start); $applicationtable->data[] = $result; - $store->cleanup(); + $store->instance_deleted(); } } @@ -136,7 +136,7 @@ foreach (get_plugin_list_with_file('cachestore', 'lib.php', true) as $plugin => } $result[5] = sprintf('%01.4f', microtime(true) - $start); $sessiontable->data[] = $result; - $store->cleanup(); + $store->instance_deleted(); } } @@ -172,7 +172,7 @@ foreach (get_plugin_list_with_file('cachestore', 'lib.php', true) as $plugin => } $result[5] = sprintf('%01.4f', microtime(true) - $start); $requesttable->data[] = $result; - $store->cleanup(); + $store->instance_deleted(); } } diff --git a/cache/upgrade.txt b/cache/upgrade.txt new file mode 100644 index 00000000000..72ac93bda2e --- /dev/null +++ b/cache/upgrade.txt @@ -0,0 +1,8 @@ +This files describes API changes in /cache/stores/* - cache store plugins. +Information provided here is intended especially for developers. + +=== 2.5 === +* cleanup method renamed to instance_deleted. + It is now called when the store is deleted as all comments suggested anyway. +* instance_created method added. + It is called when the store is created for the very first time. \ No newline at end of file