Revert prevent race condition in plugin settings #15249 (#15325)

matomo-org · Dec 30, 2019 · 520ba48 · 520ba48
1 parent 4754e0e
commit 520ba48
Show file tree

Hide file tree

Showing 7 changed files with 59 additions and 120 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,8 +12,6 @@
 
 class Lock
 {
-    const MAX_KEY_LEN = 70;
-
     /**
      * @var LockBackend
      */
@@ -48,30 +46,6 @@ 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,7 +10,6 @@
 
 use Exception;
 use Piwik\Common;
-use Piwik\Concurrency\Lock;
 use Piwik\Date;
 use Piwik\Db\SchemaInterface;
 use Piwik\Db;
@@ -313,7 +312,7 @@ public function getTablesCreateSql()
                                   ) ENGINE=$engine DEFAULT CHARSET=utf8
             ",
             'locks'                   => "CREATE TABLE `{$prefixTables}locks` (
-                                      `key` VARCHAR(".Lock::MAX_KEY_LEN.") NOT NULL,
+                                      `key` VARCHAR(70) 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
diff --git a/core/Settings/Storage/Backend/MeasurableSettingsTable.php b/core/Settings/Storage/Backend/MeasurableSettingsTable.php
@@ -10,8 +10,6 @@
 namespace Piwik\Settings\Storage\Backend;
 
 use Piwik\Common;
-use Piwik\Concurrency\Lock;
-use Piwik\Container\StaticContainer;
 use Piwik\Db;
 use Exception;
 use Piwik\Version;
@@ -21,7 +19,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 extends BaseSettingsTable
+class MeasurableSettingsTable implements BackendInterface
 {
     /**
      * @var int
@@ -33,10 +31,13 @@ class MeasurableSettingsTable extends BaseSettingsTable
      */
     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');
         }
@@ -49,19 +50,39 @@ 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();
 
-        $valuesKeep = array();
+        $table = $this->getTableName();
+
+        $this->delete();
+
         foreach ($values as $name => $value) {
             if (!isset($value)) {
                 continue;
             }
+
             if (is_array($value) || is_object($value)) {
                 $jsonEncoded = 1;
                 $value = json_encode($value);
@@ -73,28 +94,12 @@ public function save($values)
                     $value = (int) $value;
                 }
             }
-            $valuesKeep[] = array($this->idSite, $this->pluginName, $name, $value, $jsonEncoded);
-        }
-
-        $columns = array('idsite', 'plugin_name', '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, true);
-            }
-        });
-    }
+            $sql  = "INSERT INTO $table (`idsite`, `plugin_name`, `setting_name`, `setting_value`, `json_encoded`) VALUES (?, ?, ?, ?, ?)";
+            $bind = array($this->idSite, $this->pluginName, $name, $value, $jsonEncoded);
 
-    /**
-     * @inheritdoc
-     */
-    public function getStorageId()
-    {
-        return 'MeasurableSettings_' . $this->idSite . '_' . $this->pluginName;
+            $this->db->query($sql, $bind);
+        }
     }
 
     private function jsonEncodedMissingError(Exception $e)
@@ -144,7 +149,7 @@ public function load()
         return $flat;
     }
 
-    protected function getTableName()
+    private function getTableName()
     {
         return Common::prefixTable('site_setting');
     }

diff --git a/core/Settings/Storage/Backend/PluginSettingsTable.php b/core/Settings/Storage/Backend/PluginSettingsTable.php
@@ -10,8 +10,6 @@
 namespace Piwik\Settings\Storage\Backend;
 
 use Piwik\Common;
-use Piwik\Concurrency\Lock;
-use Piwik\Container\StaticContainer;
 use Piwik\Db;
 use Exception;
 use Piwik\Version;
@@ -21,7 +19,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 extends BaseSettingsTable
+class PluginSettingsTable implements BackendInterface
 {
     /**
      * @var string
@@ -33,10 +31,13 @@ class PluginSettingsTable extends BaseSettingsTable
      */
     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');
         }
@@ -49,6 +50,14 @@ 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
      */
@@ -59,18 +68,20 @@ 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();
 
-        $valuesKeep = array();
+        $table = $this->getTableName();
+
+        $this->delete();
 
         foreach ($values as $name => $value) {
             if (!isset($value)) {
                 continue;
             }
+
             if (is_array($value) || is_object($value)) {
                 $jsonEncoded = 1;
                 $value = json_encode($value);
@@ -83,20 +94,11 @@ public function save($values)
                 }
             }
 
-            $valuesKeep[] = array($this->pluginName, $this->userLogin, $name, $value, $jsonEncoded);
-        }
-
-        $columns = array('plugin_name', 'user_login', 'setting_name', 'setting_value', 'json_encoded');
+            $sql  = "INSERT INTO $table (`plugin_name`, `user_login`, `setting_name`, `setting_value`, `json_encoded`) VALUES (?, ?, ?, ?, ?)";
+            $bind = array($this->pluginName, $this->userLogin, $name, $value, $jsonEncoded);
 
-        $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);
-            }
-        });
+            $this->db->query($sql, $bind);
+        }
     }
 
     private function jsonEncodedMissingError(Exception $e)
@@ -143,7 +145,7 @@ public function load()
         return $flat;
     }
 
-    protected function getTableName()
+    private 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()->delete();
+        $this->getTable()->save(array());
     }
 
     public function isEnabledGlobally()