Revert "Revert prevent race condition in plugin settings #15249 (#15325…

…)" (#15328) This reverts commit 520ba48.
matomo-org · Dec 30, 2019 · 71f81a7 · 71f81a7
1 parent 6131735
commit 71f81a7
Show file tree

Hide file tree

Showing 7 changed files with 120 additions and 59 deletions.
diff --git a/config/global.php b/config/global.php
@@ -216,6 +216,6 @@
     'archiving.performance.logger' => null,
 
     \Piwik\CronArchive\Performance\Logger::class => DI\object()->constructorParameter('logger', DI\get('archiving.performance.logger')),
-    
+
     \Piwik\Concurrency\LockBackend::class => \DI\get(\Piwik\Concurrency\LockBackend\MySqlLockBackend::class),
 );
diff --git a/core/Concurrency/Lock.php b/core/Concurrency/Lock.php
@@ -12,6 +12,8 @@
 
 class Lock
 {
+    const MAX_KEY_LEN = 70;
+
     /**
      * @var LockBackend
      */
@@ -46,6 +48,30 @@ public function getAllAcquiredLockKeys()
         return $this->backend->getKeysMatchingPattern($this->lockKeyStart . '*');
     }
 
+    public function execute($id, $callback)
+    {
+        if (Common::mb_strlen($id) > self::MAX_KEY_LEN) {
+            // Lock key might be too long for DB column, so we hash it but leave the start of the original as well
+            // to make it more readable
+            $md5Len = 32;
+            $id = Common::mb_substr($id, 0, self::MAX_KEY_LEN - $md5Len - 1) . md5($id);
+        }
+
+        $i = 0;
+        while (!$this->acquireLock($id)) {
+            $i++;
+            usleep( 100 * 1000 ); // 100ms
+            if ($i > 50) { // give up after 5seconds (50 * 100ms)
+                throw new \Exception('Could not get the lock for ID: ' . $id);
+            }
+        };
+        try {
+            return $callback();
+        } finally {
+            $this->unlock();
+        }
+    }
+
     public function acquireLock($id, $ttlInSeconds = 60)
     {
         $this->lockKey = $this->lockKeyStart . $id;

diff --git a/core/Db/Schema/Mysql.php b/core/Db/Schema/Mysql.php
@@ -10,6 +10,7 @@
 
 use Exception;
 use Piwik\Common;
+use Piwik\Concurrency\Lock;
 use Piwik\Date;
 use Piwik\Db\SchemaInterface;
 use Piwik\Db;
@@ -312,7 +313,7 @@ public function getTablesCreateSql()
                                   ) ENGINE=$engine DEFAULT CHARSET=utf8
             ",
             'locks'                   => "CREATE TABLE `{$prefixTables}locks` (
-                                      `key` VARCHAR(70) NOT NULL,
+                                      `key` VARCHAR(".Lock::MAX_KEY_LEN.") NOT NULL,
                                       `value` VARCHAR(255) NULL DEFAULT NULL,
                                       `expiry_time` BIGINT UNSIGNED DEFAULT 9999999999,
                                       PRIMARY KEY (`key`)

diff --git a/core/Settings/Storage/Backend/BaseSettingsTable.php b/core/Settings/Storage/Backend/BaseSettingsTable.php
@@ -0,0 +1,41 @@
+<?php
+/**
+ * Matomo - free/libre analytics platform
+ *
+ * @link https://matomo.org
+ * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
+ *
+ */
+namespace Piwik\Settings\Storage\Backend;
+
+use Piwik\Concurrency\Lock;
+use Piwik\Container\StaticContainer;
+use Piwik\Db;
+
+abstract class BaseSettingsTable implements BackendInterface
+{
+    /**
+     * @var Db\AdapterInterface
+     */
+    protected $db;
+
+    /** @var Lock */
+    protected $lock;
+
+    public function __construct()
+    {
+        $this->lock = StaticContainer::getContainer()->make(
+            Lock::class,
+            array ('lockKeyStart' => 'PluginSettingsTable')
+        );
+    }
+
+    protected function initDbIfNeeded()
+    {
+        if (!isset($this->db)) {
+            // we do not want to create a db connection on backend creation
+            $this->db = Db::get();
+        }
+    }
+
+}
diff --git a/core/Settings/Storage/Backend/MeasurableSettingsTable.php b/core/Settings/Storage/Backend/MeasurableSettingsTable.php
@@ -10,6 +10,8 @@
 namespace Piwik\Settings\Storage\Backend;
 
 use Piwik\Common;
