fix: #3496257 Race conditions in CacheCollector/State (again) (8d660fef) · Commits · project / drupal

core/lib/Drupal/Core/Cache/CacheCollector.php

+29 −27

Original line number	Diff line number	Diff line
		@@ -78,7 +78,7 @@ abstract class CacheCollector implements CacheCollectorInterface, DestructableIn
		* This is used to check if an invalidated cache item has been overwritten in
		* the meantime.
		*
		* @var int
		* @var float
		*/
		protected $cacheCreated;

		@@ -159,9 +159,9 @@ public function get($key) {
		* persistent caching in procedural code. Extending classes may wish to alter
		* 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.
		* should invalidate the cache item within a lock to ensure that another
		* request that starts with an empty cache item does not overwrite with the
		* previous value. For example: Drupal\Core\State\State.
		*/
		public function set($key, $value) {
		$this->lazyLoadCache();
		@@ -233,27 +233,31 @@ protected function updateCache($lock = TRUE) {
		// Lock cache writes to help avoid stampedes.
		$cid = $this->getCid();
		$lock_name = $cid . ':' . __CLASS__;
		if (!$lock \|\| $this->lock->acquire($lock_name)) {
		$write_cache = TRUE;
		$lock_acquired = FALSE;
		// Try to acquire a lock. However even if the lock is not acquired,
		// run all of the logic except for setting the cache item anyway, since we
		// may need to delete the item due to operations taken in ::set().
		if ($lock) {
		$lock_acquired = $this->lock->acquire($lock_name);
		}
		// Set and delete operations invalidate the cache item. Try to also load
		// an eventually invalidated cache entry, only update an invalidated cache
		// entry if the creation date did not change as this could result in an
		// an eventually invalidated cache entry. Only update an invalidated cache
		// entry if the creation date did not change, as this could result in an
		// inconsistent cache.
		if ($cache = $this->cache->get($cid, $this->cacheInvalidated)) {
		if ($this->cacheInvalidated && $cache->created != $this->cacheCreated) {
		// We have invalidated the cache in this request and got a different
		// cache entry. Do not attempt to overwrite data that might have been
		// changed in a different request. We'll let the cache rebuild in
		// later requests.
		$this->cache->delete($cid);
		$this->lock->release($lock_name);
		return;
		// If a cache item exists, but either there wasn't a cache item at the
		// beginning of this request, or it has changed, prevent writing back to
		// the cache.
		if ($cache->created !== $this->cacheCreated) {
		$write_cache = FALSE;
		}
		// 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;
		// If this request invalidated the cache, ensure the cache item is
		// invalidated. This always needs to happen whether the lock was acquired
		// or not.
		if ($this->cacheInvalidated) {
		$write_cache = FALSE;
		$this->cache->delete($cid);
		}
		$data = array_merge($cache->data, $data);
		}
		@@ -264,19 +268,17 @@ protected function updateCache($lock = TRUE) {
		// with incomplete data, since we won't be able to properly merge
		// the existing cache data from earlier with the new data.
		// A future request will properly hydrate the cache from scratch.
		if ($lock) {
		$this->lock->release($lock_name);
		}
		return;
		$write_cache = FALSE;
		}
		// Remove keys marked for deletion.
		foreach ($this->keysToRemove as $delete_key) {
		unset($data[$delete_key]);
		}
		if ($write_cache && (!$lock \|\| $lock_acquired)) {
		$this->cache->set($cid, $data, Cache::PERMANENT, $this->tags);
		if ($lock) {
		$this->lock->release($lock_name);
		}
		if ($lock_acquired) {
		$this->lock->release($lock_name);
		}

		$this->keysToPersist = [];

core/lib/Drupal/Core/State/State.php

+78 −18

Original line number	Diff line number	Diff line
		@@ -95,34 +95,94 @@ public function getMultiple(array $keys) {
		* {@inheritdoc}
		*/
		public function set($key, $value) {
		if (isset(self::$deprecatedState[$key])) {
		// phpcs:ignore Drupal.Semantics.FunctionTriggerError
		@trigger_error(self::$deprecatedState[$key]['message'], E_USER_DEPRECATED);
		$key = self::$deprecatedState[$key]['replacement'];
		}
		$this->registerKeySetDuringRequest($key, $value, parent::get($key));
		$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();
		$this->setMultiple([$key => $value]);
		}

		/**
		* {@inheritdoc}
		*/
		public function setMultiple(array $data) {
		$this->keyValueStore->setMultiple($data);
		$lock_name = $this->getCid() . ':' . CacheCollector::class;
		$lock_acquired = $this->lock->acquire($lock_name);
		$this->lazyLoadCache();
		foreach ($data as $key => $value) {
		$this->registerKeySetDuringRequest($key, $value, parent::get($key));
		parent::set($key, $value);
		if (isset(self::$deprecatedState[$key])) {
		// phpcs:ignore Drupal.Semantics.FunctionTriggerError
		@trigger_error(self::$deprecatedState[$key]['message'], E_USER_DEPRECATED);
		$data[self::$deprecatedState[$key]['replacement']] = $value;
		unset($data[$key]);
		}
		}
		$this->keyValueStore->setMultiple($data);
		// If another request had a cache miss before this request, and also hasn't
		// written to cache yet, then it may already have read the previous value
		// from the database and could write it to the cache at the end of the
		// request. To avoid this race condition, attempt to acquire a lock and
		// write to the cache immediately. This allows the race condition detection
		// in CacheCollector::updateCache() to work. We write to the cache whether
		// or not we acquire the lock, because CacheCollector::updateCache() handles
		// the case where there was no cache item at the beginning of the request,
		// but one was written by another request before ::updateCache() is called
		// - the new cache item functions as a tombstone record in this case.
		foreach ($data as $key => $value) {
		$this->storage[$key] = $value;
		// The key might have been marked for deletion.
		unset($this->keysToRemove[$key]);
		$this->persist($key);
		}
		if (!$lock_acquired) {
		// If we were unable to acquire a lock, immediately write the cache item
		// anyway. This acts as a tombstone for other requests that have not
		// reached a cache write yet. It also ensures that the end of this request
		// will detect that the cache item has changed in ::updateCache().
		$this->cache->set($this->getCid(), [], CacheBackendInterface::CACHE_PERMANENT, $this->tags);
		$this->cacheInvalidated = TRUE;
		// Wait for the lock to become available for a maximum of one second, then
		// attempt to acquire the lock again. If we can't acquire the lock, then
		// the one second that has passed should have given most processes that
		// were in progress time to complete anyway.
		$this->lock->wait($lock_name, 1);
		$lock_acquired = $this->lock->acquire($lock_name);
		// If we were unable to acquire the lock even after waiting, write the
		// cache item a second time, this will override any cache writes in the
		// interim.
		if (!$lock_acquired) {
		$this->cache->set($this->getCid(), [], CacheBackendInterface::CACHE_PERMANENT, $this->tags);
		}
		}
		if ($lock_acquired) {
		// Cache items are stored with millisecond precision, and are compared by
		// created time in CacheCollector. This allows for a race condition where:
		// Process A: writes a cache item.
		// Process B: reads the cache item.
		// Process C: (this process) writes a new cache item (all in the same
		// millisecond).
		// Process B: reaches CacheCollector::destruct(), and the race condition
		// protection logic compares the created timestamps of two different cache
		// items and finds them the same. By sleeping for 10 milliseconds both
		// prior to and after writing the cache item, we ensure that this
		// situation doesn't occur as long as the lock was acquired.
		// Only acquiring the lock isn't sufficient, because if the lock is
		// acquired and cache item set by two processes within the same
		// millisecond, the race condition detection won't detect that situation.
		// @todo This still doesn't account for the case where due to a clock
		// offset between servers, identical timestamps are recorded despite
		// happening at different times. Consider a more unique identifier in
		// CacheCollector.
		// @see https://www.drupal.org/project/drupal/issues/3496328
		usleep(10000);
		$this->cache->set($this->getCid(), $data, CacheBackendInterface::CACHE_PERMANENT, $this->tags);
		usleep(10000);

		// Because we've updated the cache within a lock here, we don't need to do
		// so again at the end of the request. Other requests can safely start
		// rebuilding the cache after this point.
		$this->lock->release($lock_name);
		$this->cacheInvalidated = FALSE;
		$this->keysToPersist = [];
		$this->keysToRemove = [];
		}
		}

		/**

core/modules/jsonapi/tests/src/FunctionalJavascript/JsonApiPerformanceTest.php

+4 −1

Original line number	Diff line number	Diff line
		@@ -46,11 +46,12 @@ public function testGetIndividual(): void {
		$user->addRole('administrator');
		$user->save();
		$this->drupalLogin($user);
		sleep(2);

		// Request the front page to ensure all cache collectors are fully
		// warmed, wait one second to ensure that the request finished processing.
		$this->drupalGet('');
		sleep(1);
		sleep(2);

		$url = Url::fromRoute('jsonapi.node--article.individual', ['entity' => $node->uuid()])->toString();
		$performance_data = $this->collectPerformanceData(function () use ($url) {
		@@ -132,6 +133,8 @@ public function testGetIndividual(): void {
		];
		$this->assertMetrics($expected, $performance_data);

		sleep(2);

		$url = Url::fromRoute('jsonapi.node--article.individual', ['entity' => $node->uuid()])->toString();
		$performance_data = $this->collectPerformanceData(function () use ($url) {
		$this->drupalGet($url);

core/tests/Drupal/Tests/Core/Cache/CacheCollectorTest.php

+96 −13

Original line number	Diff line number	Diff line
		@@ -12,6 +12,7 @@
		use Drupal\Tests\UnitTestCase;
		use PHPUnit\Framework\Attributes\CoversClass;
		use PHPUnit\Framework\Attributes\Group;
		use PHPUnit\Framework\Attributes\TestWith;

		/**
		* Tests Drupal\Core\Cache\CacheCollector.
		@@ -243,9 +244,23 @@ public function testUpdateCacheLockFail(): void {
		}

		/**
		* Tests updating the cache when there is a conflict after cache invalidation.
		* Tests setting to cache when there is a conflict after cache invalidation.
		*/
		public function testUpdateCacheInvalidatedConflict(): void {
		#[TestWith([TRUE, TRUE, TRUE, TRUE])]
		#[TestWith([TRUE, TRUE, TRUE, FALSE])]
		#[TestWith([TRUE, TRUE, FALSE, NULL])]
		#[TestWith([TRUE, FALSE, FALSE, NULL])]
		#[TestWith([TRUE, FALSE, TRUE, FALSE])]
		#[TestWith([FALSE, FALSE, FALSE, NULL])]
		#[TestWith([FALSE, TRUE, FALSE, NULL])]
		#[TestWith([FALSE, FALSE, TRUE, FALSE])]
		#[TestWith([FALSE, TRUE, TRUE, TRUE])]
		#[TestWith([FALSE, TRUE, TRUE, FALSE])]
		public function testSetCacheInvalidatedConflict(bool $lock_acquired, bool $start_cache_item, bool $end_cache_item, ?bool $timestamp_matches): void {

		if ($end_cache_item === FALSE && isset($timestamp_matches)) {
		throw new \BadMethodCallException('timestamp_matches is ignored when end_cache_item is FALSE');
		}
		$this->setUpMockCacheBackend();
		$this->setUpMockLockBackend();

		@@ -257,14 +272,14 @@ public function testUpdateCacheInvalidatedConflict(): void {
		->method('get')
		->with($this->cid)
		->willReturnOnConsecutiveCalls(
		(object) [
		$start_cache_item ? (object) [
		'data' => [$key => $value],
		'created' => (int) $_SERVER['REQUEST_TIME'],
		],
		(object) [
		] : FALSE,
		$end_cache_item ? (object) [
		'data' => [$key => $value],
		'created' => (int) $_SERVER['REQUEST_TIME'] + 1,
		],
		'created' => (int) $_SERVER['REQUEST_TIME'] + ($timestamp_matches ? 0 : 1),
		] : FALSE,
		);

		$this->cacheBackend->expects($this->once())
		@@ -277,14 +292,83 @@ public function testUpdateCacheInvalidatedConflict(): void {
		$this->lock->expects($this->once())
		->method('acquire')
		->with($this->cid . ':Drupal\Core\Cache\CacheCollector')
		->willReturn(TRUE);
		->willReturn($lock_acquired);
		if ($end_cache_item) {
		$this->cacheBackend->expects($this->once())
		->method('delete')
		->with($this->cid);
		}
		if ($lock_acquired) {
		$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 updating the cache when there is a conflict after cache invalidation.
		*/
		#[TestWith([TRUE, TRUE, TRUE, TRUE])]
		#[TestWith([TRUE, TRUE, TRUE, FALSE])]
		#[TestWith([TRUE, TRUE, FALSE, NULL])]
		#[TestWith([TRUE, FALSE, FALSE, NULL])]
		#[TestWith([TRUE, FALSE, TRUE, FALSE])]
		#[TestWith([FALSE, FALSE, FALSE, NULL])]
		#[TestWith([FALSE, TRUE, FALSE, NULL])]
		#[TestWith([FALSE, FALSE, TRUE, FALSE])]
		#[TestWith([FALSE, TRUE, TRUE, TRUE])]
		#[TestWith([FALSE, TRUE, TRUE, FALSE])]
		public function testUpdateCacheConflict(bool $lock_acquired, bool $start_cache_item, bool $end_cache_item, ?bool $timestamp_matches): void {

		if ($end_cache_item === FALSE && isset($timestamp_matches)) {
		throw new \BadMethodCallException('timestamp_matches is ignored when end_cache_item is FALSE');
		}
		$this->setUpMockCacheBackend();
		$this->setUpMockLockBackend();

		$key = $this->randomMachineName();
		$value = $this->randomMachineName();

		$this->collector->setCacheMissData($key, $value);
		$this->collector->setCacheMissData('another key', 'another value');

		// Set up mock cache get with conflicting entries.
		$this->cacheBackend->expects($this->exactly(2))
		->method('get')
		->with($this->cid)
		->willReturnOnConsecutiveCalls(
		$start_cache_item ? (object) [
		'data' => [],
		'created' => (int) $_SERVER['REQUEST_TIME'],
		] : FALSE,
		$end_cache_item ? (object) [
		'data' => ['something completely different' => $value],
		'created' => (int) $_SERVER['REQUEST_TIME'] + ($timestamp_matches ? 0 : 1),
		] : FALSE,
		);

		$this->collector->get($key);

		// When the cache is being warmed, if the lock can't be acquired, or if the
		// cache item has changed during the request, nothing should be set.
		$this->lock->expects($this->once())
		->method('acquire')
		->with($this->cid . ':Drupal\Core\Cache\CacheCollector')
		->willReturn($lock_acquired);
		if ($end_cache_item && !$timestamp_matches) {
		$this->cacheBackend->expects($this->never())
		->method('set')
		->with($this->cid);
		}
		if ($lock_acquired) {
		$this->lock->expects($this->once())
		->method('release')
		->with($this->cid . ':Drupal\Core\Cache\CacheCollector');
		}
		$this->cacheBackend->expects($this->never())
		->method('delete');
		// Destruct the object to trigger the update data process.
		$this->collector->destruct();
		}
		@@ -363,12 +447,11 @@ public function testUpdateCacheRace(): void {
		}

		/**
		* Tests updating the cache after a delete.
		* Tests deleting the cache after a delete.
		*/
		public function testUpdateCacheDelete(): void {
		$this->setUpMockCacheBackend();
		$this->setUpMockLockBackend();

		$key = $this->randomMachineName();
		$value = $this->randomMachineName();

		@@ -389,14 +472,14 @@ public function testUpdateCacheDelete(): void {
		$this->collector->delete($key);

		// Set up mock objects for the expected calls, first a lock acquire, then
		// a cache set and finally the lock is released again.
		// a cache delete and finally the lock is released again.
		$this->lock->expects($this->once())
		->method('acquire')
		->with($this->cid . ':Drupal\Core\Cache\CacheCollector')
		->willReturn(TRUE);
		$this->cacheBackend->expects($this->once())
		->method('set')
		->with($this->cid, [], Cache::PERMANENT, []);
		->method('delete')
		->with($this->cid);
		$this->lock->expects($this->once())
		->method('release')
		->with($this->cid . ':Drupal\Core\Cache\CacheCollector');

core/tests/Drupal/Tests/Core/Menu/MenuActiveTrailTest.php

+0 −3

Original line number	Diff line number	Diff line
		@@ -356,9 +356,6 @@ public function testGetCid(): void {
		->method('set')
		// Ensure we normalize the serialized data by sorting them.
		->with('active-trail:route:baby_llama:route_parameters:' . serialize(['a' => 0, 'b' => 1]));
		$this->lock
		->method('acquire')
		->willReturn(TRUE);
		$this->menuActiveTrail->destruct();
		}