From 2e9d5a534e8e3045923055baa1609d66cec0affe Mon Sep 17 00:00:00 2001 From: Sam Hemelryk Date: Fri, 16 Aug 2013 11:22:15 +1200 Subject: [PATCH] MDL-41106 cache: several fixes for the session cache. This issue makes several fixes for the session loader and the session store. * maxsize argument now works for session caches. * fixed performance hole when interation occurs frequently. * fixed cache purge bug occuring when multiple caches are defined before being used. * improved lastaccess handling. --- cache/classes/loaders.php | 254 ++++++++++---------- cache/stores/session/lib.php | 28 ++- cache/stores/session/tests/session_test.php | 108 ++++++--- 3 files changed, 217 insertions(+), 173 deletions(-) diff --git a/cache/classes/loaders.php b/cache/classes/loaders.php index da8fe6758bf..69949c0b859 100644 --- a/cache/classes/loaders.php +++ b/cache/classes/loaders.php @@ -858,7 +858,7 @@ class cache implements cache_loader { * Returns the loader associated with this instance. * * @since 2.4.4 - * @return cache_loader|false + * @return cache|false */ protected function get_loader() { return $this->loader; @@ -1458,6 +1458,7 @@ class cache_application extends cache implements cache_loader_with_locking { * @todo we should support locking in the session as well. Should be pretty simple to set up. * * @internal don't use me directly. + * @method cache_store|cache_is_searchable get_store() Returns the cache store which must implement both cache_is_searchable. * * @package core * @category cache @@ -1494,6 +1495,11 @@ class cache_session extends cache { */ const KEY_PREFIX = 'sess_'; + /** + * This is the key used to track last access. + */ + const LASTACCESS = 'lastaccess'; + /** * Override the cache::construct method. * @@ -1507,12 +1513,15 @@ class cache_session extends cache { * @param cache_definition $definition * @param cache_store $store * @param cache_loader|cache_data_source $loader - * @return void */ public function __construct(cache_definition $definition, cache_store $store, $loader = null) { // First up copy the loadeduserid to the current user id. $this->currentuserid = self::$loadeduserid; parent::__construct($definition, $store, $loader); + + // 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) { @@ -1561,6 +1570,21 @@ class cache_session extends cache { } } + /** + * Sets the session id for the loader. + */ + protected function set_session_id() { + $this->sessionid = preg_replace('#[^a-zA-Z0-9_]#', '_', session_id()); + } + + /** + * Returns the prefix used for all keys. + * @return string + */ + protected function get_key_prefix() { + return 'u'.$this->currentuserid.'_'.$this->sessionid; + } + /** * Parses the key turning it into a string (or array is required) suitable to be passed to the cache store. * @@ -1573,17 +1597,18 @@ class cache_session extends cache { * @return string|array String unless the store supports multi-identifiers in which case an array if returned. */ protected function parse_key($key) { - if ($key === 'lastaccess') { - $key = '__lastaccess__'; + $prefix = $this->get_key_prefix(); + if ($key === self::LASTACCESS) { + return '__lastaccess__'.$prefix; } - return 'sess_'.parent::parse_key($key); + return $prefix.'_'.parent::parse_key($key); } /** * Check that this cache instance is tracking the current user. */ protected function check_tracked_user() { - if (isset($_SESSION['USER']->id)) { + if (isset($_SESSION['USER']->id) && $_SESSION['USER']->id !== null) { // Get the id of the current user. $new = $_SESSION['USER']->id; } else { @@ -1597,55 +1622,25 @@ class cache_session extends cache { // This way we don't bloat the session. $this->purge(); // Update the session id just in case! - $this->sessionid = session_id(); + $this->set_session_id(); } self::$loadeduserid = $new; $this->currentuserid = $new; } else if ($new !== $this->currentuserid) { // The current user matches the loaded user but not the user last used by this cache. - $this->purge(); + $this->purge_current_user(); $this->currentuserid = $new; // Update the session id just in case! - $this->sessionid = session_id(); + $this->set_session_id(); } } /** - * Gets the session data. - * - * @param bool $force If true the session data will be loaded from the store again. - * @return array An array of session data. + * Purges the session cache of all data belonging to the current user. */ - protected function get_session_data($force = false) { - if ($this->sessionid === null) { - $this->sessionid = session_id(); - } - if (is_array($this->session) && !$force) { - return $this->session; - } - $session = parent::get($this->sessionid); - if ($session === false) { - $session = array(); - } - // We have to write here to ensure that the lastaccess time is recorded. - // And also in order to ensure the session entry exists as when we save it on __destruct - // $CFG is likely to have already been destroyed. - $this->save_session($session); - return $this->session; - } - - /** - * Saves the session data. - * - * This function also updates the last access time. - * - * @param array $session - * @return bool - */ - protected function save_session(array $session) { - $session['lastaccess'] = time(); - $this->session = $session; - return parent::set($this->sessionid, $this->session); + public function purge_current_user() { + $keys = $this->get_store()->find_all($this->get_key_prefix()); + $this->get_store()->delete_many($keys); } /** @@ -1664,10 +1659,8 @@ class cache_session extends cache { // 2. Parse the key. $parsedkey = $this->parse_key($key); // 3. Get it from the store. - $result = false; - $session = $this->get_session_data(); - if (array_key_exists($parsedkey, $session)) { - $result = $session[$parsedkey]; + $result = $this->get_store()->get($parsedkey); + if ($result !== false) { if ($result instanceof cache_ttl_wrapper) { if ($result->has_expired()) { $this->get_store()->delete($parsedkey); @@ -1681,10 +1674,9 @@ class cache_session extends cache { } } // 4. Load if from the loader/datasource if we don't already have it. - $setaftervalidation = false; if ($result === false) { if ($this->perfdebug) { - cache_helper::record_cache_miss('**static session**', $this->get_definition()->get_id()); + cache_helper::record_cache_miss($this->storetype, $this->get_definition()->get_id()); } if ($this->get_loader() !== false) { // We must pass the original (unparsed) key to the next loader in the chain. @@ -1694,19 +1686,18 @@ class cache_session extends cache { } else if ($this->get_datasource() !== false) { $result = $this->get_datasource()->load_for_cache($key); } - $setaftervalidation = ($result !== false); + // 5. Set it to the store if we got it from the loader/datasource. + if ($result !== false) { + $this->set($key, $result); + } } else if ($this->perfdebug) { - cache_helper::record_cache_hit('**static session**', $this->get_definition()->get_id()); + cache_helper::record_cache_hit($this->storetype, $this->get_definition()->get_id()); } // 5. Validate strictness. if ($strictness === MUST_EXIST && $result === false) { throw new moodle_exception('Requested key did not exist in any cache stores and could not be loaded.'); } - // 6. Set it to the store if we got it from the loader/datasource. - if ($setaftervalidation) { - $this->set($key, $result); - } - // 7. Make sure we don't pass back anything that could be a reference. + // 6. Make sure we don't pass back anything that could be a reference. // We don't want people modifying the data in the cache. if (!is_scalar($result)) { // If data is an object it will be a reference. @@ -1738,7 +1729,7 @@ class cache_session extends cache { public function set($key, $data) { $this->check_tracked_user(); if ($this->perfdebug) { - cache_helper::record_cache_set('**static session**', $this->get_definition()->get_id()); + cache_helper::record_cache_set($this->storetype, $this->get_definition()->get_id()); } if (is_object($data) && $data instanceof cacheable_object) { $data = new cache_cached_object($data); @@ -1750,12 +1741,10 @@ class cache_session extends cache { $data = $this->unref($data); } // We dont' support native TTL here as we consolidate data for sessions. - if ($this->has_a_ttl()) { + if ($this->has_a_ttl() && !$this->store_supports_native_ttl()) { $data = new cache_ttl_wrapper($data, $this->get_definition()->get_ttl()); } - $session = $this->get_session_data(); - $session[$this->parse_key($key)] = $data; - return $this->save_session($session); + return $this->get_store()->set($this->parse_key($key), $data); } /** @@ -1767,15 +1756,12 @@ class cache_session extends cache { * @return bool True of success, false otherwise. */ public function delete($key, $recurse = true) { - $this->check_tracked_user(); $parsedkey = $this->parse_key($key); if ($recurse && $this->get_loader() !== false) { // Delete from the bottom of the stack first. $this->get_loader()->delete($key, $recurse); } - $session = $this->get_session_data(); - unset($session[$parsedkey]); - return $this->save_session($session); + return $this->get_store()->delete($parsedkey); } /** @@ -1798,11 +1784,37 @@ class cache_session extends cache { */ public function get_many(array $keys, $strictness = IGNORE_MISSING) { $this->check_tracked_user(); - $return = array(); + $parsedkeys = array(); + $keymap = array(); foreach ($keys as $key) { - $return[$key] = $this->get($key, $strictness); + $parsedkey = $this->parse_key($key); + $parsedkeys[] = $parsedkey; + $keymap[$parsedkey] = $key; + } + $result = $this->get_store()->get_many($parsedkeys); + $mustexist = $strictness === MUST_EXIST; + $return = array(); + foreach ($result as $parsedkey => $value) { + if ($value instanceof cache_ttl_wrapper) { + /* @var cache_ttl_wrapper $value */ + if ($value->has_expired()) { + $this->get_store()->delete($parsedkey); + $value = false; + } else { + $value = $value->data; + } + } + if ($value instanceof cache_cached_object) { + /* @var cache_cached_object $value */ + $value = $value->restore_object(); + } + if ($value === false && $mustexist) { + throw new moodle_exception('Not all the requested keys existed within the cache stores.'); + } + $return[$keymap[$parsedkey]] = $value; } return $return; + } /** @@ -1814,18 +1826,12 @@ class cache_session extends cache { * @return int The number of items successfully deleted. */ public function delete_many(array $keys, $recurse = true) { - $this->check_tracked_user(); $parsedkeys = array_map(array($this, 'parse_key'), $keys); if ($recurse && $this->get_loader() !== false) { // Delete from the bottom of the stack first. $this->get_loader()->delete_many($keys, $recurse); } - $session = $this->get_session_data(); - foreach ($parsedkeys as $parsedkey) { - unset($session[$parsedkey]); - } - $this->save_session($session); - return count($keys); + return $this->get_store()->delete_many($parsedkeys); } /** @@ -1853,8 +1859,9 @@ class cache_session extends cache { */ public function set_many(array $keyvaluearray) { $this->check_tracked_user(); - $session = $this->get_session_data(); - $simulatettl = $this->has_a_ttl(); + $data = array(); + $definitionid = $this->get_definition()->get_ttl(); + $simulatettl = $this->has_a_ttl() && !$this->store_supports_native_ttl(); foreach ($keyvaluearray as $key => $value) { if (is_object($value) && $value instanceof cacheable_object) { $value = new cache_cached_object($value); @@ -1866,16 +1873,17 @@ class cache_session extends cache { $value = $this->unref($value); } if ($simulatettl) { - $value = new cache_ttl_wrapper($value, $this->get_definition()->get_ttl()); + $value = new cache_ttl_wrapper($value, $definitionid); } - $parsedkey = $this->parse_key($key); - $session[$parsedkey] = $value; + $data[$key] = array( + 'key' => $this->parse_key($key), + 'value' => $value + ); } if ($this->perfdebug) { - cache_helper::record_cache_set($this->storetype, $this->get_definition()->get_id()); + cache_helper::record_cache_set($this->storetype, $definitionid); } - $this->save_session($session); - return count($keyvaluearray); + return $this->get_store()->set_many($data); } /** @@ -1884,13 +1892,9 @@ class cache_session extends cache { * @return bool True on success, false otherwise */ public function purge() { - // 1. Purge the session object. - $this->session = array(); - // 2. Delete the record for this users session from the store. - $this->get_store()->delete($this->sessionid); - // 3. Optionally purge any stacked loaders in the same way. + $this->get_store()->purge(); if ($this->get_loader()) { - $this->get_loader()->delete($this->sessionid); + $this->get_loader()->purge(); } return true; } @@ -1919,21 +1923,27 @@ class cache_session extends cache { public function has($key, $tryloadifpossible = false) { $this->check_tracked_user(); $parsedkey = $this->parse_key($key); - $session = $this->get_session_data(); - $has = false; - if ($this->has_a_ttl()) { + $store = $this->get_store(); + if ($this->has_a_ttl() && !$this->store_supports_native_ttl()) { // The data has a TTL and the store doesn't support it natively. // We must fetch the data and expect a ttl wrapper. - if (array_key_exists($parsedkey, $session)) { - $data = $session[$parsedkey]; - $has = ($data instanceof cache_ttl_wrapper && !$data->has_expired()); - } + $data = $store->get($parsedkey); + $has = ($data instanceof cache_ttl_wrapper && !$data->has_expired()); + } else if (!$this->store_supports_key_awareness()) { + // The store doesn't support key awareness, get the data and check it manually... puke. + // Either no TTL is set of the store supports its handling natively. + $data = $store->get($parsedkey); + $has = ($data !== false); } else { - $has = array_key_exists($parsedkey, $session); + // The store supports key awareness, this is easy! + // Either no TTL is set of the store supports its handling natively. + /* @var cache_store|cache_is_key_aware $store */ + $has = $store->has($parsedkey); } if (!$has && $tryloadifpossible) { + $result = null; if ($this->get_loader() !== false) { - $result = $this->get_loader()->get($key); + $result = $this->get_loader()->get($parsedkey); } else if ($this->get_datasource() !== null) { $result = $this->get_datasource()->load_for_cache($key); } @@ -1960,25 +1970,18 @@ class cache_session extends cache { */ public function has_all(array $keys) { $this->check_tracked_user(); - $session = $this->get_session_data(); - foreach ($keys as $key) { - $has = false; - $parsedkey = $this->parse_key($key); - if ($this->has_a_ttl()) { - // The data has a TTL and the store doesn't support it natively. - // We must fetch the data and expect a ttl wrapper. - if (array_key_exists($parsedkey, $session)) { - $data = $session[$parsedkey]; - $has = ($data instanceof cache_ttl_wrapper && !$data->has_expired()); + if (($this->has_a_ttl() && !$this->store_supports_native_ttl()) || !$this->store_supports_key_awareness()) { + foreach ($keys as $key) { + if (!$this->has($key)) { + return false; } - } else { - $has = array_key_exists($parsedkey, $session); - } - if (!$has) { - return false; } + return true; } - return true; + // The cache must be key aware and if support native ttl if it a ttl is set. + /* @var cache_store|cache_is_key_aware $store */ + $store = $this->get_store(); + return $store->has_all(array_map(array($this, 'parse_key'), $keys)); } /** @@ -1995,26 +1998,17 @@ class cache_session extends cache { * @return bool True if the cache has at least one of the given keys */ public function has_any(array $keys) { - $this->check_tracked_user(); - $session = $this->get_session_data(); - foreach ($keys as $key) { - $has = false; - $parsedkey = $this->parse_key($key); - if ($this->has_a_ttl()) { - // The data has a TTL and the store doesn't support it natively. - // We must fetch the data and expect a ttl wrapper. - if (array_key_exists($parsedkey, $session)) { - $data = $session[$parsedkey]; - $has = ($data instanceof cache_ttl_wrapper && !$data->has_expired()); + if (($this->has_a_ttl() && !$this->store_supports_native_ttl()) || !$this->store_supports_key_awareness()) { + foreach ($keys as $key) { + if ($this->has($key)) { + return true; } - } else { - $has = array_key_exists($parsedkey, $session); - } - if ($has) { - return true; } + return false; } - return false; + /* @var cache_store|cache_is_key_aware $store */ + $store = $this->get_store(); + return $store->has_any(array_map(array($this, 'parse_key'), $keys)); } /** diff --git a/cache/stores/session/lib.php b/cache/stores/session/lib.php index 82215023ef6..225b45b8251 100644 --- a/cache/stores/session/lib.php +++ b/cache/stores/session/lib.php @@ -237,7 +237,7 @@ class cachestore_session extends session_data_store implements cache_is_key_awar */ public function get($key) { if (isset($this->store[$key])) { - if ($this->ttl == 0) { + if ($this->ttl === 0) { return $this->store[$key][0]; } else if ($this->store[$key][1] >= (cache::now() - $this->ttl)) { return $this->store[$key][0]; @@ -257,6 +257,7 @@ class cachestore_session extends session_data_store implements cache_is_key_awar */ public function get_many($keys) { $return = array(); + $maxtime = 0; if ($this->ttl != 0) { $maxtime = cache::now() - $this->ttl; } @@ -284,11 +285,9 @@ class cachestore_session extends session_data_store implements cache_is_key_awar */ public function set($key, $data, $testmaxsize = true) { $testmaxsize = ($testmaxsize && $this->maxsize !== false); - if ($testmaxsize) { - $increment = (!isset($this->store[$key])); - } - if ($this->ttl == 0) { - $this->store[$key][0] = $data; + $increment = ($testmaxsize && !isset($this->store[$key])); + if ($this->ttl === 0) { + $this->store[$key] = array($data, 0); } else { $this->store[$key] = array($data, cache::now()); } @@ -311,12 +310,22 @@ class cachestore_session extends session_data_store implements cache_is_key_awar */ public function set_many(array $keyvaluearray) { $count = 0; + $increment = 0; foreach ($keyvaluearray as $pair) { - $this->set($pair['key'], $pair['value'], false); + $key = $pair['key']; + $data = $pair['value']; $count++; + if (!isset($this->store[$key])) { + $increment++; + } + if ($this->ttl === 0) { + $this->store[$key] = array($data, 0); + } else { + $this->store[$key] = array($data, cache::now()); + } } if ($this->maxsize !== false) { - $this->storecount += $count; + $this->storecount += $increment; if ($this->storecount > $this->maxsize) { $this->reduce_for_maxsize(); } @@ -348,6 +357,7 @@ class cachestore_session extends session_data_store implements cache_is_key_awar * @return bool */ public function has_all(array $keys) { + $maxtime = 0; if ($this->ttl != 0) { $maxtime = cache::now() - $this->ttl; } @@ -370,6 +380,7 @@ class cachestore_session extends session_data_store implements cache_is_key_awar * @return bool */ public function has_any(array $keys) { + $maxtime = 0; if ($this->ttl != 0) { $maxtime = cache::now() - $this->ttl; } @@ -502,6 +513,7 @@ class cachestore_session extends session_data_store implements cache_is_key_awar * Finds all of the keys whose keys start with the given prefix. * * @param string $prefix + * @return array An array of keys. */ public function find_by_prefix($prefix) { $return = array(); diff --git a/cache/stores/session/tests/session_test.php b/cache/stores/session/tests/session_test.php index 3ee20f2e2d1..9a8b15fa68c 100644 --- a/cache/stores/session/tests/session_test.php +++ b/cache/stores/session/tests/session_test.php @@ -49,60 +49,66 @@ class cachestore_session_test extends cachestore_tests { * Test the maxsize option. */ public function test_maxsize() { - $defid = 'phpunit/testmaxsize'; $config = cache_config_phpunittest::instance(); - $config->phpunit_add_definition($defid, array( + $config->phpunit_add_definition('phpunit/one', array( 'mode' => cache_store::MODE_SESSION, 'component' => 'phpunit', - 'area' => 'testmaxsize', + 'area' => 'one', 'maxsize' => 3 )); - $definition = cache_definition::load($defid, $config->get_definition_by_id($defid)); - $instance = cachestore_session::initialise_test_instance($definition); - $this->assertTrue($instance->set('key1', 'value1')); - $this->assertTrue($instance->set('key2', 'value2')); - $this->assertTrue($instance->set('key3', 'value3')); + $config->phpunit_add_definition('phpunit/two', array( + 'mode' => cache_store::MODE_SESSION, + 'component' => 'phpunit', + 'area' => 'two', + 'maxsize' => 3 + )); - $this->assertTrue($instance->has('key1')); - $this->assertTrue($instance->has('key2')); - $this->assertTrue($instance->has('key3')); + $cacheone = cache::make('phpunit', 'one'); - $this->assertTrue($instance->set('key4', 'value4')); - $this->assertTrue($instance->set('key5', 'value5')); + $this->assertTrue($cacheone->set('key1', 'value1')); + $this->assertTrue($cacheone->set('key2', 'value2')); + $this->assertTrue($cacheone->set('key3', 'value3')); - $this->assertFalse($instance->has('key1')); - $this->assertFalse($instance->has('key2')); - $this->assertTrue($instance->has('key3')); - $this->assertTrue($instance->has('key4')); - $this->assertTrue($instance->has('key5')); + $this->assertTrue($cacheone->has('key1')); + $this->assertTrue($cacheone->has('key2')); + $this->assertTrue($cacheone->has('key3')); - $this->assertFalse($instance->get('key1')); - $this->assertFalse($instance->get('key2')); - $this->assertEquals('value3', $instance->get('key3')); - $this->assertEquals('value4', $instance->get('key4')); - $this->assertEquals('value5', $instance->get('key5')); + $this->assertTrue($cacheone->set('key4', 'value4')); + $this->assertTrue($cacheone->set('key5', 'value5')); + + $this->assertFalse($cacheone->has('key1')); + $this->assertFalse($cacheone->has('key2')); + $this->assertTrue($cacheone->has('key3')); + $this->assertTrue($cacheone->has('key4')); + $this->assertTrue($cacheone->has('key5')); + + $this->assertFalse($cacheone->get('key1')); + $this->assertFalse($cacheone->get('key2')); + $this->assertEquals('value3', $cacheone->get('key3')); + $this->assertEquals('value4', $cacheone->get('key4')); + $this->assertEquals('value5', $cacheone->get('key5')); // Test adding one more. - $this->assertTrue($instance->set('key6', 'value6')); - $this->assertFalse($instance->get('key3')); + $this->assertTrue($cacheone->set('key6', 'value6')); + $this->assertFalse($cacheone->get('key3')); // Test reducing and then adding to make sure we don't lost one. - $this->assertTrue($instance->delete('key6')); - $this->assertTrue($instance->set('key7', 'value7')); - $this->assertEquals('value4', $instance->get('key4')); + $this->assertTrue($cacheone->delete('key6')); + $this->assertTrue($cacheone->set('key7', 'value7')); + $this->assertEquals('value4', $cacheone->get('key4')); // Set the same key three times to make sure it doesn't count overrides. for ($i = 0; $i < 3; $i++) { - $this->assertTrue($instance->set('key8', 'value8')); + $this->assertTrue($cacheone->set('key8', 'value8')); } - $this->assertEquals('value7', $instance->get('key7'), 'Overrides are incorrectly incrementing size'); + $this->assertEquals('value7', $cacheone->get('key7'), 'Overrides are incorrectly incrementing size'); // Test adding many. - $this->assertEquals(3, $instance->set_many(array( - array('key' => 'keyA', 'value' => 'valueA'), - array('key' => 'keyB', 'value' => 'valueB'), - array('key' => 'keyC', 'value' => 'valueC') + $this->assertEquals(3, $cacheone->set_many(array( + 'keyA' => 'valueA', + 'keyB' => 'valueB', + 'keyC' => 'valueC' ))); $this->assertEquals(array( 'key4' => false, @@ -112,8 +118,40 @@ class cachestore_session_test extends cachestore_tests { 'keyA' => 'valueA', 'keyB' => 'valueB', 'keyC' => 'valueC' - ), $instance->get_many(array( + ), $cacheone->get_many(array( 'key4', 'key5', 'key6', 'key7', 'keyA', 'keyB', 'keyC' ))); + + $cachetwo = cache::make('phpunit', 'two'); + + // Test adding many. + $this->assertEquals(3, $cacheone->set_many(array( + 'keyA' => 'valueA', + 'keyB' => 'valueB', + 'keyC' => 'valueC' + ))); + + $this->assertEquals(3, $cachetwo->set_many(array( + 'key1' => 'value1', + 'key2' => 'value2', + 'key3' => 'value3' + ))); + + $this->assertEquals(array( + 'keyA' => 'valueA', + 'keyB' => 'valueB', + 'keyC' => 'valueC' + ), $cacheone->get_many(array( + 'keyA', 'keyB', 'keyC' + ))); + + $this->assertEquals(array( + 'key1' => 'value1', + 'key2' => 'value2', + 'key3' => 'value3' + ), $cachetwo->get_many(array( + 'key1', 'key2', 'key3' + ))); + } } \ No newline at end of file -- 2.43.0