Unverified Commit e3761163 authored by alexpott's avatar alexpott
Browse files

Issue #3085604 by bircher, catch, alexpott: Protect against concurrent config transformations

parent 2296602c
......@@ -13,6 +13,7 @@
@class_alias('Drupal\config_environment\Core\Config\StorageManagerInterface', 'Drupal\Core\Config\StorageManagerInterface');
@class_alias('Drupal\config_environment\Core\Config\ExportStorageManager', 'Drupal\Core\Config\ExportStorageManager');
@class_alias('Drupal\config_environment\Core\Config\ImportStorageTransformer', 'Drupal\Core\Config\ImportStorageTransformer');
@class_alias('Drupal\config_environment\Core\Config\StorageTransformerException', 'Drupal\Core\Config\StorageTransformerException');
use Drupal\Core\Routing\RouteMatchInterface;
......
......@@ -2,13 +2,13 @@ services:
# @todo: Move this back to core services in #2991683
config.import_transformer:
class: Drupal\config_environment\Core\Config\ImportStorageTransformer
arguments: ['@event_dispatcher', '@database']
arguments: ['@event_dispatcher', '@database', '@lock', '@lock.persistent']
config.storage.export:
class: Drupal\config_environment\Core\Config\ManagedStorage
arguments: ['@config.storage.export.manager']
config.storage.export.manager:
class: Drupal\config_environment\Core\Config\ExportStorageManager
arguments: ['@config.storage', '@database', '@event_dispatcher']
arguments: ['@config.storage', '@database', '@event_dispatcher', '@lock']
# config_environment services.
config_environment.excluded_modules.event_subscriber:
class: Drupal\config_environment\EventSubscriber\ExcludedModulesEventSubscriber
......
......@@ -45,6 +45,7 @@ final class ConfigEvents {
*
* @see \Drupal\Core\Config\StorageTransformEvent
* @see \Drupal\Core\Config\ConfigEvents::STORAGE_TRANSFORM_EXPORT
* @see \Drupal\Core\Config\ImportStorageTransformer::transform
*
* @var string
*/
......
......@@ -11,6 +11,7 @@
use Drupal\Core\Config\StorageCopyTrait;
use Drupal\Core\Config\StorageInterface;
use Drupal\Core\Database\Connection;
use Drupal\Core\Lock\LockBackendInterface;
use Symfony\Component\EventDispatcher\EventDispatcherInterface;
/**
......@@ -22,6 +23,11 @@ class ExportStorageManager implements StorageManagerInterface {
use StorageCopyTrait;
/**
* The name used to identify the lock.
*/
const LOCK_NAME = 'config_storage_export_manager';
/**
* The active configuration storage.
*
......@@ -43,6 +49,13 @@ class ExportStorageManager implements StorageManagerInterface {
*/
protected $eventDispatcher;
/**
* The used lock backend instance.
*
* @var \Drupal\Core\Lock\LockBackendInterface
*/
protected $lock;
/**
* ExportStorageManager constructor.
*
......@@ -52,10 +65,13 @@ class ExportStorageManager implements StorageManagerInterface {
* The database connection.
* @param \Symfony\Component\EventDispatcher\EventDispatcherInterface $event_dispatcher
* The event dispatcher.
* @param \Drupal\Core\Lock\LockBackendInterface $lock
* The used lock backend instance.
*/
public function __construct(StorageInterface $active, Connection $connection, EventDispatcherInterface $event_dispatcher) {
public function __construct(StorageInterface $active, Connection $connection, EventDispatcherInterface $event_dispatcher, LockBackendInterface $lock) {
$this->active = $active;
$this->eventDispatcher = $event_dispatcher;
$this->lock = $lock;
// The point of this service is to provide the storage and dispatch the
// event when needed, so the storage itself can not be a service.
$this->storage = new DatabaseStorage($connection, 'config_export');
......@@ -65,6 +81,15 @@ public function __construct(StorageInterface $active, Connection $connection, Ev
* {@inheritdoc}
*/
public function getStorage() {
// Acquire a lock for the request to assert that the storage does not change
// when a concurrent request transforms the storage.
if (!$this->lock->acquire(self::LOCK_NAME)) {
$this->lock->wait(self::LOCK_NAME);
if (!$this->lock->acquire(self::LOCK_NAME)) {
throw new StorageTransformerException("Cannot acquire config export transformer lock.");
}
}
self::replaceStorageContents($this->active, $this->storage);
// @todo: Use ConfigEvents::STORAGE_TRANSFORM_EXPORT in #2991683
$this->eventDispatcher->dispatch('config.transform.export', new StorageTransformEvent($this->storage));
......
......@@ -6,7 +6,9 @@
// @codingStandardsIgnoreEnd
namespace Drupal\config_environment\Core\Config;
use Drupal\Core\Config\ConfigImporter;
use Drupal\Core\Database\Connection;
use Drupal\Core\Lock\LockBackendInterface;
use Symfony\Component\EventDispatcher\EventDispatcherInterface;
use Drupal\Core\Config\DatabaseStorage;
use Drupal\Core\Config\StorageCopyTrait;
......@@ -21,6 +23,11 @@ class ImportStorageTransformer {
use StorageCopyTrait;
/**
* The name used to identify the lock.
*/
const LOCK_NAME = 'config_import_transformer';
/**
* The event dispatcher to get changes to the configuration.
*
......@@ -35,6 +42,22 @@ class ImportStorageTransformer {
*/
protected $connection;
/**
* The normal lock for the duration of the request.
*
* @var \Drupal\Core\Lock\LockBackendInterface
*/
protected $requestLock;
/**
* The persistent lock which the config importer uses across requests.
*
* @see \Drupal\Core\Config\ConfigImporter::alreadyImporting()
*
* @var \Drupal\Core\Lock\LockBackendInterface
*/
protected $persistentLock;
/**
* ImportStorageTransformer constructor.
*
......@@ -42,10 +65,16 @@ class ImportStorageTransformer {
* The event dispatcher.
* @param \Drupal\Core\Database\Connection $connection
* The database connection.
* @param \Drupal\Core\Lock\LockBackendInterface $requestLock
* The lock for the request.
* @param \Drupal\Core\Lock\LockBackendInterface $persistentLock
* The persistent lock used by the config importer.
*/
public function __construct(EventDispatcherInterface $event_dispatcher, Connection $connection) {
public function __construct(EventDispatcherInterface $event_dispatcher, Connection $connection, LockBackendInterface $requestLock, LockBackendInterface $persistentLock) {
$this->eventDispatcher = $event_dispatcher;
$this->connection = $connection;
$this->requestLock = $requestLock;
$this->persistentLock = $persistentLock;
}
/**
......@@ -63,11 +92,32 @@ public function __construct(EventDispatcherInterface $event_dispatcher, Connecti
*
* @return \Drupal\Core\Config\StorageInterface
* The transformed storage ready to be imported from.
*
* @throws \Drupal\Core\Config\StorageTransformerException
* Thrown when the lock could not be acquired.
*/
public function transform(StorageInterface $storage) {
// We use a database storage to reduce the memory requirement.
$mutable = new DatabaseStorage($this->connection, 'config_import');
if (!$this->persistentLock->lockMayBeAvailable(ConfigImporter::LOCK_NAME)) {
// If the config importer is already importing, the transformation will
// always be the one the config importer is already using. This makes sure
// that even if the storage changes the importer continues importing the
// same configuration.
return $mutable;
}
// Acquire a lock to ensure that the storage is not changed when a
// concurrent request tries to transform the storage. The lock will be
// released at the end of the request.
if (!$this->requestLock->acquire(self::LOCK_NAME)) {
$this->requestLock->wait(self::LOCK_NAME);
if (!$this->requestLock->acquire(self::LOCK_NAME)) {
throw new StorageTransformerException("Cannot acquire config import transformer lock.");
}
}
// Copy the sync configuration to the created mutable storage.
self::replaceStorageContents($storage, $mutable);
......
......@@ -18,6 +18,9 @@ interface StorageManagerInterface {
*
* @return \Drupal\Core\Config\StorageInterface
* The config storage.
*
* @throws \Drupal\Core\Config\StorageTransformerException
* Thrown when the lock could not be acquired.
*/
public function getStorage();
......
<?php
namespace Drupal\config_environment\Core\Config;
/**
* Thrown by config storage transformers if they cannot acquire a lock.
*
* @see \Drupal\Core\Config\ImportStorageTransformer
* @see \Drupal\Core\Config\ExportStorageManager
*/
class StorageTransformerException extends \Exception {}
......@@ -5,6 +5,9 @@
// @codingStandardsIgnoreEnd
namespace Drupal\Tests\config_environment\Kernel\Core\Config;
use Drupal\Core\Config\ExportStorageManager;
use Drupal\Core\Config\StorageTransformerException;
use Drupal\Core\Lock\NullLockBackend;
use Drupal\KernelTests\KernelTestBase;
/**
......@@ -39,7 +42,15 @@ public function testGetStorage() {
$rawConfig = $this->config('system.site')->getRawData();
$this->container->get('config.storage.sync')->write('system.site', $rawConfig);
$storage = $this->container->get('config.storage.export.manager')->getStorage();
// The export storage manager under test.
$manager = new ExportStorageManager(
$this->container->get('config.storage'),
$this->container->get('database'),
$this->container->get('event_dispatcher'),
new NullLockBackend()
);
$storage = $manager->getStorage();
$exported = $storage->read('system.site');
// The test subscriber adds "Arrr" to the slogan of the sync config.
$this->assertEquals($rawConfig['name'], $exported['name']);
......@@ -52,7 +63,7 @@ public function testGetStorage() {
->save();
// Get the storage again.
$storage = $this->container->get('config.storage.export.manager')->getStorage();
$storage = $manager->getStorage();
$exported = $storage->read('system.site');
// The test subscriber adds "Arrr" to the slogan of the sync config.
$this->assertEquals('New name', $exported['name']);
......@@ -61,10 +72,40 @@ public function testGetStorage() {
// Change what the transformer does without changing anything else to assert
// that the event is dispatched every time the storage is needed.
$this->container->get('state')->set('config_transform_test_mail', 'config@drupal.example');
$storage = $this->container->get('config.storage.export.manager')->getStorage();
$storage = $manager->getStorage();
$exported = $storage->read('system.site');
// The mail is still set to the value from the beginning.
$this->assertEquals('config@drupal.example', $exported['mail']);
}
/**
* Test the export storage when it is locked.
*/
public function testGetStorageLock() {
$lock = $this->createMock('Drupal\Core\Lock\LockBackendInterface');
$lock->expects($this->at(0))
->method('acquire')
->with(ExportStorageManager::LOCK_NAME)
->will($this->returnValue(FALSE));
$lock->expects($this->at(1))
->method('wait')
->with(ExportStorageManager::LOCK_NAME);
$lock->expects($this->at(2))
->method('acquire')
->with(ExportStorageManager::LOCK_NAME)
->will($this->returnValue(FALSE));
// The export storage manager under test.
$manager = new ExportStorageManager(
$this->container->get('config.storage'),
$this->container->get('database'),
$this->container->get('event_dispatcher'),
$lock
);
$this->expectException(StorageTransformerException::class);
$this->expectExceptionMessage("Cannot acquire config export transformer lock.");
$manager->getStorage();
}
}
......@@ -5,7 +5,11 @@
// @codingStandardsIgnoreEnd
namespace Drupal\Tests\config_environment\Kernel\Core\Config;
use Drupal\Core\Config\ConfigImporter;
use Drupal\Core\Config\ImportStorageTransformer;
use Drupal\Core\Config\MemoryStorage;
use Drupal\Core\Config\StorageTransformerException;
use Drupal\Core\Lock\NullLockBackend;
use Drupal\KernelTests\KernelTestBase;
/**
......@@ -60,4 +64,66 @@ public function testTransform() {
$this->assertEquals('New slogan', $config['slogan']);
}
/**
* Test that the import transformer throws an exception.
*/
public function testTransformLocked() {
// Mock the request lock not being available.
$lock = $this->createMock('Drupal\Core\Lock\LockBackendInterface');
$lock->expects($this->at(0))
->method('acquire')
->with(ImportStorageTransformer::LOCK_NAME)
->will($this->returnValue(FALSE));
$lock->expects($this->at(1))
->method('wait')
->with(ImportStorageTransformer::LOCK_NAME);
$lock->expects($this->at(2))
->method('acquire')
->with(ImportStorageTransformer::LOCK_NAME)
->will($this->returnValue(FALSE));
// The import transformer under test.
$transformer = new ImportStorageTransformer(
$this->container->get('event_dispatcher'),
$this->container->get('database'),
$lock,
new NullLockBackend()
);
$this->expectException(StorageTransformerException::class);
$this->expectExceptionMessage("Cannot acquire config import transformer lock.");
$transformer->transform(new MemoryStorage());
}
/**
* Test the import transformer during a running config import.
*/
public function testTransformWhileImporting() {
// Set up the database table with the current active config.
// This simulates the config importer having its transformation done.
$storage = $this->container->get('config.import_transformer')->transform($this->container->get('config.storage'));
// Mock the persistent lock being unavailable due to a config import.
$lock = $this->createMock('Drupal\Core\Lock\LockBackendInterface');
$lock->expects($this->at(0))
->method('lockMayBeAvailable')
->with(ConfigImporter::LOCK_NAME)
->will($this->returnValue(FALSE));
// The import transformer under test.
$transformer = new ImportStorageTransformer(
$this->container->get('event_dispatcher'),
$this->container->get('database'),
new NullLockBackend(),
$lock
);
// Transform an empty memory storage.
$import = $transformer->transform(new MemoryStorage());
// Assert that the transformed storage is the same as the one used to
// set up the simulated config importer.
$this->assertEquals($storage->listAll(), $import->listAll());
$this->assertNotEmpty($import->read('system.site'));
}
}
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment