Problem/Motivation
Here's an example from \Drupal\Core\Batch\BatchStorage::ensureTableExists()
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;
}
}
// 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;
}
The only way we can return FALSE is if the table exists! Which is completely wrong according to the docs and how things are supposed to work. We return false if $database_schema->tableExists(static::TABLE_NAME)
returns TRUE!
This causes problems if a table is being creating by an ajax request and there several hitting the server at the same time. We get races and this code then fails to protect us.
Steps to reproduce
Proposed resolution
Change the method to do this:
protected function ensureTableExists() {
try {
$database_schema = $this->connection->schema();
$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) {
}
catch (\Exception $e) {
return FALSE;
}
return TRUE;
}
Table existence is already checked in $database_schema->createTable()
and if that throws an exception... it is caught and handled. The caller is rethrowing the initial exception in \Drupal\Core\Batch\BatchStorage::create().
Remaining tasks
Fix:
- \Drupal\Core\Batch\BatchStorage::ensureTableExists()
- \Drupal\Core\Cache\DatabaseCacheTagsChecksum::ensureTableExists()
- \Drupal\Core\Flood\DatabaseBackend::ensureTableExists()
- \Drupal\Core\Lock\DatabaseLockBackend::ensureTableExists()
- \Drupal\Core\Queue\DatabaseQueue::ensureTableExists()
- \Drupal\Core\Config\DatabaseStorage::ensureTableExists()
- \Drupal\Core\KeyValueStore\DatabaseStorage::ensureTableExists()
- \Drupal\Core\Routing\MatcherDumper::ensureTableExists()
- \Drupal\Core\Menu\MenuTreeStorage::ensureTableExists()
User interface changes
API changes
The method Drupal\Core\Cache\DatabaseCacheTagsChecksum::catchException()
is deprecated.
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#23 | 3240601-23.patch | 17.67 KB | alexpott |
#23 | 20-23-interdiff.txt | 3.78 KB | alexpott |
#20 | 3240601-20.patch | 15.84 KB | alexpott |
#20 | 15-20-interdiff.txt | 502 bytes | alexpott |
Comments
Comment #3
alexpott@dbosen found this issue while working on broken javascript tests in Thunder.
Comment #4
alexpottHere's the fix. It might be nice to put this into a trait and somehow notify modules that have copy&pasted this code.
Comment #5
alexpottComment #6
catchVery nice find.
I think it should, although you could also argue that all the others should use SchemaObjectExistsException? But either way we should use one consistently.
Probably just in a follow-up.
There are two issues trying to do something like this already:
#2721271: Simplify exception handling for lazy-load pattern and #2371709: Move the on-demand-table creation into the database API.
Comment #7
alexpottThanks for the review @catch.
Re #6.1 I think we should go for DatabaseException. There's a chance of a race in \Drupal\Core\Database\Schema::createTable() between the table existence check and executing the query. If the query fails we won;t change the exception to a SchemaObjectExistsException - it'll be a normal DatabaseException.
Re #6.2 Well the changes we make here make it completely impossible whereas before it was only unlikely. So I think we need to address it here. I've moved the throws from ::ensureTableExists() up to the caller to maintain the API of ::ensureTableExists always returning a TRUE or FALSE regardless of errors and giving the caller a chance to emit the original exception and at the same time maintaining the current exception classes.
I think we should leave the trait / further API stuff to #2371709: Move the on-demand-table creation into the database API
Comment #8
daniel.bosenTo be able to use this in our tests where we initially ran into the problem, I backported it to 9.2.x
Comment #9
alexpottFixing issue credit... @dbosen is not @daniel.bosen
Comment #10
alexpottHere's a patch that combines this with #3240813: \Drupal\Core\KeyValueStore\DatabaseStorage::catchException() fails due to races for testing on 9.2.x with both patches... so Thunder can see if the combined patch fixes the issues.
Comment #11
daffie CreditAttribution: daffie commented@alexpott: Good find!
Comment #12
alexpott@daffie see earlier comments re trait / common way of doing it... i.e. #6 / #7.
Re testing - this will be exceptionally hard as this is caused by race. So it'll have to be forks and all sorts... I'll have a look to see what is possible. IN the meantime will remove the needs tests since I'm not sure that it is possible.
Re-upping the 9.3.x patch as that should be the last
Comment #13
alexpottOkay I can write a test that proves the problem on SQLite. For the other DB drivers the way we or PDO manages the connections makes it impossible :( I've tried quite a few different ways to keep the db connections out of the forks but it keeps on breaking.
Also since the test is on the key value database I need to include the fix fro the other race condition - ie. #3240813: \Drupal\Core\KeyValueStore\DatabaseStorage::catchException() fails due to races. I propose we merge the issues since have a test is more valuable than the pure application of issue scoping.
Comment #14
alexpottFWIW this test is not actually testing the database storage in HEAD :D ... it's testing the memory storage because it is missing this!
Comment #15
alexpottTurns out I can make the test database driver agnostic.
Comment #17
daffie CreditAttribution: daffie commented@alexpott: thank for for adding the Drupal\KernelTests\Core\KeyValueStore\DatabaseStorageTest::testConcurrent() test. It was not the test I was looking for, as I did not know that we could do what your test is testing. Really great!
I have added the testing that I was looking for and I have removed a couple of small changes that to me are not necessary. More testing for the other changes is still necessary to me.
Comment #18
daffie CreditAttribution: daffie commentedFixed the style code violations.
Comment #19
alexpott@daffie I don't think we should be adding the test that #17 adds. Using reflection to test protected method is not really the best. It ties our tests to internals that are not supposed to be exposed. It's not necessary. It would pass against EHAD and you've re-broken DatabaseStorageTest :( The changes to add the setUp are vital so it testing what it actually claims to.
Comment #20
alexpottRemoving the test from #17 and fixing the change to public for $connection - I did that to work out how to actually test this.
Comment #21
alexpottThe memory storage does not need to be tested twice - it has it's own test in \Drupal\KernelTests\Core\KeyValueStore\MemoryStorageTest
Comment #22
daffie CreditAttribution: daffie commentedWhy do we have this line here. As far as I can see it can be removed.
If we make this change the docblock also need an update.
For this change the methods docblock needs to be updated.
Can we do without this change? If not, then the methods docblock needs to be updated.
We are updating these methods. Can we then also update the docblock for the thrown exceptions.
The @todo needs to be fixed.
ensureTableExists()
in 9 classes. We have only addeed testing for one class. Is that enough or do we need to add testing to all 9 classes? Can we add a dataprovider to the added test?Comment #23
alexpottThanks for the review @daffie
Re #22
\Drupal\Core\Cache\DatabaseCacheTagsChecksum::catchException()
but I think we should only document @throws when exceptions are used for control and passing information or on public interfaces. But that's a discussion for another issue.::ensureTableExists()
is actually broken our test suite would fail very very hard. I think that it is great to have coverage of the fact that the current approach is broken when multiple threads call this code and that the approach fixes it but doing this for every instance is unnecessary. Especially as we have plans to make this code generic.Comment #24
daffie CreditAttribution: daffie commentedAll code changes look good to me.
All the 9 methods
ensureTableExists()
are the same. In #2371709: Move the on-demand-table creation into the database API will these methods be replaced by something else.The class for the database storage of keyvalue values has testing added for asynchronous table creation. According to @alexpott is this enough.
For the added deprecation of
Drupal\Core\Cache\DatabaseCacheTagsChecksum::catchException()
which is a protected method is a CR created. I have updated the IS for it.For me it is RTBC.
Comment #26
catchLooks good to me now. I think the single point of test coverage is OK given we're going to remove all the copypasta in the follow-up, and this code runs implicitly thousands of times in the test suite overall.
Committed e0b4b7e and pushed to 9.3.x. Thanks!
It would be possible to backport this to 9.2.x, but given the new deprecation leaving this in 9.3.x