+use Piwik\Concurrency\Lock;
+use Piwik\Container\StaticContainer;
 use Piwik\Db;
 use Exception;
 use Piwik\Version;
@@ -19,7 +21,7 @@
  *
  * If a value that needs to be stored is an array, will insert a new row for each value of this array.
  */
-class MeasurableSettingsTable implements BackendInterface
+class MeasurableSettingsTable extends BaseSettingsTable
 {
     /**
      * @var int
@@ -31,13 +33,10 @@ class MeasurableSettingsTable implements BackendInterface
      */
     private $pluginName;
 
-    /**
-     * @var Db\AdapterInterface
-     */
-    private $db;
-
     public function __construct($idSite, $pluginName)
     {
+        parent::__construct();
+
         if (empty($pluginName)) {
             throw new Exception('No plugin name given for MeasurableSettingsTable backend');
         }
@@ -50,39 +49,19 @@ public function __construct($idSite, $pluginName)
         $this->pluginName = $pluginName;
     }
 
-    private function initDbIfNeeded()
-    {
-        if (!isset($this->db)) {
-            // we need to avoid db creation on instance creation, especially important in tracker mode
-            // the db might be never actually used when values are eg fetched from a cache
-            $this->db = Db::get();
-        }
-    }
-
-    /**
-     * @inheritdoc
-     */
-    public function getStorageId()
-    {
-        return 'MeasurableSettings_' . $this->idSite . '_' . $this->pluginName;
-    }
-
     /**
      * Saves (persists) the current setting values in the database.
+     * @param array $values Key/value pairs of setting values to be written
      */
     public function save($values)
     {
         $this->initDbIfNeeded();
 
-        $table = $this->getTableName();
-
-        $this->delete();
-
+        $valuesKeep = array();
         foreach ($values as $name => $value) {
             if (!isset($value)) {
                 continue;
             }
-
             if (is_array($value) || is_object($value)) {
                 $jsonEncoded = 1;
                 $value = json_encode($value);
@@ -94,12 +73,28 @@ public function save($values)
                     $value = (int) $value;
                 }
             }
+            $valuesKeep[] = array($this->idSite, $this->pluginName, $name, $value, $jsonEncoded);
+        }
 
-            $sql  = "INSERT INTO $table (`idsite`, `plugin_name`, `setting_name`, `setting_value`, `json_encoded`) VALUES (?, ?, ?, ?, ?)";
-            $bind = array($this->idSite, $this->pluginName, $name, $value, $jsonEncoded);
+        $columns = array('idsite', 'plugin_name', 'setting_name', 'setting_value', 'json_encoded');
 
-            $this->db->query($sql, $bind);
-        }
+        $table = $this->getTableName();
+        $lockKey = $this->getStorageId();
+        $this->lock->execute($lockKey, function() use ($valuesKeep, $table, $columns) {
+            $this->delete();
+            // No values = nothing to save
+            if (!empty($valuesKeep)) {
+                Db\BatchInsert::tableInsertBatchSql($table, $columns, $valuesKeep, true);
+            }
+        });
+    }
+
+    /**
+     * @inheritdoc
+     */
+    public function getStorageId()
+    {
+        return 'MeasurableSettings_' . $this->idSite . '_' . $this->pluginName;
     }
 
     private function jsonEncodedMissingError(Exception $e)
@@ -149,7 +144,7 @@ public function load()
         return $flat;
     }
 
