From d391b3472a1394028f9e4f6b934a2a6691e9ca51 Mon Sep 17 00:00:00 2001 From: Russell Smith Date: Thu, 18 Aug 2016 21:38:12 +1000 Subject: [PATCH] MDL-55604 cache: Fix identifier and cacheable_object When a cachable object is store in the static cache from the backing store, it was incorrect serialised rather than using the wake function. This has been resolved and tests added. During the investigation into cacheable_object, it was discovered that set_identifiers never removes identifiers when you call it, so set_identifiers(array('a')) and set_identifiers(array('b')) really resulted in array('a','b') as the identifiers rather than 'b'. The fix for this issue depends on the set_identifiers fix and they have been coupled together as a result. --- cache/classes/definition.php | 6 +-- cache/classes/loaders.php | 30 +++++++++------ cache/tests/cache_test.php | 74 +++++++++++++++++++++++++++++++++++- cache/tests/fixtures/lib.php | 34 +++++++++++++++-- 4 files changed, 124 insertions(+), 20 deletions(-) diff --git a/cache/classes/definition.php b/cache/classes/definition.php index a5fd64a6ca8..53f591583ae 100644 --- a/cache/classes/definition.php +++ b/cache/classes/definition.php @@ -781,10 +781,8 @@ class cache_definition { } } - if ($this->identifiers === null) { - // Initialize identifiers if they have not been. - $this->identifiers = array(); - } + $this->identifiers = array(); + foreach ($identifiers as $name => $value) { $this->identifiers[$name] = (string)$value; } diff --git a/cache/classes/loaders.php b/cache/classes/loaders.php index fcc22ba31d2..a5f01fe4cc0 100644 --- a/cache/classes/loaders.php +++ b/cache/classes/loaders.php @@ -306,13 +306,14 @@ class cache implements cache_loader { $result = $result->data; } } - if ($result instanceof cache_cached_object) { - $result = $result->restore_object(); - } if ($this->use_static_acceleration()) { $this->static_acceleration_set($parsedkey, $result); } + if ($result instanceof cache_cached_object) { + $result = $result->restore_object(); + } } + // 4. Load if from the loader/datasource if we don't already have it. $setaftervalidation = false; if ($result === false) { @@ -405,12 +406,12 @@ class cache implements cache_loader { $value = $value->data; } } - if ($value instanceof cache_cached_object) { - $value = $value->restore_object(); - } if ($value !== false && $this->use_static_acceleration()) { $this->static_acceleration_set($key, $value); } + if ($value instanceof cache_cached_object) { + $value = $value->restore_object(); + } $resultstore[$key] = $value; } } @@ -828,11 +829,7 @@ class cache implements cache_loader { */ public function purge() { // 1. Purge the static acceleration array. - $this->staticaccelerationarray = array(); - if ($this->staticaccelerationsize !== false) { - $this->staticaccelerationkeys = array(); - $this->staticaccelerationcount = 0; - } + $this->static_acceleration_purge(); // 2. Purge the store. $this->store->purge(); // 3. Optionally pruge any stacked loaders. @@ -1140,6 +1137,17 @@ class cache implements cache_loader { return true; } + /** + * Purge the static acceleration cache. + */ + protected function static_acceleration_purge() { + $this->staticaccelerationarray = array(); + if ($this->staticaccelerationsize !== false) { + $this->staticaccelerationkeys = array(); + $this->staticaccelerationcount = 0; + } + } + /** * Returns the timestamp from the first request for the time from the cache API. * diff --git a/cache/tests/cache_test.php b/cache/tests/cache_test.php index 26ab1454f63..255cbf32c13 100644 --- a/cache/tests/cache_test.php +++ b/cache/tests/cache_test.php @@ -187,6 +187,35 @@ class core_cache_testcase extends advanced_testcase { $this->assertEquals('test data 1', $cache->get('contest')); } + /** + * Tests set_identifiers resets identifiers and static cache + */ + public function test_set_identifiers() { + $instance = cache_config_testing::instance(); + $instance->phpunit_add_definition('phpunit/identifier', array( + 'mode' => cache_store::MODE_APPLICATION, + 'component' => 'phpunit', + 'area' => 'identifier', + 'simplekeys' => true, + 'simpledata' => true, + 'staticacceleration' => true + )); + $cache = cache::make('phpunit', 'identifier', array('area')); + $this->assertTrue($cache->set('contest', 'test data 1')); + $this->assertEquals('test data 1', $cache->get('contest')); + + $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')); + } + /** * Tests the default application cache */ @@ -306,15 +335,17 @@ class core_cache_testcase extends advanced_testcase { $this->assertTrue($cache->set($key, $dataobject)); $this->assertEquals($dataobject, $cache->get($key)); - $specobject = new cache_phpunit_dummy_object('red', 'blue'); + $starttime = microtime(true); + $specobject = new cache_phpunit_dummy_object('red', 'blue', $starttime); $this->assertTrue($cache->set($key, $specobject)); $result = $cache->get($key); $this->assertInstanceOf('cache_phpunit_dummy_object', $result); $this->assertEquals('red_ptc_wfc', $result->property1); $this->assertEquals('blue_ptc_wfc', $result->property2); + $this->assertGreaterThan($starttime, $result->propertytime); // Test array of objects. - $specobject = new cache_phpunit_dummy_object('red', 'blue'); + $specobject = new cache_phpunit_dummy_object('red', 'blue', $starttime); $data = new cacheable_object_array(array( clone($specobject), clone($specobject), @@ -328,6 +359,8 @@ class core_cache_testcase extends advanced_testcase { $this->assertInstanceOf('cache_phpunit_dummy_object', $item); $this->assertEquals('red_ptc_wfc', $item->property1); $this->assertEquals('blue_ptc_wfc', $item->property2); + // Ensure that wake from cache is called in all cases. + $this->assertGreaterThan($starttime, $item->propertytime); } // Test set many. @@ -1847,6 +1880,43 @@ class core_cache_testcase extends advanced_testcase { $this->assertEquals('D', $cache->phpunit_static_acceleration_get('d')); $this->assertEquals('E', $cache->phpunit_static_acceleration_get('e')); + // Store a cacheable_object, get many times and ensure each time wake_for_cache is used. + // Both get and get_many are tested. Two cache entries are used to ensure the times aren't + // confused with multiple calls to get()/get_many(). + $startmicrotime = microtime(true); + $cacheableobject = new cache_phpunit_dummy_object(1, 1, $startmicrotime); + $cacheableobject2 = new cache_phpunit_dummy_object(2, 2, $startmicrotime); + $this->assertTrue($cache->set('a', $cacheableobject)); + $this->assertTrue($cache->set('b', $cacheableobject2)); + $staticaccelerationreturntime = $cache->phpunit_static_acceleration_get('a')->propertytime; + $staticaccelerationreturntimeb = $cache->phpunit_static_acceleration_get('b')->propertytime; + $this->assertGreaterThan($startmicrotime, $staticaccelerationreturntime, 'Restore time of static must be newer.'); + + // Reset the static cache without resetting backing store. + $cache->phpunit_static_acceleration_purge(); + + // Get the value from the backend store, populating the static cache. + $cachevalue = $cache->get('a'); + $this->assertInstanceOf('cache_phpunit_dummy_object', $cachevalue); + $this->assertGreaterThan($staticaccelerationreturntime, $cachevalue->propertytime); + $backingstorereturntime = $cachevalue->propertytime; + + $results = $cache->get_many(array('b')); + $this->assertInstanceOf('cache_phpunit_dummy_object', $results['b']); + $this->assertGreaterThan($staticaccelerationreturntimeb, $results['b']->propertytime); + $backingstorereturntimeb = $results['b']->propertytime; + + // Obtain the value again and confirm that static cache is using wake_from_cache. + // Upon failure, the times are not adjusted as wake_from_cache is skipped as the + // value is stored serialized in the static acceleration cache. + $cachevalue = $cache->phpunit_static_acceleration_get('a'); + $this->assertInstanceOf('cache_phpunit_dummy_object', $cachevalue); + $this->assertGreaterThan($backingstorereturntime, $cachevalue->propertytime); + + $results = $cache->get_many(array('b')); + $this->assertInstanceOf('cache_phpunit_dummy_object', $results['b']); + $this->assertGreaterThan($backingstorereturntimeb, $results['b']->propertytime); + /** @var cache_phpunit_application $cache */ $cache = cache::make('phpunit', 'accelerated2'); $this->assertInstanceOf('cache_phpunit_application', $cache); diff --git a/cache/tests/fixtures/lib.php b/cache/tests/fixtures/lib.php index 8b3ef558964..a4b7094e71c 100644 --- a/cache/tests/fixtures/lib.php +++ b/cache/tests/fixtures/lib.php @@ -308,6 +308,9 @@ class cache_config_phpunittest extends cache_config_testing { /** * Dummy object for testing cacheable object interface and interaction * + * Wake from cache needs specific testing at times to ensure that during multiple + * cache get() requests it's possible to verify that it's getting woken each time. + * * @copyright 2012 Sam Hemelryk * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ @@ -322,21 +325,27 @@ class cache_phpunit_dummy_object extends stdClass implements cacheable_object { * @var string */ public $property2; + /** + * Test property time for verifying wake is run at each get() call. + * @var float + */ + public $propertytime; /** * Constructor * @param string $property1 * @param string $property2 */ - public function __construct($property1, $property2) { + public function __construct($property1, $property2, $propertytime = null) { $this->property1 = $property1; $this->property2 = $property2; + $this->propertytime = $propertytime === null ? microtime(true) : $propertytime; } /** * Prepares this object for caching * @return array */ public function prepare_to_cache() { - return array($this->property1.'_ptc', $this->property2.'_ptc'); + return array($this->property1.'_ptc', $this->property2.'_ptc', $this->propertytime); } /** * Returns this object from the cache @@ -344,7 +353,15 @@ class cache_phpunit_dummy_object extends stdClass implements cacheable_object { * @return cache_phpunit_dummy_object */ public static function wake_from_cache($data) { - return new cache_phpunit_dummy_object(array_shift($data).'_wfc', array_shift($data).'_wfc'); + $time = null; + if (!is_null($data[2])) { + // Windows 32bit microtime() resolution is 15ms, we ensure the time has moved forward. + do { + $time = microtime(true); + } while ($time == $data[2]); + + } + return new cache_phpunit_dummy_object(array_shift($data).'_wfc', array_shift($data).'_wfc', $time); } } @@ -426,6 +443,17 @@ class cache_phpunit_application extends cache_application { $key = $this->parse_key($key); return $this->static_acceleration_get($key); } + + /** + * Purges only the static acceleration while leaving the rest of the store in tack. + * + * Used for behaving like you have loaded 2 pages, and reset static while the backing store + * still contains all the same data. + * + */ + public function phpunit_static_acceleration_purge() { + $this->static_acceleration_purge(); + } } /**