Problem/Motivation
We create the key_value
table on the installation of the system
module.
Let's make that lazy like for example the cache system.
Proposed resolution
The patch from #67 was committed (5571193) and then reverted (eaaea5d for two reasons:
- This breaks saving config entities without UUIDs. See #73
- There are test failures with PHP7 and some database drivers. See #74
The first problem is addressed in #77 and more recent patches. Some patches (up to #94) addressed the second problem, but not in a satisfactory way, so that problem was split off into #2847855: Incorrect statement class caused by race condition when closing and opening the connection and this issue was postponed on that, #107.
In order to help reviews and future work, when this was no longer postponed, a new patch was made based on #77 which included typo fixes and documentation changes from #94. When that was complete the issue was postponed in #113 until the follow 2 questions were answered.
1. What really confuses me is why this starts to fail as part of this patch. I mean it used to work fine without the patch, I guess. See #80
The answer. The Connection class has the wrong statementClass, leading to an exception. The patches on this issue change the exception handling in KeyValueStore\DatabaseStorage::getMultiple(). Previously, all exceptions were ignored. Earlier patches on this issue propagated the exception unless it seemed to be caused by a missing (to-be-lazy-loaded) database table. Different implementations of tableExists() give different results, which explains why the test fails only for PostgresQL. See #116
2. It's good that the workaround works, but do we understand what the race condition is? And is it reproducible enough to post a PHP/PDO bug report for? See #96
The answer. TBD
This was unblocked in #116, where all changes to getMultiple() were removed. That resulted in passing tests for MySQL, PostgreSQL and SQLite. This was followed by several rounds of code cleanup.
When this was picked up again it was time to reroll against 9.0.x which removed uuid changes since they were part of a BC layer. The result is a cleanup patch which passes tests in #141.
Then deprecation was added because now that the tables are lazy loaded contrib test could fails.
Remaining tasks
Review the patch
Answer this, if not already answered.
It's good that the workaround works, but do we understand what the race condition is? And is it reproducible enough to post a PHP/PDO bug report for? See #96
The answer. TBD
Make a followup for, if not already made. #116, 3rd item. Different implementations of tableExists() give different results, which explains why the test fails only for PostgresQL.
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#159 | 2664322-159.patch | 48.31 KB | daffie |
#159 | interdiff-2664322-158-159.txt | 1.36 KB | daffie |
Comments
Comment #2
dawehnerComment #4
dawehnerThis should fix a good bunch of the failures.
Comment #6
dawehnerThere we go.
I would have not expected that we catch exceptions silently.
Comment #8
dawehnerLet's fix the remaining failures.
Comment #9
dawehnerRerolled
Comment #10
dawehnerRebased
Comment #12
dawehnerNow this patch also patches the expireable key value store.
Comment #14
almaudoh CreditAttribution: almaudoh commentedThese look unrelated...
Comment #15
dawehnerThere we go.
Comment #17
benjifisherI see a few oddities. For example,
I guess the idea of setting
$uuid
is that you planned to use the value later instead of making another call to theuuid()
method.This pattern is used twice:
I think you can eliminate the
$try_again
variable completely by moving$this->set($key, $value);
inside thecatch
block. IMO this makes the control flow easier to follow, which is a good thing to worry about when a function calls itself from acatch
block!I will look at it some more tomorrow.
Comment #18
benjifisherI forgot to say: I was looking at
KeyValueStore/DatabaseStorage.php
in my previous comment. Here is another little complaint:You can combine those into one line and still stay under 80 characters.
Funny indentation:
Comparing
schemaDefinition()
in KeyValueStore/DatabaseStorage.php and KeyValueStore/DatabaseStorageExpirable.php, I seein one and
in the other. I would rewrite
DatabaseStorageExpirable/schemaDefinition()
asThis would fix the inconsistencies that are already there, and it would prevent future inconsistencies.
This looks wrong: don't you want
use Drupal\Core\KeyValueStore\DatabaseStorage;
?Do we really need to keep multi-line array syntax here?
Comment #19
dawehnerGreat comments!
Comment #20
benjifisherThe attached patch does a few things:
use
directive in UrlAliasFixtures.php as I suggested in #18.The removed files seem to cause one of the test failures for the patch in #14. I am getting additional failures when I run that test locally, so I am curious to see what the testbot does now.
@dawehner points out that some of my other suggestions affect code already in core, so I will open a couple of follow-up issues to suggest those changes.
Comment #22
benjifisherIt already needs a re-roll!
Comment #24
benjifisherFailing two tests. I think one is easy to fix. I am attaching an interdiff with my previous patch.
Comment #26
benjifisherI was right: we are down to one failing test.
After adding some extra lines to the failing test, I get the following message when running it locally:
This is the extra dblog message that makes the test fail. The offending line in
system.module
isSo I think the problem is that
KeyValueStore\KeyValueDatabaseExpirableFactory::garbageCollection()
is trying to delete entries from a table that does not exist.That is as far as I plan to go with this.
Comment #27
dawehner@benjifisher
Right, so yeah we need this exception wrapping here as well.
Comment #28
benjifisher@dawehner: Yes, and I decided to give it a try myself.
Since the
DatabaseStorage::catchException()
method is neither public nor static, I copied it (replacing$this->table
with'key_value_expire'
) toKeyValueDatabaseExpirableFactory
and used it in thecatch
block. The DbLog test now passes locally; let's see if the testbot gets the same result.While working on this, I noticed that some of the code comments had been copied from the
BatchStorage
class. Since that is part of this issue, I updated the comments. This is a signal that we should formalize this pattern for lazy table creation.Another thing I noticed is that the table name
'key_value_expire'
appears in two places: the constructor forDatabaseStorageExpirable
and inKeyValueDatabaseExpirableFactory::garbageCollection()
. (After this patch, it appears in another method in that class.) I think that string should be used in just the factory class, passed to theDatabaseStorageExpirable
constructor. In fact, the only reason that constructor exists seems to be to override the default value of$table
from theDatabaseStorage
constructor, so if we pass'key_value_expire'
, then we can eliminate the child constructor. Unless someone tells me I am missing something, I will open an issue to fix this.Comment #29
almaudoh CreditAttribution: almaudoh commentedThere's a really old issue where that was being done - #2371709: Move the on-demand-table creation into the database API
Comment #30
benjifisher@almaudoh: Thanks for the reference.
I am moving this back to NW even though it is passing tests. I searched for
key_value
andkey_value_expire
in the code base. I found a few places, mostly in tests, where there are direct database queries. For example, (drupal-8.views_entity_reference_plugins-2429191.php):Probably these should be instantiating the appropriate DatabaseStorage class instead. Maybe in a follow-up issue? Or maybe they really should talk to the database directly?
In
MigrationCreationTrait::getLegacyDrupalVersion()
there is something similar, and I think this one should stay as is.There are several tests that do something like this in their
setUp()
methods (DbLogFormInjectionTest.php):I think the right thing to do is remove
'key_value_expire'
from the array, and I think it should be done as part of this issue. But maybe some or all of these should be creating the tables indirectly by instantiating the appropriate class.Also, there are several gzipped files that directly reference these database tables:
core/modules/system/tests/fixtures/update/*.php.gz
.Comment #31
dawehner@benjifisher
Great feedback!
I totally agree. Good point! I went through all instances of it.
Those places are fine. These are one of implementations for update path tests.
Let's not touch that here either ... its wrapped in a
$connection->schema()->tableExists('key_value')
call.Comment #33
benjifisherThe patch from #31 ran afoul of #2687897: Convert system module's kernel tests to NG, which moved the tests to a new location. Don't take my word for it, ask git:
That makes it easy to re-roll:
The resulting patch (attached) applies to the current head of 8.2.x or 8.3.x. I do not think an interdiff is appropriate in this case.
I checked the interdiff attached to #31. It does affect only tests, and it does remove
'key_value_expire'
from the array of modules to be enabled for the test, as advertised. In appropriate places, thesetUp()
method does not do anything else, so it is removed completely.I again searched the code base for
key_value
andkey_value_expire
. I found one new test that installs the module in itssetUp()
method. I removed that: a second patch and an interdiff are attached. Let's see what the testbot has to say!Comment #34
almaudoh CreditAttribution: almaudoh commentedThe 33-a.patch is so much bigger...
Comment #35
benjifisher@almaudoh: Thanks for catching that. After making sure that my patch applied against 8.3.x, I made a diff against 8.2.x, so I accidentally included the differences between 8.2.x and 8.3.x in that patch.
I am hiding the bad patch and uploading a replacement. The interdiff on #33 shows the differences between 2664322-33.patch and 2664322-35.patch.
Comment #37
dawehnerThis patch looks almost ready to go!
These are out of scope changes and are also a bit wrong :) (See https://www.drupal.org/node/1354 )
Do you think we could create its own follow up for just this?
These changes also seem to be unrelated ...
Comment #38
dawehnerComment #39
almaudoh CreditAttribution: almaudoh commentedJust a reroll
Comment #40
almaudoh CreditAttribution: almaudoh commentedReverted unrelated changes in #37.1 and #37.3
Also raised #2787737: Finalize return value for KeyValueStoreInterface::getMultiple() for #37.2
Comment #41
almaudoh CreditAttribution: almaudoh commentedReverted some leftovers of unrelated changes in File.php. Also added the #2787737: Finalize return value for KeyValueStoreInterface::getMultiple() issue link in the todo.
Comment #42
dawehnerNice, thank you for addressing those points!
Comment #43
alexpottThis change is looking pretty good. One thing that makes me hesitate is what happened when we made the change for url_alias. However I don't think that is a concern here because the state table will definitely exist by the end of the install. But still we should check that once this patch is applied requests to regular routes like node/1. Since if we're getting exceptions here then we'll slow down the critical path.
Comment #44
dawehnerThe only key_value entry we use more or less regularily is the
system.cron_last
key. Even in case we would not have the table installed after the installation, the first cron would trigger it, and we no longer have the exception.I just checked a checkout of this patch and a new install, and we both had a
key_value
as well askey_value_expire
in there. Forkey_value
we have a lot ofconfig.entity.key_store.action
entries. Forkey_value_expire
there is update module related stuff in by default.I also attached a log of key_value::get(Multiple) calls, which happened after a
drush cr;
Given that I'm quite convinced that there is no risk, unless with the
url_alias
example.Comment #45
larowlanAre these unintended changes or are they because we've got tests for key value entity storage?
Other than that looks good to me
Comment #46
dawehnerThis change was not related with the key-value entity storage, as that one is for content entities. This change fixes some of the test failures in #2664322-6: key_value table is only used by a core service but it depends on system install, but yeah I don't remember 100% the exact reason why it was needed. Fact is, the interface allows you to return NULL, so better not rely on the value :)
Comment #47
larowlanworks for me
Comment #48
catchThat change looked odd to me as well, uploading a patch without the hunk (essentially patch -p1 -R #45) to see what fails exactly.
Comment #50
dawehnerI debugged for a while, why actually those failures occur. The problem is that the exported yml files don't have a uuid in them.
The problem is then that we setup the {config} table with raw data, so the uuid is never generated. Given that the fix is actually not needed and can be replaced by touching a couple of existing yml files and related code.
Comment #52
dawehnerThere was one uuid missing.
Comment #54
dawehnerJust a quick reroll.
Comment #56
dawehnerForgot to use the
--binary
option.Comment #57
alexpottNice - I think we should open an issue to add a SchemaDefinitionInterface so we can discover all the services which have schema for reporting and schema comparison.
Comment #58
catchWhy is this change from protected to public in this patch? Looks unrelated.
Comment #59
dawehnerI'm personally not convinced :) Schema definitions are kinda separated out in different areas already with the entity system. Having those few tables being part of some system, but some aren't is hard. Depending on the implementation #2371709: Move the on-demand-table creation into the database API though might give us that.
Comment #61
dawehnerJust a random failure
Comment #63
dawehnerRandom failure
Comment #64
catchStill looking for an answer to #58.
Comment #65
dawehnerWell, let's try it out. I think its mostly about being consistent, as most other instances have that (KV needs it for some test
\Drupal\system\Tests\Path\UrlAliasFixtures::tableDefinition
), but sure, let's see whether we actually need it.Comment #67
almaudoh CreditAttribution: almaudoh commentedGuess you forgot the
--binary
option again :) Same patch as in #65Comment #68
almaudoh CreditAttribution: almaudoh commentedSince #58 has been addressed, back to RTBC.
Comment #69
catchCommitted 5571193 and pushed to 8.3.x. Thanks!
Comment #71
dawehnerThanks a ton!
Comment #73
alexpottIt turns out that this broke saving config entities without UUIDs. For example:
From #2808087: Improve captcha_install(), correctly create dynamic captcha points, move static captcha points to default config
The problem is that when \Drupal\Core\Config\Entity\ConfigEntityBase::preSave() fires it assumes
$this->uuid()
returns a UUID but creating and saving config entities as above by passes the uuid assignment in entity storage create. We need to work out what to do here.Comment #74
xjmThis also introduced a test failure on PHP7/Postgres so we should ensure any subsequent patch is tested against all environments.
Comment #75
dawehnerI think the right fix for this issue would be to return an empty array, if needed.
Comment #77
dawehnerThere we go
Comment #78
mradcliffeFor some reason the connection class is trying to use PDOStatement instead of the statement class. This throws the
HY00: General error: user-supplied statement does not accept constructor arguments
error. I am not sure why the driver or our DBAL is switching back to PDOStatement.All three queries being run, the watchdog insert query, the key_value store select query, and the table exists query are being run with PDOStatement. Everything else is using the defined statement class.
In C: https://github.com/php/php-src/blob/master/ext/pdo/pdo_dbh.c#L411
Comment #79
mradcliffeConnectionFailureTest calls
Database::closeConnection();
, which resets ATTR_STATEMENT_CLASS to PDOStatement. And then DBlog's custom connection thing might not be going through the right pathways to set it again.I think the bug lies in the general Database/Connection classes rather than Watchdog module because Watchdog module shouldn't need to know about the intricacies of setting up the connection correctly to use Statement. It should be happening correctly in the constructor for a driver class, but it's not?
Comment #80
dawehner@mradcliffe
What really confuses me is why this starts to fail as part of this patch. I mean it used to work fine without the patch, I guess.
Comment #81
mradcliffeYes, quite confusing.
This patch is probably not the correct approach, but to show that it's the problem.
Maybe a race condition in PDO object destruction and now since that test is doing more queries it uses an existing PDO object? Checking the statement class inside Dblog shows that it's Statement, but then checking inside prepareQuery shows that it's PDOStatement. If it is a race condition it would theoretically affect any driver using statement class.
Comment #83
mradcliffeI had some weird changes locally and did not apply the patch in #77 correctly. Hiding my patch and interdiff.
The SQLite fail is valid as that driver doesn't override statementClass, which might be better if it did. I wanted to apply this generally because I have a sneaking suspicion that it could apply across the board in some rare cases. Or that we are hiding the error somewhere in exception handling.
I will apply the patch on a clean checkout and redo.
Comment #84
mradcliffeTry with a clean patch.
Also an experimental patch that if it fails deserves its own issue. Edit: I expect sqlite to behave oddly, but we'll see if mysql and pgsql display an actual issue.
Comment #85
dawehner@mradcliffe
So it seems to be that this is its own issue actually. Do you think we should open up its own issue and postpone this one, on the new one?
Comment #86
daffie CreditAttribution: daffie commentedIf I look at your statement-class-test.patch I see that you are trying to get the Statement class with the getAttribute() method. Looking at the PHP documentation for this, there are a number of PDO constants that can be used. And \PDO::ATTR_STATEMENT_CLASS is not one of them. So the construction that you are using is not supported by PHP. Maybe unofficial but not according to their manual (http://php.net/manual/en/pdo.getattribute.php). The constant \PDO::ATTR_STATEMENT_CLASS can only be used with the method setAttribute() to: "Set user-supplied statement class derived from PDOStatement. Cannot be used with persistent PDO instances. Requires array(string classname, array(mixed constructor_args)).". This is also according to the PHP manual. AFAIK a PDOStatement class is the result that you get after you run a PDO query.
Comment #87
mradcliffe@dawehner, I retested #77 for php7 sqlite and got the same error as in #84 for php7 sqlite. I queued up #84 (pgsql and sqlite) and #77 (sqlite only) tests again in case these errors in #77 and #84 were random test failures.
Thanks, @daffie. It looks like it's a php docs bug. It's possible that it only applies on certain drivers: "Note that some database/driver combinations may not support all of the database connection attributes." Connection::__construct sets the attribute to use Statement class from a PDO Query, not PDOStatement. Drupal switches back to PDOStatement during Connection::destroy.
Edit: #77 and #84 still fail on SQLite (php7). Probably the same for PostgreSQL.
Comment #88
mradcliffePHP 7 isn't passing at the moment due to a PHP bug: https://www.drupal.org/node/2813981.
So i guess #84 is working for 5.5 and 5.6 mysql, sqlite, and pgsql. That's the patch that only adds a check in pgsql to ensure that Statement is the statement class instead of PDOStatement.
Comment #89
dawehner@mradcliffe
So this is actually not the fault of this patch. Thank you for figuring that out!
Comment #90
almaudoh CreditAttribution: almaudoh commentedRetesting on php7. So how do we want to proceed? Commit #84 as is? Or take those new parts to a new issue?
Comment #91
dawehnerWe certainly need #84, because unlike, #77 this doesn't fail on PGSQL.
Comment #92
almaudoh CreditAttribution: almaudoh commentedWe should use
!==
instead of<>
. Didn't realise that still works in php :)Comment #93
dawehnerOh yeah that's right, @mradcliffe certainlys spends too much time in SQL land :)
Comment #94
almaudoh CreditAttribution: almaudoh commentedJust the nitpicks. I guess I can RTBC this as I haven't made any changes since the revert.
Comment #95
dawehnerOh yeah that should be fine. Thank you @almaudoh!
Comment #96
catchIt's good that the workaround works, but do we understand what the race condition is? And is it reproducible enough to post a PHP/PDO bug report for?
Comment #98
dawehner@mradcliffe
Can you comment on that?
Comment #99
mradcliffe@catch, I don't have any specific data, but I think this is an application-level condition, not a PHP/PDO bug.
$connection is held open if there are any references to the object even when the destroy command is issued on it. I'm not sure where that could be coming from in the code base. A debug_zval_dump would help to prove that this is the case, but that doesn't help to figure out where it could be.
I added debug_zval_dump into ::destroy to see what it usually comes up with. During the test run of ConnectionFailureTest it is set to 2 reference counts in that method. Most likely those references are destroyed quickly in most use cases of database, but maybe not when Dblog is doing it?
Comment #100
dawehner@mradcliffe
So do you think the question from catch is addressed and we could go back to RTBC here?
Comment #101
mradcliffeComment #102
catchI don't think we should add drivercode to cover a bug that's only reproducible in a test that's specifically messing with the database connection, or do it in this issue. Can we split this discussion out to a new issue and postpone this one?
Comment #106
benjifisherThe patch from #67 was committed (5571193) and then reverted (eaaea5d for two reasons:
The first problem is addressed in #77, and later patches (up to #94) addressed the second problem.
There have been several suggestions (at least #85 and #102) to handle the second problem in a separate issue and to postpone this issue. I am updating the "Remaining Tasks" section of the issue summary.
Comment #107
benjifisherI just created #2847855: Incorrect statement class caused by race condition when closing and opening the connection, added it as a related issue, and marked this issue as Postponed. Is there any way (other than this comment) to indicate that the related issue and the Postponed status are connected?
I did my best to describe the problem, but do not be shy about updating #2847855 if you have a better handle on it.
Comment #108
benjifisherIn order to make reviews easier, I am attaching two interdiffs:
In fact, interdiff-67-77.txt is essentially the same as the interdiff attached to #74. (We used different methods to generate the interdiffs.) I think the only difference between #74 and #77 is that the second one was correctly generated with
git diff --binary
and the first without the--binary
option.The patch in #94 corrects a few typos in documentation comments. The code change that it introduces should be made in the new issue #2847855: Incorrect statement class caused by race condition when closing and opening the connection.
I think that further work on this issue should start with the patch in #77 (along with fixing the typos as in #94), which may need to be re-rolled.
Comment #109
benjifisherHere is an attempt to re-roll the patch from #77, with the typo corrections from #94. Because of the changes in HEAD, I do not see an easy way to generate an interdiff. :-(
There was a merge conflict in
EntityAutocompleteElementFormTest::setUp()
:That snippet now looks like
so it is easy to see why the patch did not apply. Also easy to apply it manually.
The other conflict was in
ConfigEntityStorageTest::testSaveMismatch()
. This is more complicated, since that test was rewritten in #2824165: Remove brittleness from ConfigEntityStorageTest. But maybe the only reason that change was in the patch was to fix failing tests, in which case we may not have to deal with it in this issue.I am curious to see which tests fail this time around. ;) So I am temporarily moving from Postponed to NR.
Comment #111
benjifisherHere is a new patch that, instead of giving up on on merge conflicts in
ConfigEntityStorageTest
, leaves that test completely alone. I expect that this will fix the test failures with MySQL.I also removed the change to the
dblog
tests. If that fixes the test failures with Postgres, then we will have to re-think the idea that those are not really related to this issue.Comment #112
dawehner@benjifisher
Thank you for continuously working on it.
Comment #113
benjifisher@dawehner, it has been interesting!
I have attached an interdiff for the changes between #109 and #111. I already described the two small changes in #111.
Now that the testbot has spoken, I am moving this issue back to Postponed. I think the next step is to answer the question (#80, #96) why this patch triggers the PostgresQL error. In other words, we need to work on #2847855: Incorrect statement class caused by race condition when closing and opening the connection.
Comment #114
daffie CreditAttribution: daffie commentedChanging the implementation of the PostgreSQL version of the method Schema::tableExists to the same as the MySQL version fixes the problem in #2847855: Incorrect statement class caused by race condition when closing and opening the connection. Lets see if it does the same for this issue.
Comment #115
benjifisherLet's give the testbot a workout. I expect PostgreSQL to work with various versions of PHP, but I do not know what to expect with SQLite.
See my comments in #111 and the interdiff in #114: only half of the changes I added in #111 should be kept.
As I suggested on #2847855: Incorrect statement class caused by race condition when closing and opening the connection I think that fixing
tableExists()
should be done in a separate issue, but I am really excited to see those test passes on this issue!I would also like to address some of the comment from #17 before this issue is closed.
Comment #116
benjifisherI did some debugging (reported on one of the related issues) and I now think we can fix this issue. We discovered some new issues while working on this, but this issue is no longer blocked by any of them. See my "fail" patch at #2847855-10: Incorrect statement class caused by race condition when closing and opening the connection, which reproduces the PostgreSQL test failure that has been blocking this issue.
This is why the test fails (answering @dawehner's question in #80):
Connection
class has the wrongstatementClass
, leading to an exception.KeyValueStore\DatabaseStorage::getMultiple()
. Previously, all exceptions were ignored. Earlier patches on this issue propagated the exception unless it seemed to be caused by a missing (to-be-lazy-loaded) database table.tableExists()
give different results, which explains why the test fails only for PostgresQL.I created #2847855: Incorrect statement class caused by race condition when closing and opening the connection to follow up on #1.
I just created #2850292: Improve exception handling indirectly in KeyValueStore\DatabaseStorage::getMultiple() method to follow up on #2.
@daffie has posted patches here and on #2847855: Incorrect statement class caused by race condition when closing and opening the connection related to #3. I suggest dealing with that problem in another issue: perhaps create a new one.
The attached patch reverts all changes to
getMultiple()
. Locally, this fixes theConnectionFailureTest
failure with PostgresQL. Like the patch in #111, it reverts all changes toConfigEntityStorageTest
because #2824165: Remove brittleness from ConfigEntityStorageTest already did a better job of fixing that test. Because of all that, I have attached an interdiff comparing to the re-roll in #109, not to one of the more recent patches.I also plan to do some code cleanup once I have a patch that passes testing.
Comment #117
benjifisherHere is a patch simplifying some of the try/catch blocks as I suggested in my review in #17. Unless I am missing something, this patch is functionally equivalent to the one in #116.
I have a little more cleanup to do, but this is tricky enough that I want to provide an interdiff that is not cluttered up with any other changes.
Comment #118
benjifisherOuch, I made two mistakes. One is the mistake that @almaudoh kept fixing on earlier patches: I forgot to use the
--binary
option when generating the patches in #116 and #117. Then I accidentally committed changes tocomposer.json
andcomposer.lock
in #117.The attached patch fixes those problems and gives an interdiff against what #117 was supposed to be.
As the interdiff shows, this only does some cleanup: avoid mixing
aray(...)
and[...]
syntax in the same file, in order to follow Drupal coding standards.This is the last cleanup I have in mind, although I still plan to take another look at the patch.
Comment #119
benjifisherHiding patches (but not interdiffs) as explained in #118.
Comment #121
benjifisherIt does not help to introduce a syntax error. :-(
Again, I attach a patch and an interdiff against what I meant to upload in #117.
I need some sleep. That should give the testbot time to finish.
Comment #122
benjifisherSorry, I keep tweaking this.
I do not like the pattern
It is bad enough that we are using exceptions for control flow. (Where did I see a policy against that?) If the
if(...)
does the wrong thing, then we could be caught in an infinite loop. The attached patch replaces this pattern (twice each inDatabaseStorage
andDatabaseStorageExpirable
) with the pattern that I see inBatch\BatchStorage
andCache\DatabaseBackend
:Actually, my pattern is a little different from what I see in those two classes. As in #117, I simplify the control flow, removing the
$try_again
flag. With the simpler control flow, I think we do not need so many comments on the control flow, so I have removed several comment lines.Comment #123
dawehnerWell, for me its not that much of a difference at the end. But yeah, we have an infinite loop protection!
@benjifisher
Do you think we need to wait until pgsql is fixed?
Comment #124
benjifisher@dawehner: No, we do not have to wait for fixes to pgsql. See my comment in #116. And see all the green from the testbot. It needs a reroll, probably because of the short array syntax.
Comment #125
benjifisherThat was a nasty reroll. I hope the testbot approves! I think the only conflicts were #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase (as expected) and #2854529: Fix Drupal.Scope.MethodScope - all methods should have scopes.
Comment #126
tstoecklerThe testbot now complains about this not using the new array syntax.
Comment #127
benjifisherThanks for checking that! I was so worried about fixing the merge conflicts that I neglected to check the files where the merge was clean.
Here is a new version of the patch, and an interdiff.
Comment #129
jibranPatch still applies do we have any feedback pending here?
Comment #131
dawehnerI think adding uuids in all these places isn't the right solution either, this just hides the problem.
Comment #132
BerdirAlso:
>>> \Drupal\Component\Uuid\Uuid::isValid('8C0C65A8-7099-4E90-9F6C-C5EE7BD3445D');
=> false
UUID's need to be lowercase.
Comment #135
benjifisherFrom #73:
From #74:
I think we fixed the problem from #74, but we still need to fix the problem from #73. If I were sure of myself, then I would set this back to NW. Maybe the right thing to do is to add a failing test, and then the testbot will set the issue back to NW.
I see some recent activity on #2721271: Simplify exception handling for lazy-load pattern. If that issue gets in, then this patch will need a reroll.
Comment #137
daffie CreditAttribution: daffie commentedShould we also postpone this issue on #2850292: Improve exception handling indirectly in KeyValueStore\DatabaseStorage::getMultiple() method?
Comment #138
daffie CreditAttribution: daffie commentedRerolled the patch from comment #127.
Removed all the UUID stuff, because it was all in BC layers that where removed in Drupal 9.0. Therefor I have set this issue for 9.1
Removed from a couple of places in tests:
$this->installSchema('system', ['key_value_expire']);
.The combined patch has the patches from #2847855: Incorrect statement class caused by race condition when closing and opening the connection and #2850292: Improve exception handling indirectly in KeyValueStore\DatabaseStorage::getMultiple() method.
The 2 problems named by @benjifisher in comment #135 are fixed (PHP7 on PostgreSQL) or are no longer relevant (config entities without UUIDs).
Comment #140
daffie CreditAttribution: daffie commentedRerolled for 9.1.
Comment #141
daffie CreditAttribution: daffie commentedThe patch without any stuff from #2850292: Improve exception handling indirectly in KeyValueStore\DatabaseStorage::getMultiple() method and #2847855: Incorrect statement class caused by race condition when closing and opening the connection as requested by @alexpott.
Comment #142
alexpottSo there are tonnes of these changes. Which makes it very likely this change will break tonnes of contrib and custom kernel tests. We need to handle this in a nicer way.
In \Drupal\KernelTests\KernelTestBase::installSchema we should trigger a silenced deprecation for removal in D10 that tells people that key_value and key_value_expire are no longer provided by the System module and are created on demand. That gives people most of D9 to upgrade their tests.
Comment #143
andypost++ to 142 because it affects related
Comment #144
daffie CreditAttribution: daffie commentedCreated a CR and updated the patch for comment #142.
Comment #145
catchIt'd be nice if there was a way for contrib to be able to do this too - might be worth a follow-up? I think the hard-coding is fine for this patch though, this is a very low-level API and it would only help test authors writing kernel tests with dependencies on schema from contrib modules they don't control.
Comment #146
benjifisherLooking at the snippet in #145, I think it should be therefore instead of therefor (twice).
BTW, I wish I had time to contribute more to this issue recently. A lot of people put a lot of effort into it (including me, a few years ago). I am happy to see that we are getting close to getting some benefit from that effort. Thanks for moving the issue forward!
Comment #147
daffie CreditAttribution: daffie commentedFor #145: Created the followup #3143448: Allow contrib modules to overrule the working of KernelTestBase::installSchema().
For #146: Fixed. Why do easy when you can do complicated. Sorry, I am not a native English speaker.
Comment #148
daffie CreditAttribution: daffie commentedThis issue is blocking #3129534: Automatically enable the module that is providing the current database driver and #3129043: Move core database drivers to modules of their own.
Comment #149
daffie CreditAttribution: daffie commentedComment #150
quietone CreditAttribution: quietone commentedHad a go at updating the IS, the proposed resolution and remaining tasks.
Comment #151
quietone CreditAttribution: quietone commentedFix formatting in IS
Comment #152
quietone CreditAttribution: quietone commentedAnother format fix to the IS.
Comment #153
quietone CreditAttribution: quietone commentedPatch no longer applies to 9.1.x and there is a coding standard error.
Comment #154
daffie CreditAttribution: daffie commentedWorking on a reroll.
Comment #155
daffie CreditAttribution: daffie commentedComment #156
daffie CreditAttribution: daffie commentedRerolled the patch and fixed the coding standard errors.
Comment #157
quietone CreditAttribution: quietone commentedI've reviewed the patch paying particular attention to the comments and doc blocs and only found a few things. :-)
Needs an @return
Why is is better to do this instead of checking if the tables exists first?
I am having difficulty with this text. Can we simplify it a bit? And expand the 'that's fine' to an explanation. Why is it fine to ignore the exception?
Same applies to catchException in KeyValueStore/KeyValueDatabaseExpirableFactory.php.
Nit, out of scope change.
Removed the item and therefor/therefore from the IS because that is complete. It was my mistake that added it the IS.
Comment #158
daffie CreditAttribution: daffie commented@quietone: Thank you for your review.
For #157.1: Fixed.
For #157.2: For > 99% of the time that this method is called the table will not exists. Every time we check if a table exists it will result in a query to the database. Which we like to do a few as possible. Also other database storage classes where the table is lazy loaded, have the same method ensureTableExists(). They all work in the same way.
For #157.3: When a select, update or delete query fails, because the table does not exists, that is not a problem. The table is created by lazy loading. Therefore an exception must be caught and suppressed when it is the result of a non-existing table. I did not create the text and the current text is very much in line with the other methods in core with the same name. Also I am not a native English speaker, so if you have any suggestion how to improve the text then please say so.
For #157.4: Removed the change.
Comment #159
daffie CreditAttribution: daffie commentedAs discussed with @quietone on Slack, 2 docblock texts are updated.
@quietone: Thank you for your help!
Comment #160
quietone CreditAttribution: quietone commented@daffie, your welcome!
I reviewed the changes in response to #157 and they have all been addressed or fixed as needed. I think the patch looks good and is ready for RTBC but this is not an area I have enough knowledge in to set to RTBC.
Comment #161
catchThis looks great to me now, looked over older comments and can't believe it's 4 years since we had to revert the original patch.
Comment #162
daffie CreditAttribution: daffie commentedThank you @catch for the RTBC!
Comment #163
alexpottCommitted faabcf1 and pushed to 9.1.x. Thanks!
I tried to re-create the problem in #73 on 9.1.x. Firstly I can the captcha tests with this patch and they passed and then i ran that code. We successfully created a config entity with a NULL uuid - which is wrong but that's what we've told the system to do and it's not related to the this issue. The state query we make on config entity save does not appear to be broken by this change so that's great.
Comment #165
daffie CreditAttribution: daffie commentedThanks to everybody who worked on this issue!!!