-    private function getTableName()
+    protected function getTableName()
     {
         return Common::prefixTable('site_setting');
     }

diff --git a/core/Settings/Storage/Backend/PluginSettingsTable.php b/core/Settings/Storage/Backend/PluginSettingsTable.php
@@ -10,6 +10,8 @@
 namespace Piwik\Settings\Storage\Backend;
 
 use Piwik\Common;
+use Piwik\Concurrency\Lock;
+use Piwik\Container\StaticContainer;
 use Piwik\Db;
 use Exception;
 use Piwik\Version;
@@ -19,7 +21,7 @@
  *
  * If a value that needs to be stored is an array, will insert a new row for each value of this array.
  */
-class PluginSettingsTable implements BackendInterface
+class PluginSettingsTable extends BaseSettingsTable
 {
     /**
      * @var string
@@ -31,13 +33,10 @@ class PluginSettingsTable implements BackendInterface
      */
     private $userLogin;
 
-    /**
-     * @var Db\AdapterInterface
-     */
-    private $db;
-
     public function __construct($pluginName, $userLogin)
     {
+        parent::__construct();
+
         if (empty($pluginName)) {
             throw new Exception('No plugin name given for PluginSettingsTable backend');
         }
@@ -50,14 +49,6 @@ public function __construct($pluginName, $userLogin)
         $this->userLogin = $userLogin;
     }
 
-    private function initDbIfNeeded()
-    {
-        if (!isset($this->db)) {
-            // we do not want to create a db connection on backend creation
-            $this->db = Db::get();
-        }
-    }
-
     /**
      * @inheritdoc
      */
@@ -68,20 +59,18 @@ public function getStorageId()
 
     /**
      * Saves (persists) the current setting values in the database.
+     * @param array $values Key/value pairs of setting values to be written
      */
     public function save($values)
     {
         $this->initDbIfNeeded();
 
-        $table = $this->getTableName();
-
-        $this->delete();
+        $valuesKeep = array();
 
         foreach ($values as $name => $value) {
             if (!isset($value)) {
                 continue;
             }
-
             if (is_array($value) || is_object($value)) {
                 $jsonEncoded = 1;
                 $value = json_encode($value);
@@ -94,11 +83,20 @@ public function save($values)
                 }
             }
 
-            $sql  = "INSERT INTO $table (`plugin_name`, `user_login`, `setting_name`, `setting_value`, `json_encoded`) VALUES (?, ?, ?, ?, ?)";
-            $bind = array($this->pluginName, $this->userLogin, $name, $value, $jsonEncoded);
-
-            $this->db->query($sql, $bind);
+            $valuesKeep[] = array($this->pluginName, $this->userLogin, $name, $value, $jsonEncoded);
         }
+
+        $columns = array('plugin_name', 'user_login', 'setting_name', 'setting_value', 'json_encoded');
+
+        $table = $this->getTableName();
+        $lockKey = $this->getStorageId();
+        $this->lock->execute($lockKey, function() use ($valuesKeep, $table, $columns) {
+            $this->delete();
+            // No values = nothing to save
+            if (!empty($valuesKeep)) {
+                Db\BatchInsert::tableInsertBatchSql($table, $columns, $valuesKeep);
+            }
+        });
     }
 
     private function jsonEncodedMissingError(Exception $e)
@@ -145,7 +143,7 @@ public function load()
         return $flat;
     }
 
-    private function getTableName()
+    protected function getTableName()
     {
         return Common::prefixTable('plugin_setting');
     }

diff --git a/plugins/CorePluginsAdmin/Model/TagManagerTeaser.php b/plugins/CorePluginsAdmin/Model/TagManagerTeaser.php
@@ -68,7 +68,7 @@ public function reset()
         Option::delete(self::DISABLE_GLOBALLY_KEY);
 
         // no need to keep any old login entries
-        $this->getTable()->save(array());
+        $this->getTable()->delete();
     }
 
     public function isEnabledGlobally()