diff --git a/cache/classes/definition.php b/cache/classes/definition.php index 0e5c62e44ba..2354dcaae60 100644 --- a/cache/classes/definition.php +++ b/cache/classes/definition.php @@ -796,10 +796,12 @@ class cache_definition { * @throws coding_exception */ public function set_identifiers(array $identifiers = array()) { - // If we are setting the exact same identifiers then just return as nothing really changed. - // We don't care about order as cache::make will use the same definition order all the time. - if ($identifiers === $this->identifiers) { - return false; + if ($this->identifiers !== null) { + throw new coding_exception("You can only set identifiers on initial definition creation." . + " Define a new cache to set different identifiers."); + } + if (!empty($identifiers) && !empty($this->invalidationevents)) { + throw new coding_exception("You cannot use event invalidation and identifiers at the same time."); } foreach ($this->requireidentifiers as $identifier) { diff --git a/cache/classes/factory.php b/cache/classes/factory.php index c174befec19..6ae461d7181 100644 --- a/cache/classes/factory.php +++ b/cache/classes/factory.php @@ -191,15 +191,18 @@ class cache_factory { * @return cache_application|cache_session|cache_request */ public function create_cache_from_definition($component, $area, array $identifiers = array(), $unused = null) { - $definitionname = $component.'/'.$area; + $identifierstring = empty($identifiers) ? '' : '/'.http_build_query($identifiers); + $definitionname = $component.'/'.$area.$identifierstring; if (isset($this->cachesfromdefinitions[$definitionname])) { $cache = $this->cachesfromdefinitions[$definitionname]; - $cache->set_identifiers($identifiers); return $cache; } $definition = $this->create_definition($component, $area); - $definition->set_identifiers($identifiers); - $cache = $this->create_cache($definition); + // Identifiers are cached as part of the cache creation, so we store a cloned version of the cache. + $cacheddefinition = clone($definition); + $cacheddefinition->set_identifiers($identifiers); + $cache = $this->create_cache($cacheddefinition); + // Loaders are always held onto to speed up subsequent requests. $this->cachesfromdefinitions[$definitionname] = $cache; return $cache; @@ -222,10 +225,14 @@ class cache_factory { * @return cache_application|cache_session|cache_request */ public function create_cache_from_params($mode, $component, $area, array $identifiers = array(), array $options = array()) { - $key = "{$mode}_{$component}_{$area}"; - if (array_key_exists($key, $this->cachesfromparams)) { + $identifierstring = empty($identifiers) ? '' : '_'.http_build_query($identifiers); + $key = "{$mode}_{$component}_{$area}{$identifierstring}"; + if (isset($this->cachesfromparams[$key])) { return $this->cachesfromparams[$key]; } + // Regular cache definitions are cached inside create_definition(). This is not the case for Adhoc definitions + // using load_adhoc(). They are built as a new object on each call. + // We do not need to clone the definition because we know it's new. $definition = cache_definition::load_adhoc($mode, $component, $area, $options); $definition->set_identifiers($identifiers); $cache = $this->create_cache($definition); diff --git a/cache/classes/helper.php b/cache/classes/helper.php index 30f82350699..f15e2ee8fb5 100644 --- a/cache/classes/helper.php +++ b/cache/classes/helper.php @@ -229,7 +229,8 @@ class cache_helper { /** * Invalidates a given set of keys by means of an event. * - * @todo add support for identifiers to be supplied and utilised. + * Events cannot determine what identifiers might need to be cleared. Event based purge and invalidation + * are only supported on caches without identifiers. * * @param string $event * @param array $keys @@ -292,8 +293,9 @@ class cache_helper { if ($cache instanceof cache_store) { $factory = cache_factory::instance(); $definition = $factory->create_definition($component, $area, null); - $definition->set_identifiers($identifiers); - $cache->initialise($definition); + $cacheddefinition = clone $definition; + $cacheddefinition->set_identifiers($identifiers); + $cache->initialise($cacheddefinition); } // Purge baby, purge. $cache->purge(); @@ -303,6 +305,9 @@ class cache_helper { /** * Purges a cache of all information on a given event. * + * Events cannot determine what identifiers might need to be cleared. Event based purge and invalidation + * are only supported on caches without identifiers. + * * @param string $event */ public static function purge_by_event($event) { diff --git a/cache/classes/loaders.php b/cache/classes/loaders.php index 3c5bd423ccb..9ea6df32080 100644 --- a/cache/classes/loaders.php +++ b/cache/classes/loaders.php @@ -275,6 +275,65 @@ class cache implements cache_loader { } } + /** + * Process any outstanding invalidation events for the cache we are registering, + * + * Identifiers and event invalidation are not compatible with each other at this time. + * As a result the cache does not need to consider identifiers when working out what to invalidate. + */ + protected function handle_invalidation_events() { + if (!$this->definition->has_invalidation_events()) { + return; + } + + $lastinvalidation = $this->get('lastinvalidation'); + if ($lastinvalidation === false) { + // This is a new cache or purged globally, there won't be anything to invalidate. + // Set the time of the last invalidation and move on. + $this->set('lastinvalidation', self::now()); + return; + } else if ($lastinvalidation == self::now()) { + // We've already invalidated during this request. + return; + } + + // Get the event invalidation cache. + $cache = self::make('core', 'eventinvalidation'); + $events = $cache->get_many($this->definition->get_invalidation_events()); + $todelete = array(); + $purgeall = false; + // Iterate the returned data for the events. + foreach ($events as $event => $keys) { + if ($keys === false) { + // No data to be invalidated yet. + continue; + } + // Look at each key and check the timestamp. + foreach ($keys as $key => $timestamp) { + // If the timestamp of the event is more than or equal to the last invalidation (happened between the last + // invalidation and now)then we need to invaliate the key. + if ($timestamp >= $lastinvalidation) { + if ($key === 'purged') { + $purgeall = true; + break; + } else { + $todelete[] = $key; + } + } + } + } + if ($purgeall) { + $this->purge(); + } else if (!empty($todelete)) { + $todelete = array_unique($todelete); + $this->delete_many($todelete); + } + // Set the time of the last invalidation. + if ($purgeall || !empty($todelete)) { + $this->set('lastinvalidation', self::now()); + } + } + /** * Retrieves the value for the given key from the cache. * @@ -1214,54 +1273,7 @@ class cache_application extends cache implements cache_loader_with_locking { $this->requirelockingwrite = $definition->require_locking_write(); } - if ($definition->has_invalidation_events()) { - $lastinvalidation = $this->get('lastinvalidation'); - if ($lastinvalidation === false) { - // This is a new session, there won't be anything to invalidate. Set the time of the last invalidation and - // move on. - $this->set('lastinvalidation', cache::now()); - return; - } else if ($lastinvalidation == cache::now()) { - // We've already invalidated during this request. - return; - } - - // Get the event invalidation cache. - $cache = cache::make('core', 'eventinvalidation'); - $events = $cache->get_many($definition->get_invalidation_events()); - $todelete = array(); - $purgeall = false; - // Iterate the returned data for the events. - foreach ($events as $event => $keys) { - if ($keys === false) { - // No data to be invalidated yet. - continue; - } - // Look at each key and check the timestamp. - foreach ($keys as $key => $timestamp) { - // If the timestamp of the event is more than or equal to the last invalidation (happened between the last - // invalidation and now)then we need to invaliate the key. - if ($timestamp >= $lastinvalidation) { - if ($key === 'purged') { - $purgeall = true; - break; - } else { - $todelete[] = $key; - } - } - } - } - if ($purgeall) { - $this->purge(); - } else if (!empty($todelete)) { - $todelete = array_unique($todelete); - $this->delete_many($todelete); - } - // Set the time of the last invalidation. - if ($purgeall || !empty($todelete)) { - $this->set('lastinvalidation', cache::now()); - } - } + $this->handle_invalidation_events(); } /** @@ -1615,54 +1627,7 @@ class cache_session extends cache { // This will trigger check tracked user. If this gets removed a call to that will need to be added here in its place. $this->set(self::LASTACCESS, cache::now()); - if ($definition->has_invalidation_events()) { - $lastinvalidation = $this->get('lastsessioninvalidation'); - if ($lastinvalidation === false) { - // This is a new session, there won't be anything to invalidate. Set the time of the last invalidation and - // move on. - $this->set('lastsessioninvalidation', cache::now()); - return; - } else if ($lastinvalidation == cache::now()) { - // We've already invalidated during this request. - return; - } - - // Get the event invalidation cache. - $cache = cache::make('core', 'eventinvalidation'); - $events = $cache->get_many($definition->get_invalidation_events()); - $todelete = array(); - $purgeall = false; - // Iterate the returned data for the events. - foreach ($events as $event => $keys) { - if ($keys === false) { - // No data to be invalidated yet. - continue; - } - // Look at each key and check the timestamp. - foreach ($keys as $key => $timestamp) { - // If the timestamp of the event is more than or equal to the last invalidation (happened between the last - // invalidation and now)then we need to invaliate the key. - if ($timestamp >= $lastinvalidation) { - if ($key === 'purged') { - $purgeall = true; - break; - } else { - $todelete[] = $key; - } - } - } - } - if ($purgeall) { - $this->purge(); - } else if (!empty($todelete)) { - $todelete = array_unique($todelete); - $this->delete_many($todelete); - } - // Set the time of the last invalidation. - if ($purgeall || !empty($todelete)) { - $this->set('lastsessioninvalidation', cache::now()); - } - } + $this->handle_invalidation_events(); } /** diff --git a/cache/disabledlib.php b/cache/disabledlib.php index 565f27e14d6..3a83d1fd643 100644 --- a/cache/disabledlib.php +++ b/cache/disabledlib.php @@ -212,6 +212,9 @@ class cache_factory_disabled extends cache_factory { * @return cache_application|cache_session|cache_request */ public function create_cache_from_definition($component, $area, array $identifiers = array(), $unused = null) { + // Regular cache definitions are cached inside create_definition(). This is not the case for disabledlib.php + // definitions as they use load_adhoc(). They are built as a new object on each call. + // We do not need to clone the definition because we know it's new. $definition = $this->create_definition($component, $area); $definition->set_identifiers($identifiers); $cache = $this->create_cache($definition); @@ -233,7 +236,10 @@ class cache_factory_disabled extends cache_factory { * @return cache_application|cache_session|cache_request */ public function create_cache_from_params($mode, $component, $area, array $identifiers = array(), array $options = array()) { - $definition = cache_definition::load_adhoc($mode, $component, $area); + // Regular cache definitions are cached inside create_definition(). This is not the case for disabledlib.php + // definitions as they use load_adhoc(). They are built as a new object on each call. + // We do not need to clone the definition because we know it's new. + $definition = cache_definition::load_adhoc($mode, $component, $area, $options); $definition->set_identifiers($identifiers); $cache = $this->create_cache($definition); return $cache; diff --git a/cache/tests/cache_test.php b/cache/tests/cache_test.php index ac6bfed2c08..ef4709670b0 100644 --- a/cache/tests/cache_test.php +++ b/cache/tests/cache_test.php @@ -188,7 +188,9 @@ class core_cache_testcase extends advanced_testcase { } /** - * Tests set_identifiers resets identifiers and static cache + * Tests set_identifiers fails post cache creation. + * + * set_identifiers cannot be called after initial cache instantiation, as you need to create a difference cache. */ public function test_set_identifiers() { $instance = cache_config_testing::instance(); @@ -204,16 +206,8 @@ class core_cache_testcase extends advanced_testcase { $this->assertTrue($cache->set('contest', 'test data 1')); $this->assertEquals('test data 1', $cache->get('contest')); + $this->expectException('coding_exception'); $cache->set_identifiers(array()); - $this->assertFalse($cache->get('contest')); - $this->assertTrue($cache->set('contest', 'empty ident')); - $this->assertEquals('empty ident', $cache->get('contest')); - - $cache->set_identifiers(array('area')); - $this->assertEquals('test data 1', $cache->get('contest')); - - $cache->set_identifiers(array()); - $this->assertEquals('empty ident', $cache->get('contest')); } /** @@ -2022,6 +2016,16 @@ class core_cache_testcase extends advanced_testcase { $this->assertEquals('b', $returnedinstance2->name); } + public function test_identifiers_have_separate_caches() { + $cachepg = cache::make('core', 'databasemeta', array('dbfamily' => 'pgsql')); + $cachepg->set(1, 'here'); + $cachemy = cache::make('core', 'databasemeta', array('dbfamily' => 'mysql')); + $cachemy->set(2, 'there'); + $this->assertEquals('here', $cachepg->get(1)); + $this->assertEquals('there', $cachemy->get(2)); + $this->assertFalse($cachemy->get(1)); + } + public function test_performance_debug() { global $CFG; $this->resetAfterTest(true); diff --git a/cache/upgrade.txt b/cache/upgrade.txt index e995cfff8eb..16bbbf59a50 100644 --- a/cache/upgrade.txt +++ b/cache/upgrade.txt @@ -1,6 +1,14 @@ This files describes API changes in /cache/stores/* - cache store plugins. Information provided here is intended especially for developers. +=== 3.3 === +* Identifiers and invalidation events have been explictly been marked as incompatible and will + throw a coding exception. Unexpected results would have occurred if the previous behaviour was attempted. +* Identifiers are now part of loaded caches, so identifiers can only be set at cache::make() + a coding_exception will be thrown if attempts are made at other times. + Multiple calls to cache::make with different identifiers will produce 2 caches instead of changing the + keyspace of a single cache. + === 3.2 === * The following methods have been finally deprecated and should no longer be used. - cache_definition::should_be_persistent() diff --git a/lib/dml/moodle_database.php b/lib/dml/moodle_database.php index 078c237e32f..82a98abb12c 100644 --- a/lib/dml/moodle_database.php +++ b/lib/dml/moodle_database.php @@ -124,6 +124,9 @@ abstract class moodle_database { /** @var cache_application for column info */ protected $metacache; + /** @var cache_request for column info on temp tables */ + protected $metacachetemp; + /** @var bool flag marking database instance as disposed */ protected $disposed; @@ -332,13 +335,14 @@ abstract class moodle_database { /** * Handle the creation and caching of the databasemeta information for all databases. * - * TODO MDL-53267 impelement caching of cache::make() results when it's safe to do so. - * * @return cache_application The databasemeta cachestore to complete operations on. */ protected function get_metacache() { - $properties = array('dbfamily' => $this->get_dbfamily(), 'settings' => $this->get_settings_hash()); - return cache::make('core', 'databasemeta', $properties); + if (!isset($this->metacache)) { + $properties = array('dbfamily' => $this->get_dbfamily(), 'settings' => $this->get_settings_hash()); + $this->metacache = cache::make('core', 'databasemeta', $properties); + } + return $this->metacache; } /** @@ -347,9 +351,12 @@ abstract class moodle_database { * @return cache_application The temp_tables cachestore to complete operations on. */ protected function get_temp_tables_cache() { - // Using connection data to prevent collisions when using the same temp table name with different db connections. - $properties = array('dbfamily' => $this->get_dbfamily(), 'settings' => $this->get_settings_hash()); - return cache::make('core', 'temp_tables', $properties); + if (!isset($this->metacachetemp)) { + // Using connection data to prevent collisions when using the same temp table name with different db connections. + $properties = array('dbfamily' => $this->get_dbfamily(), 'settings' => $this->get_settings_hash()); + $this->metacachetemp = cache::make('core', 'temp_tables', $properties); + } + return $this->metacachetemp; } /**