From 02652c7651ee2a4df7a46489962c30274000ddaf Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Tue, 13 Jun 2023 21:48:23 +0800 Subject: [PATCH] MDL-78466 cache: Perform strict test on static cache values If a statically accelerated cache returns an empty array then the value was still fetched from the non-static cache store. The check of the `$result` should be strictly checked against `false`, which is the value used if no value was found. --- cache/classes/loaders.php | 6 +- cache/tests/cache_test.php | 157 +++++++++++++++++++++++++++++++++++++ 2 files changed, 160 insertions(+), 3 deletions(-) diff --git a/cache/classes/loaders.php b/cache/classes/loaders.php index e78697be15a..80c96411b0d 100644 --- a/cache/classes/loaders.php +++ b/cache/classes/loaders.php @@ -457,7 +457,7 @@ class cache implements cache_loader { } } else { // If there's no result, obviously it doesn't meet the required version. - if (!$result) { + if ($result === false) { return false; } if (!($result instanceof \core_cache\version_wrapper)) { @@ -490,7 +490,7 @@ class cache implements cache_loader { if ($usesstaticacceleration) { $result = $this->static_acceleration_get($key); - if ($result && self::check_version($result, $requiredversion)) { + if ($result !== false && self::check_version($result, $requiredversion)) { if ($requiredversion === self::VERSION_NONE) { return $result; } else { @@ -505,7 +505,7 @@ class cache implements cache_loader { // 3. Get it from the store. Obviously wasn't in the static acceleration array. $result = $this->store->get($parsedkey); - if ($result) { + if ($result !== false) { // Check the result has at least the required version. try { $validversion = self::check_version($result, $requiredversion); diff --git a/cache/tests/cache_test.php b/cache/tests/cache_test.php index 5a660c34cd9..0c6cff0412c 100644 --- a/cache/tests/cache_test.php +++ b/cache/tests/cache_test.php @@ -50,6 +50,7 @@ use cacheable_object_array; * @copyright 2012 Sam Hemelryk * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later * @coversDefaultClass \cache + * @covers \cache */ class cache_test extends \advanced_testcase { @@ -2827,6 +2828,162 @@ class cache_test extends \advanced_testcase { $startstats[$requestid]['stores']['default_request']['sets']); } + /** + * Data provider for static acceleration performance tests. + * + * @return array + */ + public function static_acceleration_performance_provider(): array { + // Note: These are the delta values, not the absolute values. + // Also note that the set will actually store the valuein the static cache immediately. + $validfirst = [ + 'default_application' => [ + 'hits' => 1, + 'misses' => 0, + ], + cache_store::STATIC_ACCEL => [ + 'hits' => 0, + 'misses' => 1, + ], + ]; + + $validsecond = [ + 'default_application' => [ + 'hits' => 0, + 'misses' => 0, + ], + cache_store::STATIC_ACCEL => [ + 'hits' => 1, + 'misses' => 0, + ], + ]; + + $invalidfirst = [ + 'default_application' => [ + 'hits' => 0, + 'misses' => 1, + ], + cache_store::STATIC_ACCEL => [ + 'hits' => 0, + 'misses' => 1, + ], + ]; + $invalidsecond = [ + 'default_application' => [ + 'hits' => 0, + 'misses' => 1, + ], + cache_store::STATIC_ACCEL => [ + 'hits' => 0, + 'misses' => 1, + ], + ];; + + return [ + 'Truthy' => [ + true, + $validfirst, + $validsecond, + ], + 'Null' => [ + null, + $validfirst, + $validsecond, + ], + 'Empty Array' => [ + [], + $validfirst, + $validsecond, + ], + 'Empty String' => [ + '', + $validfirst, + $validsecond, + ], + 'False' => [ + false, + $invalidfirst, + $invalidsecond, + ], + ]; + } + + /** + * Test performance of static acceleration caches with values which are frequently confused with missing values. + * + * @dataProvider static_acceleration_performance_provider + * @param mixed $value The value to test + * @param array $firstfetchstats The expected stats on the first fetch + * @param array $secondfetchstats The expected stats on the subsequent fetch + */ + public function test_static_acceleration_values_performance( + $value, + array $firstfetchstats, + array $secondfetchstats + ): void { + // Note: We need to modify perfdebug to test this. + global $CFG; + $this->resetAfterTest(true); + $CFG->perfdebug = 15; + + $instance = cache_config_testing::instance(); + $instance->phpunit_add_definition('phpunit/accelerated', [ + 'mode' => cache_store::MODE_APPLICATION, + 'component' => 'phpunit', + 'area' => 'accelerated', + 'staticacceleration' => true, + 'staticaccelerationsize' => 1, + ]); + + $cache = cache::make('phpunit', 'accelerated'); + $this->assertInstanceOf(cache_phpunit_application::class, $cache); + + $this->assertTrue($cache->set('value', $value)); + + $checkstats = function( + array $start, + array $expectedstats + ): array { + $applicationid = 'phpunit/accelerated'; + $endstats = cache_helper::get_stats(); + + $start = $start[$applicationid]['stores']; + $end = $endstats[$applicationid]['stores']; + + foreach ($expectedstats as $cachename => $expected) { + foreach ($expected as $type => $value) { + $startvalue = array_key_exists($cachename, $start) ? $start[$cachename][$type] : 0; + $endvalue = array_key_exists($cachename, $end) ? $end[$cachename][$type] : 0; + $diff = $endvalue - $startvalue; + $this->assertEquals( + $value, + $diff, + "Expected $cachename $type to be $value, got $diff" + ); + } + } + + return $endstats; + }; + + // Reset the cache factory so that we can get the stats from a fresh instance. + $factory = cache_factory::instance(); + $factory->reset_cache_instances(); + $cache = cache::make('phpunit', 'accelerated'); + + // Get the initial stats. + $startstats = cache_helper::get_stats(); + + // Fetching the value the first time should seed the static cache from the application cache. + $this->assertEquals($value, $cache->get('value')); + $startstats = $checkstats($startstats, $firstfetchstats); + + // Fetching the value should only hit the static cache. + $this->assertEquals($value, $cache->get('value')); + $checkstats($startstats, $secondfetchstats); + } + + public function test_static_cache() { global $CFG; $this->resetAfterTest(true);