Commit e0b4b7ef authored by catch's avatar catch
Browse files

Issue #3240601 by alexpott, daffie, daniel.bosen: The standard logic we use in...

Issue #3240601 by alexpott, daffie, daniel.bosen: The standard logic we use in ::ensureTableExists() is wrong
parent 364ecc6c
......@@ -165,19 +165,18 @@ protected function doCreate(array $batch) {
protected function ensureTableExists() {
try {
$database_schema = $this->connection->schema();
if (!$database_schema->tableExists(static::TABLE_NAME)) {
$schema_definition = $this->schemaDefinition();
$database_schema->createTable(static::TABLE_NAME, $schema_definition);
return TRUE;
}
$schema_definition = $this->schemaDefinition();
$database_schema->createTable(static::TABLE_NAME, $schema_definition);
}
// If another process has already created the batch table, attempting to
// recreate it will throw an exception. In this case just catch the
// exception and do nothing.
catch (DatabaseException $e) {
return TRUE;
}
return FALSE;
catch (\Exception $e) {
return FALSE;
}
return TRUE;
}
/**
......
......@@ -159,6 +159,10 @@ public function reset() {
*
* @return int[]
* List of invalidation counts keyed by the respective cache tag.
*
* @throws \Exception
* Thrown if the table could not be created or the database connection
* failed.
*/
abstract protected function getTagInvalidationCounts(array $tags);
......@@ -175,6 +179,10 @@ abstract protected function getDatabaseConnection();
*
* @param string[] $tags
* The set of tags for which to invalidate cache items.
*
* @throws \Exception
* Thrown if the table could not be created or the database connection
* failed.
*/
abstract protected function doInvalidateTags(array $tags);
......
......@@ -47,7 +47,7 @@ protected function doInvalidateTags(array $tags) {
// core install where cache tags are invalidated before the table is
// created.
if (!$this->ensureTableExists()) {
$this->catchException($e);
throw $e;
}
}
}
......@@ -63,7 +63,7 @@ protected function getTagInvalidationCounts(array $tags) {
catch (\Exception $e) {
// If the table does not exist yet, create.
if (!$this->ensureTableExists()) {
$this->catchException($e);
throw $e;
}
}
return [];
......@@ -75,21 +75,18 @@ protected function getTagInvalidationCounts(array $tags) {
protected function ensureTableExists() {
try {
$database_schema = $this->connection->schema();
// Create the cache tags table if it does not exist.
if (!$database_schema->tableExists('cachetags')) {
$schema_definition = $this->schemaDefinition();
$database_schema->createTable('cachetags', $schema_definition);
return TRUE;
}
$schema_definition = $this->schemaDefinition();
$database_schema->createTable('cachetags', $schema_definition);
}
// If another process has already created the cachetags table, attempting to
// recreate it will throw an exception. In this case just catch the
// exception and do nothing.
catch (DatabaseException $e) {
return TRUE;
}
return FALSE;
catch (\Exception $e) {
return FALSE;
}
return TRUE;
}
/**
......@@ -131,8 +128,14 @@ public function schemaDefinition() {
* The exception.
*
* @throws \Exception
*
* @deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. There is no
* replacement.
*
* @see https://www.drupal.org/node/3243014
*/
protected function catchException(\Exception $e) {
@trigger_error('\Drupal\Core\Cache\DatabaseCacheTagsChecksum::catchException is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. There is no replacement. See https://www.drupal.org/node/3243014', E_USER_DEPRECATED);
if ($this->connection->schema()->tableExists('cachetags')) {
throw $e;
}
......
......@@ -128,7 +128,7 @@ public function write($name, array $data) {
return $this->doWrite($name, $data);
}
// Some other failure that we can not recover from.
throw $e;
throw new StorageException($e->getMessage(), 0, $e);
}
}
......@@ -161,10 +161,7 @@ protected function doWrite($name, $data) {
*/
protected function ensureTableExists() {
try {
if (!$this->connection->schema()->tableExists($this->table)) {
$this->connection->schema()->createTable($this->table, static::schemaDefinition());
return TRUE;
}
$this->connection->schema()->createTable($this->table, static::schemaDefinition());
}
// If another process has already created the config table, attempting to
// recreate it will throw an exception. In this case just catch the
......@@ -173,9 +170,9 @@ protected function ensureTableExists() {
return TRUE;
}
catch (\Exception $e) {
throw new StorageException($e->getMessage(), 0, $e);
return FALSE;
}
return FALSE;
return TRUE;
}
/**
......
......@@ -150,19 +150,18 @@ public function garbageCollection() {
protected function ensureTableExists() {
try {
$database_schema = $this->connection->schema();
if (!$database_schema->tableExists(static::TABLE_NAME)) {
$schema_definition = $this->schemaDefinition();
$database_schema->createTable(static::TABLE_NAME, $schema_definition);
return TRUE;
}
$schema_definition = $this->schemaDefinition();
$database_schema->createTable(static::TABLE_NAME, $schema_definition);
}
// If another process has already created the table, attempting to create
// it will throw an exception. In this case just catch the exception and do
// nothing.
catch (DatabaseException $e) {
return TRUE;
}
return FALSE;
catch (\Exception $e) {
return FALSE;
}
return TRUE;
}
/**
......
......@@ -5,7 +5,7 @@
use Drupal\Component\Serialization\SerializationInterface;
use Drupal\Core\Database\Query\Merge;
use Drupal\Core\Database\Connection;
use Drupal\Core\Database\SchemaObjectExistsException;
use Drupal\Core\Database\DatabaseException;
use Drupal\Core\DependencyInjection\DependencySerializationTrait;
/**
......@@ -255,24 +255,26 @@ public function deleteAll() {
protected function ensureTableExists() {
try {
$database_schema = $this->connection->schema();
if (!$database_schema->tableExists($this->table)) {
$database_schema->createTable($this->table, $this->schemaDefinition());
return TRUE;
}
$database_schema->createTable($this->table, $this->schemaDefinition());
}
// If the table already exists, then attempting to recreate it will throw an
// exception. In this case just catch the exception and do nothing.
catch (SchemaObjectExistsException $e) {
return TRUE;
catch (DatabaseException $e) {
}
catch (\Exception $e) {
return FALSE;
}
return FALSE;
return TRUE;
}
/**
* Act on an exception when the table might not have been created.
*
* If the table does not yet exist, that's fine, but if the table exists and
* yet the query failed, then the exception needs to propagate.
* yet the query failed, then the exception needs to propagate if it is not
* a DatabaseException. Due to race conditions it is possible that another
* request has created the table in the meantime. Therefore we can not rethrow
* for any database exception.
*
* @param \Exception $e
* The exception.
......@@ -280,7 +282,7 @@ protected function ensureTableExists() {
* @throws \Exception
*/
protected function catchException(\Exception $e) {
if ($this->connection->schema()->tableExists($this->table)) {
if (!($e instanceof DatabaseException) && $this->connection->schema()->tableExists($this->table)) {
throw $e;
}
}
......
......@@ -177,19 +177,18 @@ public function releaseAll($lock_id = NULL) {
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;
}
$schema_definition = $this->schemaDefinition();
$database_schema->createTable(static::TABLE_NAME, $schema_definition);
}
// 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 (DatabaseException $e) {
return TRUE;
}
return FALSE;
catch (\Exception $e) {
return FALSE;
}
return TRUE;
}
/**
......
......@@ -247,7 +247,7 @@ protected function safeExecuteSelect(SelectInterface $query) {
return $query->execute();
}
// Some other failure that we can not recover from.
throw $e;
throw new PluginException($e->getMessage(), 0, $e);
}
}
......@@ -1157,27 +1157,20 @@ protected function treeDataRecursive(array &$links, array $parents, $depth) {
*
* @return bool
* TRUE if the table was created, FALSE otherwise.
*
* @throws \Drupal\Component\Plugin\Exception\PluginException
* If a database error occurs.
*/
protected function ensureTableExists() {
try {
if (!$this->connection->schema()->tableExists($this->table)) {
$this->connection->schema()->createTable($this->table, static::schemaDefinition());
return TRUE;
}
$this->connection->schema()->createTable($this->table, static::schemaDefinition());
}
catch (DatabaseException $e) {
// If another process has already created the config table, attempting to
// recreate it will throw an exception. In this case just catch the
// exception and do nothing.
return TRUE;
}
catch (\Exception $e) {
throw new PluginException($e->getMessage(), 0, $e);
return FALSE;
}
return FALSE;
return TRUE;
}
/**
......
......@@ -266,19 +266,18 @@ public function garbageCollection() {
protected function ensureTableExists() {
try {
$database_schema = $this->connection->schema();
if (!$database_schema->tableExists(static::TABLE_NAME)) {
$schema_definition = $this->schemaDefinition();
$database_schema->createTable(static::TABLE_NAME, $schema_definition);
return TRUE;
}
$schema_definition = $this->schemaDefinition();
$database_schema->createTable(static::TABLE_NAME, $schema_definition);
}
// If another process has already created the queue table, attempting to
// recreate it will throw an exception. In this case just catch the
// exception and do nothing.
catch (DatabaseException $e) {
return TRUE;
}
return FALSE;
catch (\Exception $e) {
return FALSE;
}
return TRUE;
}
/**
......
......@@ -83,6 +83,10 @@ public function addRoutes(RouteCollection $routes) {
*
* @param array $options
* An array of options.
*
* @throws \Exception
* Thrown if the table could not be created or the database connection
* failed.
*/
public function dump(array $options = []) {
// Convert all of the routes into database records.
......@@ -100,7 +104,9 @@ public function dump(array $options = []) {
->execute();
}
catch (\Exception $e) {
$this->ensureTableExists();
if (!$this->ensureTableExists()) {
throw $e;
}
}
// Split the routes into chunks to avoid big INSERT queries.
......@@ -174,18 +180,17 @@ public function getRoutes() {
*/
protected function ensureTableExists() {
try {
if (!$this->connection->schema()->tableExists($this->tableName)) {
$this->connection->schema()->createTable($this->tableName, $this->schemaDefinition());
return TRUE;
}
$this->connection->schema()->createTable($this->tableName, $this->schemaDefinition());
}
catch (DatabaseException $e) {
// If another process has already created the config table, attempting to
// recreate it will throw an exception. In this case just catch the
// exception and do nothing.
return TRUE;
}
return FALSE;
catch (\Exception $e) {
return FALSE;
}
return TRUE;
}
/**
......
......@@ -2,7 +2,9 @@
namespace Drupal\KernelTests\Core\KeyValueStore;
use Drupal\Core\Database\Database;
use Drupal\Core\DependencyInjection\ContainerBuilder;
use Drupal\Core\KeyValueStore\KeyValueDatabaseFactory;
use Drupal\Core\KeyValueStore\KeyValueFactory;
/**
......@@ -12,6 +14,14 @@
*/
class DatabaseStorageTest extends StorageTestBase {
/**
* {@inheritdoc}
*/
protected function setUp(): void {
parent::setUp();
$this->factory = 'keyvalue.database';
}
/**
* {@inheritdoc}
*/
......@@ -22,4 +32,58 @@ public function register(ContainerBuilder $container) {
$container->setParameter('factory.keyvalue', $parameter);
}
/**
* Tests asynchronous table creation.
*/
public function testConcurrent() {
if (!function_exists('pcntl_fork')) {
$this->markTestSkipped('Requires the pcntl_fork() function');
}
$functions = [];
for ($i = 1; $i <= 10; $i++) {
$functions[] = 'set';
$functions[] = 'getAll';
}
$default_connection = Database::getConnectionInfo();
Database::removeConnection('default');
$time_to_start = microtime(TRUE) + 0.1;
// This loop creates a new fork to set or get key values keys.
foreach ($functions as $i => $function) {
$pid = pcntl_fork();
if ($pid == -1) {
$this->fail("Error forking");
}
elseif ($pid == 0) {
Database::addConnectionInfo('default' . $i, 'default', $default_connection['default']);
Database::setActiveConnection('default' . $i);
// Create a new factory using the new connection to avoid problems with
// forks closing the database connections.
$factory = new KeyValueDatabaseFactory($this->container->get('serialization.phpserialize'), Database::getConnection());
$store = $factory->get('test');
// Sleep so that all the forks start at the same time.
usleep(($time_to_start - microtime(TRUE)) * 1000000);
if ($function === 'getAll') {
$this->assertIsArray($store->getAll());
}
else {
$this->assertNull($store->set('foo' . $i, 'bar'));
}
exit();
}
}
// This while loop holds the parent process until all the child threads
// are complete - at which point the script continues to execute.
while (pcntl_waitpid(0, $status) != -1);
Database::addConnectionInfo('default', 'default', $default_connection['default']);
$factory = new KeyValueDatabaseFactory($this->container->get('serialization.phpserialize'), Database::getConnection());
$store = $factory->get('test');
$this->assertCount(10, $store->getAll());
}
}
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