Issue #3438424 by catch, Berdir, alexpott, longwave: [random test failures] Race condition in state when individual keys are set with an empty cache
Alex Pott
parent
8dbc3f3cd9
commit
3a3e618602
|
|
@ -156,7 +156,11 @@ abstract class CacheCollector implements CacheCollectorInterface, DestructableIn
|
|||
* value will only apply while the object is in scope and will not be written
|
||||
* back to the persistent cache. This follows a similar pattern to static vs.
|
||||
* persistent caching in procedural code. Extending classes may wish to alter
|
||||
* this behavior, for example by adding a call to persist().
|
||||
* this behavior, for example by adding a call to persist(). If you are
|
||||
* writing data to somewhere in addition to the cache item in ::set(), you
|
||||
* should call static::updateCache() at the end of your ::set implementation.
|
||||
* This avoids a race condition if another request starts with an empty cache
|
||||
* before your ::set() call. For example: Drupal\Core\State\State.
|
||||
*/
|
||||
public function set($key, $value) {
|
||||
$this->lazyLoadCache();
|
||||
|
|
@ -164,7 +168,6 @@ abstract class CacheCollector implements CacheCollectorInterface, DestructableIn
|
|||
// The key might have been marked for deletion.
|
||||
unset($this->keysToRemove[$key]);
|
||||
$this->invalidateCache();
|
||||
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -244,6 +247,13 @@ abstract class CacheCollector implements CacheCollectorInterface, DestructableIn
|
|||
$this->lock->release($lock_name);
|
||||
return;
|
||||
}
|
||||
// If there wasn't a cache item at the beginning of the request, but
|
||||
// there is now, then there has been a cache write in the interim.
|
||||
// Discard our data if so since the cache may have been written by
|
||||
// a request that was also setting data.
|
||||
if (!$this->cacheCreated) {
|
||||
return;
|
||||
}
|
||||
$data = array_merge($cache->data, $data);
|
||||
}
|
||||
elseif ($this->cacheCreated) {
|
||||
|
|
|
|||
|
|
@ -112,8 +112,15 @@ class State extends CacheCollector implements StateInterface {
|
|||
$key = self::$deprecatedState[$key]['replacement'];
|
||||
}
|
||||
$this->keyValueStore->set($key, $value);
|
||||
// If another request had a cache miss before this request, and also hasn't
|
||||
// written to cache yet, then it may already have read this value from the
|
||||
// database and could write that value to the cache to the end of the
|
||||
// request. To avoid this race condition, write to the cache immediately
|
||||
// after calling parent::set(). This allows the race condition detection in
|
||||
// CacheCollector::set() to work.
|
||||
parent::set($key, $value);
|
||||
$this->persist($key);
|
||||
static::updateCache();
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -60,6 +60,8 @@ class StandardPerformanceTest extends PerformanceTestBase {
|
|||
$expected_queries = [
|
||||
'SELECT "base_table"."id" AS "id", "base_table"."path" AS "path", "base_table"."alias" AS "alias", "base_table"."langcode" AS "langcode" FROM "path_alias" "base_table" WHERE ("base_table"."status" = 1) AND ("base_table"."alias" LIKE "/node" ESCAPE ' . "'\\\\'" . ') AND ("base_table"."langcode" IN ("en", "und")) ORDER BY "base_table"."langcode" ASC, "base_table"."id" DESC',
|
||||
'SELECT "name", "route", "fit" FROM "router" WHERE "pattern_outline" IN ( "/node" ) AND "number_parts" >= 1',
|
||||
'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "system.private_key" ) AND "collection" = "state"',
|
||||
'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "views.view_route_names" ) AND "collection" = "state"',
|
||||
'SELECT COUNT(*) AS "expression" FROM (SELECT 1 AS "expression" FROM "node_field_data" "node_field_data" WHERE ("node_field_data"."promote" = 1) AND ("node_field_data"."status" = 1)) "subquery"',
|
||||
'SELECT "node_field_data"."sticky" AS "node_field_data_sticky", "node_field_data"."created" AS "node_field_data_created", "node_field_data"."nid" AS "nid" FROM "node_field_data" "node_field_data" WHERE ("node_field_data"."promote" = 1) AND ("node_field_data"."status" = 1) ORDER BY "node_field_data_sticky" DESC, "node_field_data_created" DESC LIMIT 10 OFFSET 0',
|
||||
'SELECT "revision"."vid" AS "vid", "revision"."langcode" AS "langcode", "revision"."revision_uid" AS "revision_uid", "revision"."revision_timestamp" AS "revision_timestamp", "revision"."revision_log" AS "revision_log", "revision"."revision_default" AS "revision_default", "base"."nid" AS "nid", "base"."type" AS "type", "base"."uuid" AS "uuid", CASE "base"."vid" WHEN "revision"."vid" THEN 1 ELSE 0 END AS "isDefaultRevision" FROM "node" "base" INNER JOIN "node_revision" "revision" ON "revision"."vid" = "base"."vid" WHERE "base"."nid" IN (1)',
|
||||
|
|
@ -69,12 +71,16 @@ class StandardPerformanceTest extends PerformanceTestBase {
|
|||
'SELECT "t".* FROM "node__field_image" "t" WHERE ("entity_id" IN (1)) AND ("deleted" = 0) AND ("langcode" IN ("en", "und", "zxx")) ORDER BY "delta" ASC',
|
||||
'SELECT "t".* FROM "node__field_tags" "t" WHERE ("entity_id" IN (1)) AND ("deleted" = 0) AND ("langcode" IN ("en", "und", "zxx")) ORDER BY "delta" ASC',
|
||||
'SELECT "ces".* FROM "comment_entity_statistics" "ces" WHERE ("ces"."entity_id" IN (1)) AND ("ces"."entity_type" = "node")',
|
||||
'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "twig_extension_hash_prefix" ) AND "collection" = "state"',
|
||||
'SELECT "config"."name" AS "name" FROM "config" "config" WHERE ("collection" = "") AND ("name" LIKE "comment.type.%" ESCAPE ' . "'\\\\'" . ') ORDER BY "collection" ASC, "name" ASC',
|
||||
'SELECT "config"."name" AS "name" FROM "config" "config" WHERE ("collection" = "") AND ("name" LIKE "node.type.%" ESCAPE ' . "'\\\\'" . ') ORDER BY "collection" ASC, "name" ASC',
|
||||
'SELECT 1 AS "expression" FROM "path_alias" "base_table" WHERE ("base_table"."status" = 1) AND ("base_table"."path" LIKE "/node%" ESCAPE ' . "'\\\\'" . ') LIMIT 1 OFFSET 0',
|
||||
'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "theme:stark" ) AND "collection" = "config.entity.key_store.block"',
|
||||
'SELECT "menu_tree"."menu_name" AS "menu_name", "menu_tree"."route_name" AS "route_name", "menu_tree"."route_parameters" AS "route_parameters", "menu_tree"."url" AS "url", "menu_tree"."title" AS "title", "menu_tree"."description" AS "description", "menu_tree"."parent" AS "parent", "menu_tree"."weight" AS "weight", "menu_tree"."options" AS "options", "menu_tree"."expanded" AS "expanded", "menu_tree"."enabled" AS "enabled", "menu_tree"."provider" AS "provider", "menu_tree"."metadata" AS "metadata", "menu_tree"."class" AS "class", "menu_tree"."form_class" AS "form_class", "menu_tree"."id" AS "id" FROM "menu_tree" "menu_tree" WHERE ("route_name" = "view.frontpage.page_1") AND ("route_param_key" = "view_id=frontpage&display_id=page_1") AND ("menu_name" = "main") ORDER BY "depth" ASC, "weight" ASC, "id" ASC',
|
||||
'SELECT "menu_tree"."menu_name" AS "menu_name", "menu_tree"."route_name" AS "route_name", "menu_tree"."route_parameters" AS "route_parameters", "menu_tree"."url" AS "url", "menu_tree"."title" AS "title", "menu_tree"."description" AS "description", "menu_tree"."parent" AS "parent", "menu_tree"."weight" AS "weight", "menu_tree"."options" AS "options", "menu_tree"."expanded" AS "expanded", "menu_tree"."enabled" AS "enabled", "menu_tree"."provider" AS "provider", "menu_tree"."metadata" AS "metadata", "menu_tree"."class" AS "class", "menu_tree"."form_class" AS "form_class", "menu_tree"."id" AS "id" FROM "menu_tree" "menu_tree" WHERE ("route_name" = "view.frontpage.page_1") AND ("route_param_key" = "view_id=frontpage&display_id=page_1") AND ("menu_name" = "account") ORDER BY "depth" ASC, "weight" ASC, "id" ASC',
|
||||
'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "asset.css_js_query_string" ) AND "collection" = "state"',
|
||||
'INSERT INTO "semaphore" ("name", "value", "expire") VALUES ("state:Drupal\Core\Cache\CacheCollector", "LOCK_ID", "EXPIRE")',
|
||||
'DELETE FROM "semaphore" WHERE ("name" = "state:Drupal\Core\Cache\CacheCollector") AND ("value" = "LOCK_ID")',
|
||||
'INSERT INTO "semaphore" ("name", "value", "expire") VALUES ("theme_registry:runtime:stark:Drupal\Core\Utility\ThemeRegistry", "LOCK_ID", "EXPIRE")',
|
||||
'DELETE FROM "semaphore" WHERE ("name" = "theme_registry:runtime:stark:Drupal\Core\Utility\ThemeRegistry") AND ("value" = "LOCK_ID")',
|
||||
'INSERT INTO "semaphore" ("name", "value", "expire") VALUES ("library_info:stark:Drupal\Core\Cache\CacheCollector", "LOCK_ID", "EXPIRE")',
|
||||
|
|
@ -86,9 +92,9 @@ class StandardPerformanceTest extends PerformanceTestBase {
|
|||
];
|
||||
$recorded_queries = $performance_data->getQueries();
|
||||
$this->assertSame($expected_queries, $recorded_queries);
|
||||
$this->assertSame(25, $performance_data->getQueryCount());
|
||||
$this->assertSame(136, $performance_data->getCacheGetCount());
|
||||
$this->assertSame(47, $performance_data->getCacheSetCount());
|
||||
$this->assertSame(31, $performance_data->getQueryCount());
|
||||
$this->assertSame(137, $performance_data->getCacheGetCount());
|
||||
$this->assertSame(48, $performance_data->getCacheSetCount());
|
||||
$this->assertSame(0, $performance_data->getCacheDeleteCount());
|
||||
$this->assertCountBetween(38, 41, $performance_data->getCacheTagChecksumCount());
|
||||
$this->assertCountBetween(43, 46, $performance_data->getCacheTagIsValidCount());
|
||||
|
|
|
|||
|
|
@ -66,4 +66,14 @@ class CacheCollectorHelper extends CacheCollector {
|
|||
return $this->cacheMisses;
|
||||
}
|
||||
|
||||
/**
|
||||
* Setter for the cacheCreated property for use in unit tests.
|
||||
*
|
||||
* @param int $cache_created
|
||||
* A unix timestamp.
|
||||
*/
|
||||
public function setCacheCreated(int $cache_created):void {
|
||||
$this->cacheCreated = $cache_created;
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
|||
|
|
@ -261,7 +261,7 @@ class CacheCollectorTest extends UnitTestCase {
|
|||
}
|
||||
|
||||
/**
|
||||
* Tests updating the cache when a different request.
|
||||
* Tests a cache hit, then item updated by a different request.
|
||||
*/
|
||||
public function testUpdateCacheMerge() {
|
||||
$key = $this->randomMachineName();
|
||||
|
|
@ -270,6 +270,43 @@ class CacheCollectorTest extends UnitTestCase {
|
|||
$this->collector->setCacheMissData($key, $value);
|
||||
$this->collector->get($key);
|
||||
|
||||
// Set up mock objects for the expected calls, first a lock acquire, then
|
||||
// cache get to look for existing cache entries, which does find
|
||||
// and then it merges them.
|
||||
$this->lock->expects($this->once())
|
||||
->method('acquire')
|
||||
->with($this->cid . ':Drupal\Core\Cache\CacheCollector')
|
||||
->willReturn(TRUE);
|
||||
$cache = (object) [
|
||||
'data' => ['other key' => 'other value'],
|
||||
'created' => (int) $_SERVER['REQUEST_TIME'] + 1,
|
||||
];
|
||||
$this->collector->setCacheCreated($cache->created);
|
||||
$this->cacheBackend->expects($this->once())
|
||||
->method('get')
|
||||
->with($this->cid)
|
||||
->willReturn($cache);
|
||||
$this->cacheBackend->expects($this->once())
|
||||
->method('set')
|
||||
->with($this->cid, ['other key' => 'other value', $key => $value], Cache::PERMANENT, []);
|
||||
$this->lock->expects($this->once())
|
||||
->method('release')
|
||||
->with($this->cid . ':Drupal\Core\Cache\CacheCollector');
|
||||
|
||||
// Destruct the object to trigger the update data process.
|
||||
$this->collector->destruct();
|
||||
}
|
||||
|
||||
/**
|
||||
* Tests a cache miss, then item created by another request.
|
||||
*/
|
||||
public function testUpdateCacheRace() {
|
||||
$key = $this->randomMachineName();
|
||||
$value = $this->randomMachineName();
|
||||
|
||||
$this->collector->setCacheMissData($key, $value);
|
||||
$this->collector->get($key);
|
||||
|
||||
// Set up mock objects for the expected calls, first a lock acquire, then
|
||||
// cache get to look for existing cache entries, which does find
|
||||
// and then it merges them.
|
||||
|
|
@ -285,12 +322,6 @@ class CacheCollectorTest extends UnitTestCase {
|
|||
->method('get')
|
||||
->with($this->cid)
|
||||
->willReturn($cache);
|
||||
$this->cacheBackend->expects($this->once())
|
||||
->method('set')
|
||||
->with($this->cid, ['other key' => 'other value', $key => $value], Cache::PERMANENT, []);
|
||||
$this->lock->expects($this->once())
|
||||
->method('release')
|
||||
->with($this->cid . ':Drupal\Core\Cache\CacheCollector');
|
||||
|
||||
// Destruct the object to trigger the update data process.
|
||||
$this->collector->destruct();
|
||||
|
|
|
|||
Loading…
Reference in New Issue