Issue #2186113 by Berdir: Avoid key value expire garbage collection on actual requests

core/core.services.yml

@@ -197,8 +197,6 @@ services:
   keyvalue.expirable.database:
     class: Drupal\Core\KeyValueStore\KeyValueDatabaseExpirableFactory
     arguments: ['@serialization.phpserialize', '@database']
-    tags:
-      - { name: needs_destruction }
   logger.factory:
     class: Drupal\Core\Logger\LoggerChannelFactory
     parent: container.trait

core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php

@@ -8,7 +8,6 @@
 namespace Drupal\Core\KeyValueStore;
 
 use Drupal\Component\Serialization\SerializationInterface;
-use Drupal\Core\DestructableInterface;
 use Drupal\Core\Database\Connection;
 use Drupal\Core\Database\Query\Merge;
 
@@ -18,20 +17,7 @@
  * This key/value store implementation uses the database to store key/value
  * data with an expire date.
  */
-class DatabaseStorageExpirable extends DatabaseStorage implements KeyValueStoreExpirableInterface, DestructableInterface {
-
-  /**
-   * Flag indicating whether garbage collection should be performed.
-   *
-   * When this flag is TRUE, garbage collection happens at the end of the
-   * request when the object is destructed. The flag is set during set and
-   * delete operations for expirable data, when a write to the table is already
-   * being performed. This eliminates the need for an external system to remove
-   * stale data.
-   *
-   * @var bool
-   */
-  protected $needsGarbageCollection = FALSE;
+class DatabaseStorageExpirable extends DatabaseStorage implements KeyValueStoreExpirableInterface {
 
   /**
    * Overrides Drupal\Core\KeyValueStore\StorageBase::__construct().
@@ -91,9 +77,6 @@ public function getAll() {
    * {@inheritdoc}
    */
   function setWithExpire($key, $value, $expire) {
-    // We are already writing to the table, so perform garbage collection at
-    // the end of this request.
-    $this->needsGarbageCollection = TRUE;
     $this->connection->merge($this->table)
       ->keys(array(
         'name' => $key,
@@ -110,9 +93,6 @@ function setWithExpire($key, $value, $expire) {
    * Implements Drupal\Core\KeyValueStore\KeyValueStoreExpirableInterface::setWithExpireIfNotExists().
    */
   function setWithExpireIfNotExists($key, $value, $expire) {
-    // We are already writing to the table, so perform garbage collection at
-    // the end of this request.
-    $this->needsGarbageCollection = TRUE;
     $result = $this->connection->merge($this->table)
       ->insertFields(array(
         'collection' => $this->collection,
@@ -139,28 +119,8 @@ function setMultipleWithExpire(array $data, $expire) {
    * Implements Drupal\Core\KeyValueStore\KeyValueStoreInterface::deleteMultiple().
    */
   public function deleteMultiple(array $keys) {
-    // We are already writing to the table, so perform garbage collection at
-    // the end of this request.
-    $this->needsGarbageCollection = TRUE;
     parent::deleteMultiple($keys);
   }
 
-  /**
-   * Deletes expired items.
-   */
-  protected function garbageCollection() {
-    $this->connection->delete($this->table)
-      ->condition('expire', REQUEST_TIME, '<')
-      ->execute();
-  }
-
-  /**
-   * Implements Drupal\Core\DestructableInterface::destruct().
-   */
-  public function destruct() {
-    if ($this->needsGarbageCollection) {
-      $this->garbageCollection();
-    }
-  }
 
 }

core/lib/Drupal/Core/KeyValueStore/KeyValueDatabaseExpirableFactory.php

@@ -14,7 +14,7 @@
 /**
  * Defines the key/value store factory for the database backend.
  */
-class KeyValueDatabaseExpirableFactory implements KeyValueExpirableFactoryInterface, DestructableInterface {
+class KeyValueDatabaseExpirableFactory implements KeyValueExpirableFactoryInterface {
 
   /**
    * Holds references to each instantiation so they can be terminated.
@@ -61,14 +61,11 @@ public function get($collection) {
   }
 
   /**
-   * {@inheritdoc}
+   * Deletes expired items.
    */
-  public function destruct() {
-    if (!empty($this->storages)) {
-      // Each instance does garbage collection for all collections, so we can
-      // optimize and only have to call the first, avoids multiple DELETE.
-      $storage = reset($this->storages);
-      $storage->destruct();
-    }
+  public function garbageCollection() {
+    $this->connection->delete('key_value_expire')
+      ->condition('expire', REQUEST_TIME, '<')
+      ->execute();
   }
 }

core/modules/system/src/Tests/KeyValueStore/GarbageCollectionTest.php

@@ -28,9 +28,10 @@ class GarbageCollectionTest extends KernelTestBase {
 
   protected function setUp() {
     parent::setUp();
-    $this->installSchema('system', array('key_value_expire'));
-  }
 
+    // These additional tables are necessary due to the call to system_cron().
+    $this->installSchema('system', array('key_value_expire', 'flood', 'queue'));
+  }
 
   /**
    * Tests garbage collection.
@@ -58,10 +59,10 @@ public function testGarbageCollection() {
         ->execute();
     }
 
-    // Perform a new set operation and then manually destruct the object to
-    // trigger garbage collection.
+
+    // Perform a new set operation and then trigger garbage collection.
     $store->setWithExpire('autumn', 'winter', rand(500, 1000000));
-    $store->destruct();
+    system_cron();
 
     // Query the database and confirm that the stale records were deleted.
     $result = db_query(
@@ -69,7 +70,7 @@ public function testGarbageCollection() {
       array(
         ':collection' => $collection,
       ))->fetchAll();
-    $this->assertIdentical(sizeof($result), 1, 'Only one item remains after garbage collection');
+    $this->assertIdentical(count($result), 1, 'Only one item remains after garbage collection');
 
   }
 

core/modules/system/system.module

@@ -9,10 +9,9 @@
 use Drupal\Core\Cache\Cache;
 use Drupal\Core\Extension\Extension;
 use Drupal\Core\Extension\ExtensionDiscovery;
-use Drupal\Core\Form\FormState;
 use Drupal\Core\Form\FormStateInterface;
+use Drupal\Core\KeyValueStore\KeyValueDatabaseExpirableFactory;
 use Drupal\Core\Routing\RouteMatchInterface;
-use Drupal\Core\StringTranslation\TranslationWrapper;
 use Drupal\Core\Language\LanguageInterface;
 use Drupal\Core\Menu\MenuTreeParameters;
 use Drupal\Core\Url;
@@ -1187,17 +1186,22 @@ function system_get_module_admin_tasks($module, array $info) {
 /**
  * Implements hook_cron().
  *
- * Remove older rows from flood and batch table. Remove old temporary files.
+ * Remove older rows from flood, batch cache and expirable keyvalue tables.
  */
 function system_cron() {
-  // Cleanup the flood.
+  // Clean up the flood.
   \Drupal::flood()->garbageCollection();
 
   foreach (Cache::getBins() as $cache_backend) {
     $cache_backend->garbageCollection();
   }
 
-  // Cleanup the queue for failed batches.
+  // Clean up the expirable key value database store.
+  if (\Drupal::service('keyvalue.expirable.database') instanceof KeyValueDatabaseExpirableFactory) {
+    \Drupal::service('keyvalue.expirable.database')->garbageCollection();
+  }
+
+  // Clean up the queue for failed batches.
   db_delete('queue')
     ->condition('created', REQUEST_TIME - 864000, '<')
     ->condition('name', 'drupal_batch:%', 'LIKE')