From 9de35948b65fd2628cc05cf10d451d92cccaa134 Mon Sep 17 00:00:00 2001 From: Nathaniel Catchpole Date: Wed, 17 Feb 2016 09:54:31 +0900 Subject: [PATCH] Issue #2664302 by alexpott: Semaphore table is only used by a core service but it depends on system install --- .../Drupal/Core/Lock/DatabaseLockBackend.php | 114 +++++++++++++++++- .../system/src/Tests/Lock/LockUnitTest.php | 8 -- .../system/src/Tests/Update/DbDumpTest.php | 2 - core/modules/system/system.install | 31 ----- .../Kernel/Scripts/DbImportCommandTest.php | 1 - .../user/src/Tests/TempStoreDatabaseTest.php | 2 +- .../Core/Cache/CacheCollectorTest.php | 13 -- 7 files changed, 110 insertions(+), 61 deletions(-) diff --git a/core/lib/Drupal/Core/Lock/DatabaseLockBackend.php b/core/lib/Drupal/Core/Lock/DatabaseLockBackend.php index 267b6c18573..e252f1d81e6 100644 --- a/core/lib/Drupal/Core/Lock/DatabaseLockBackend.php +++ b/core/lib/Drupal/Core/Lock/DatabaseLockBackend.php @@ -9,6 +9,7 @@ namespace Drupal\Core\Lock; use Drupal\Core\Database\Connection; use Drupal\Core\Database\IntegrityConstraintViolationException; +use Drupal\Core\Database\SchemaObjectExistsException; /** * Defines the database lock backend. This is the default backend in Drupal. @@ -17,6 +18,11 @@ use Drupal\Core\Database\IntegrityConstraintViolationException; */ class DatabaseLockBackend extends LockBackendAbstract { + /** + * The database table name. + */ + const TABLE_NAME = 'semaphore'; + /** * The database connection. * @@ -84,6 +90,16 @@ class DatabaseLockBackend extends LockBackendAbstract { // the offending row from the database table in case it has expired. $retry = $retry ? FALSE : $this->lockMayBeAvailable($name); } + catch (\Exception $e) { + // Create the semaphore table if it does not exist and retry. + if ($this->ensureTableExists()) { + // Retry only once. + $retry = !$retry; + } + else { + throw $e; + } + } // We only retry in case the first attempt failed, but we then broke // an expired lock. } while ($retry); @@ -95,7 +111,14 @@ class DatabaseLockBackend extends LockBackendAbstract { * {@inheritdoc} */ public function lockMayBeAvailable($name) { - $lock = $this->database->query('SELECT expire, value FROM {semaphore} WHERE name = :name', array(':name' => $name))->fetchAssoc(); + try { + $lock = $this->database->query('SELECT expire, value FROM {semaphore} WHERE name = :name', array(':name' => $name))->fetchAssoc(); + } + catch (\Exception $e) { + $this->catchException($e); + // If the table does not exist yet then the lock may be available. + $lock = FALSE; + } if (!$lock) { return TRUE; } @@ -119,10 +142,15 @@ class DatabaseLockBackend extends LockBackendAbstract { */ public function release($name) { unset($this->locks[$name]); - $this->database->delete('semaphore') - ->condition('name', $name) - ->condition('value', $this->getLockId()) - ->execute(); + try { + $this->database->delete('semaphore') + ->condition('name', $name) + ->condition('value', $this->getLockId()) + ->execute(); + } + catch (\Exception $e) { + $this->catchException($e); + } } /** @@ -140,4 +168,80 @@ class DatabaseLockBackend extends LockBackendAbstract { ->execute(); } } + + /** + * Check if the semaphore table exists and create it if not. + */ + protected function ensureTableExists() { + try { + $database_schema = $this->database->schema(); + if (!$database_schema->tableExists(static::TABLE_NAME)) { + $schema_definition = $this->schemaDefinition(); + $database_schema->createTable(static::TABLE_NAME, $schema_definition); + return TRUE; + } + } + // If another process has already created the semaphore table, attempting to + // recreate it will throw an exception. In this case just catch the + // exception and do nothing. + catch (SchemaObjectExistsException $e) { + return TRUE; + } + return FALSE; + } + + /** + * Act on an exception when semaphore might be stale. + * + * If the table does not yet exist, that's fine, but if the table exists and + * yet the query failed, then the semaphore is stale and the exception needs + * to propagate. + * + * @param $e + * The exception. + * + * @throws \Exception + */ + protected function catchException(\Exception $e) { + if ($this->database->schema()->tableExists(static::TABLE_NAME)) { + throw $e; + } + } + + /** + * Defines the schema for the semaphore table. + */ + public function schemaDefinition() { + return [ + 'description' => 'Table for holding semaphores, locks, flags, etc. that cannot be stored as state since they must not be cached.', + 'fields' => [ + 'name' => [ + 'description' => 'Primary Key: Unique name.', + 'type' => 'varchar_ascii', + 'length' => 255, + 'not null' => TRUE, + 'default' => '' + ], + 'value' => [ + 'description' => 'A value for the semaphore.', + 'type' => 'varchar_ascii', + 'length' => 255, + 'not null' => TRUE, + 'default' => '' + ], + 'expire' => [ + 'description' => 'A Unix timestamp with microseconds indicating when the semaphore should expire.', + 'type' => 'float', + 'size' => 'big', + 'not null' => TRUE + ], + ], + 'indexes' => [ + 'value' => ['value'], + 'expire' => ['expire'], + ], + 'primary key' => ['name'], + ]; + } + } diff --git a/core/modules/system/src/Tests/Lock/LockUnitTest.php b/core/modules/system/src/Tests/Lock/LockUnitTest.php index ec249c5a01e..b726add0011 100644 --- a/core/modules/system/src/Tests/Lock/LockUnitTest.php +++ b/core/modules/system/src/Tests/Lock/LockUnitTest.php @@ -24,17 +24,9 @@ class LockUnitTest extends KernelTestBase { */ protected $lock; - /** - * Modules to enable. - * - * @var array - */ - public static $modules = array('system'); - protected function setUp() { parent::setUp(); $this->lock = new DatabaseLockBackend($this->container->get('database')); - $this->installSchema('system', 'semaphore'); } /** diff --git a/core/modules/system/src/Tests/Update/DbDumpTest.php b/core/modules/system/src/Tests/Update/DbDumpTest.php index 7cd099b954c..964433b7269 100644 --- a/core/modules/system/src/Tests/Update/DbDumpTest.php +++ b/core/modules/system/src/Tests/Update/DbDumpTest.php @@ -90,7 +90,6 @@ class DbDumpTest extends KernelTestBase { // Create some schemas so our export contains tables. $this->installSchema('system', [ 'key_value_expire', - 'semaphore', 'sessions', 'url_alias', ]); @@ -131,7 +130,6 @@ class DbDumpTest extends KernelTestBase { 'key_value_expire', 'menu_link_content', 'menu_link_content_data', - 'semaphore', 'sequences', 'sessions', 'url_alias', diff --git a/core/modules/system/system.install b/core/modules/system/system.install index 27077832011..a248066b896 100644 --- a/core/modules/system/system.install +++ b/core/modules/system/system.install @@ -968,37 +968,6 @@ function system_schema() { 'primary key' => array('name'), ); - $schema['semaphore'] = array( - 'description' => 'Table for holding semaphores, locks, flags, etc. that cannot be stored as state since they must not be cached.', - 'fields' => array( - 'name' => array( - 'description' => 'Primary Key: Unique name.', - 'type' => 'varchar_ascii', - 'length' => 255, - 'not null' => TRUE, - 'default' => '' - ), - 'value' => array( - 'description' => 'A value for the semaphore.', - 'type' => 'varchar_ascii', - 'length' => 255, - 'not null' => TRUE, - 'default' => '' - ), - 'expire' => array( - 'description' => 'A Unix timestamp with microseconds indicating when the semaphore should expire.', - 'type' => 'float', - 'size' => 'big', - 'not null' => TRUE - ), - ), - 'indexes' => array( - 'value' => array('value'), - 'expire' => array('expire'), - ), - 'primary key' => array('name'), - ); - $schema['sequences'] = array( 'description' => 'Stores IDs.', 'fields' => array( diff --git a/core/modules/system/tests/src/Kernel/Scripts/DbImportCommandTest.php b/core/modules/system/tests/src/Kernel/Scripts/DbImportCommandTest.php index 8cf23d5b719..446d3d8de71 100644 --- a/core/modules/system/tests/src/Kernel/Scripts/DbImportCommandTest.php +++ b/core/modules/system/tests/src/Kernel/Scripts/DbImportCommandTest.php @@ -42,7 +42,6 @@ class DbImportCommandTest extends KernelTestBase { 'key_value_expire', 'menu_link_content', 'menu_link_content_data', - 'semaphore', 'sessions', 'url_alias', 'user__roles', diff --git a/core/modules/user/src/Tests/TempStoreDatabaseTest.php b/core/modules/user/src/Tests/TempStoreDatabaseTest.php index 6e4d837ff6a..7d104df085d 100644 --- a/core/modules/user/src/Tests/TempStoreDatabaseTest.php +++ b/core/modules/user/src/Tests/TempStoreDatabaseTest.php @@ -61,7 +61,7 @@ class TempStoreDatabaseTest extends KernelTestBase { // Install system tables to test the key/value storage without installing a // full Drupal environment. - $this->installSchema('system', array('semaphore', 'key_value_expire')); + $this->installSchema('system', array('key_value_expire')); // Create several objects for testing. for ($i = 0; $i <= 3; $i++) { diff --git a/core/tests/Drupal/KernelTests/Core/Cache/CacheCollectorTest.php b/core/tests/Drupal/KernelTests/Core/Cache/CacheCollectorTest.php index c05d6304622..ff2bf6ae80d 100644 --- a/core/tests/Drupal/KernelTests/Core/Cache/CacheCollectorTest.php +++ b/core/tests/Drupal/KernelTests/Core/Cache/CacheCollectorTest.php @@ -17,19 +17,6 @@ use Symfony\Component\DependencyInjection\Reference; */ class CacheCollectorTest extends KernelTestBase { - /** - * {@inheritdoc} - */ - public static $modules = ['system']; - - /** - * {@inheritdoc} - */ - protected function setUp() { - parent::setUp(); - $this->installSchema('system', ['semaphore']); - } - /** * {@inheritdoc} */