garbageCollection followups to TempStore

At a minimum I think we should set a static to make sure it only happens once per request or something. It seemed like an excessive number of queries to me.

Also, I think it should be static; it already doesn't pay attention to what collection it's cleaning up.

#1805412: TempStore: Enable garbage collection on all temp store objects via hook_cron() or similar was the other issue.

Looking at a compromise that won't run N queries, at least. :)

Here's a start. I think a static boolean is a decent tradeoff for not running the same query N times.

garbageCollection() is still public. Making it protected would mean that we need to add another way to force expirations in order to test it, since the static gets set in the earlier tests. I don't want to use drupal_static() because it's not a cache and the world won't end if records don't get deleted until the next request. Dunno.

+++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php
@@ -123,11 +126,19 @@ public function deleteMultiple(array $keys) {
+    static $deleted = FALSE;

Statics in a method are the wrong choice 99.4% of the time, because they're still global and shared across all instances of the class. (That may not make a difference here, for now, but it will bite you later. We ran into that with the DB layer early on.) If you really want it shared between all instances, make it a static class property. If not (and you rarely do), object property.

That said, I'm unclear why this needs to happen on set() calls. Why not on __destruct(), so that it happens at the end of a request, after it's done blocking the request itself? On __destruct(), run call $this->garbageCollect().

You don't need to make it public to test, either. What you're testing is not that calling garbageCollect() does something. You're testing that on destruct, the database state changes. So you create a store object, set something on it that should expire, unset the object, then query the DB table to see if the record is there. That's unit-test safe.

@Crell and @chx and I talked through this a bit in IRC; the consensus is:

1. Add a (non-static) $needsGarbageCollection boolean object property.
2. Set that boolean during set or delete operations.
3. Check it on __destruct() and run garbageCollection() if it's set.

Like this.

Hm:

exception 'PDOException' with message 'SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupaltestbotmysql.simpletest679599key_value_expire' doesn't exist' in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Statement.php:58

It works fine locally. I've requeued again and I'll try it with run-tests.sh.

Runs fine locally with run-tests.sh. Hrm. Waiting on the bot again then. It went to #664 three times in a row, but now it's going to #958, so we'll know shortly if it's just a testbot problem.

Okay, I'm stumped. It works fine locally, and everything else is exactly the same as in the other k/v tests and the previous garbage collection tests, which work fine on the bot.

exception 'PDOException' with message 'SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupaltestbotmysql.simpletest848320key_value_expire' doesn't exist' in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Statement.php:58
Stack trace:
#0 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Statement.php(58): PDOStatement->execute(Array)
#1 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php(506): Drupal\Core\Database\Statement->execute(Array, Array)
#2 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Query/Delete.php(141): Drupal\Core\Database\Connection->query('DELETE FROM {ke...', Array, Array)
#3 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php(160): Drupal\Core\Database\Query\Delete->execute()
#4 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php(65): Drupal\Core\KeyValueStore\DatabaseStorageExpirable->garbageCollection()
#5 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/StorageTestBase.php(37): Drupal\Core\KeyValueStore\DatabaseStorageExpirable->__destruct()
#6 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/DatabaseStorageExpirableTest.php(31): Drupal\system\Tests\KeyValueStore\StorageTestBase->setUp()
#7 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php(613): Drupal\system\Tests\KeyValueStore\DatabaseStorageExpirableTest->setUp()
#8 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(381): Drupal\simpletest\TestBase->run()
#9 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(22): simpletest_script_run_one_test('263', 'Drupal\system\T...')
#10 {main}

Next exception 'Drupal\Core\Database\DatabaseExceptionWrapper' with message 'SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupaltestbotmysql.simpletest848320key_value_expire' doesn't exist: DELETE FROM {key_value_expire}
WHERE  (expire < :db_condition_placeholder_0) ; Array
(
    [:db_condition_placeholder_0] => 1349787811
)
' in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php:532
Stack trace:
#0 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Query/Delete.php(141): Drupal\Core\Database\Connection->query('DELETE FROM {ke...', Array, Array)
#1 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php(160): Drupal\Core\Database\Query\Delete->execute()
#2 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php(65): Drupal\Core\KeyValueStore\DatabaseStorageExpirable->garbageCollection()
#3 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/StorageTestBase.php(37): Drupal\Core\KeyValueStore\DatabaseStorageExpirable->__destruct()
#4 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/DatabaseStorageExpirableTest.php(31): Drupal\system\Tests\KeyValueStore\StorageTestBase->setUp()
#5 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php(613): Drupal\system\Tests\KeyValueStore\DatabaseStorageExpirableTest->setUp()
#6 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(381): Drupal\simpletest\TestBase->run()
#7 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(22): simpletest_script_run_one_test('263', 'Drupal\system\T...')
#8 {main}
Fatal error: Exception thrown without a stack frame in Unknown on line 0
FATAL Drupal\system\Tests\KeyValueStore\DatabaseStorageExpirableTest: test runner returned a non-zero error code (1).

Maybe?

Oh, no, it's choking in DatabaseStorageExpirableTest. Interesting. So...

So #11 shows the same error as above, and #12 fixes it. This has interesting implications -- do we need to worry in general about the storage going away before the object is destroyed, or is it safe to assume that issue will always be unique to the SimpleTest environment, and change the test so that the stores are no longer referenced on the test class so that they get destroyed before DatabaseStorageExpirableTest::tearDown()?

The fatal #12 is:

PHP Fatal error:  Exception thrown without a stack frame in Unknown on line 0
TempStore 19 passes, 0 fails, and 0 exceptions

Fatal error: Exception thrown without a stack frame in Unknown on line 0
FATAL Drupal\user\Tests\TempStoreDatabaseTest: test runner returned a non-zero error code (255).

I'll do some more debugging.

Okay, so this solves the issue for TempStoreDatabaseTest locally.

Still need to restructure the k/v tests and remove the hack for DatabaseStorageExpirableTest.

And fixing DatabaseStorageExpirableTest with the minimum possible alteration to StorageTestBase. Maybe referencing the child test in comments on the parent is a bit goofy, but the alternative would be to completely refactor the k/v tests.

#16: kvstore-1805444-16.patch queued for re-testing.

+++ b/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/DatabaseStorageExpirableTest.phpundefined
@@ -43,125 +43,118 @@ protected function tearDown() {
-    $this->store1->setWithExpire('foo', $this->objects[0], rand(500, 299792458));
...
+    $stores[0]->setWithExpire('foo', $this->objects[0], rand(500, 299792458));

+++ b/core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.phpundefined
@@ -91,65 +82,67 @@ protected function tearDown() {
+      $users[$i] = mt_rand(500, 5000000);

Was not introduced here, but I would love a follow-up to either change this upper-bound value to something sensical or else make it a constant so it's clear what it's referring to (I think I over-read on IRC something about speed of light :))

Down below you are using a simple 5000000 as the upper-bound and this causes no head-scratching. I would recommend doing the same here (and other places where this number is used).

+++ b/core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.phpundefined
@@ -91,65 +82,67 @@ protected function tearDown() {
-  protected function getStorePerUID($uid) {

I was confused why this was removed, and tim said:

webchick: its a test helper only that wasn't helpful
webchick: the stores can't be per class, they need to be per method
webchick: or they don't get torn down properly

so this is fair enough.

Committed and pushed to 8.x. Thanks!

Filed: #1808544: Use a more easily recognized arbitrary integer in k/v expiration tests :)

Fatal error: Exception thrown without a stack frame in Unknown on line 0

I am getting the above error on loading the site. How to resolve this ? Please help

Refer following link I resolved my #21 issue from this blog post. Thanks Zufelt.
http://zufelt.ca/blog/trying-catch-missing-stack-frame