# HG changeset patch
# User Sebastien Jodogne <s.jodogne@gmail.com>
# Date 1557394157 -7200
# Node ID 52b3859ee0b755a7c0edd633dc8eda0301a1725c
# Parent  3266785d5627dc2fb13ba46a6941e9c2132ca1a2
MySQL: acquiring named locks instead of numbers

diff -r 3266785d5627 -r 52b3859ee0b7 Framework/MySQL/MySQLDatabase.cpp
--- a/Framework/MySQL/MySQLDatabase.cpp	Thu May 09 09:52:11 2019 +0200
+++ b/Framework/MySQL/MySQLDatabase.cpp	Thu May 09 11:29:17 2019 +0200
@@ -290,41 +290,65 @@
   }
 
 
-  bool MySQLDatabase::RunAdvisoryLockStatement(const std::string& s)
+  bool MySQLDatabase::RunAdvisoryLockStatement(Query& query,
+                                               const std::string& lock)
   {
-    Query query(s, false);
+    const std::string& dbName = parameters_.GetDatabase();
+
+    // Prepend the name of the lock by the database name. This allows
+    // to create a namespace for advisory locks:
+    // https://groups.google.com/d/msg/orthanc-users/yV3LSTh_TjI/MQIcvnMlAQAJ
+    std::string prefix;
+    prefix.reserve(dbName.size());
+    for (size_t i = 0; i < dbName.size(); i++)
+    {
+      if (isalnum(dbName[i]) ||
+          dbName[i] == '$' ||
+          dbName[i] == '_')
+      {
+        prefix.push_back(dbName[i]);
+      }
+    }
+
+    query.SetType("lock", ValueType_Utf8String);
+    
     MySQLStatement statement(*this, query);
 
-    MySQLTransaction t(*this);
     Dictionary args;
+    args.SetUtf8Value("lock", prefix + "." + lock);
 
-    std::auto_ptr<IResult> result(t.Execute(statement, args));
+    bool success;
 
-    bool success = (!result->IsDone() &&
-                    result->GetField(0).GetType() == ValueType_Integer64 &&
-                    dynamic_cast<const Integer64Value&>(result->GetField(0)).GetValue() == 1);
+    {
+      MySQLTransaction t(*this);
+      std::auto_ptr<IResult> result(t.Execute(statement, args));
 
-    t.Commit();
+      success = (!result->IsDone() &&
+                 result->GetField(0).GetType() == ValueType_Integer64 &&
+                 dynamic_cast<const Integer64Value&>(result->GetField(0)).GetValue() == 1);
+      
+      t.Commit();
+    }
 
     return success;
   }
   
 
-  bool MySQLDatabase::AcquireAdvisoryLock(int32_t lock)
+  bool MySQLDatabase::AcquireAdvisoryLock(const std::string& lock)
   {
-    return RunAdvisoryLockStatement("SELECT GET_LOCK('Lock" +
-                                    boost::lexical_cast<std::string>(lock) + "', 0);");
+    Query query("SELECT GET_LOCK(${lock}, 0)", false);
+    return RunAdvisoryLockStatement(query, lock);
   }
   
 
-  bool MySQLDatabase::ReleaseAdvisoryLock(int32_t lock)
+  bool MySQLDatabase::ReleaseAdvisoryLock(const std::string& lock)
   {
-    return RunAdvisoryLockStatement("SELECT RELEASE_LOCK('Lock" +
-                                    boost::lexical_cast<std::string>(lock) + "');");
+    Query query("SELECT RELEASE_LOCK(${lock})", false);
+    return RunAdvisoryLockStatement(query, lock);
   }
 
 
-  void MySQLDatabase::AdvisoryLock(int32_t lock)
+  void MySQLDatabase::AdvisoryLock(const std::string& lock)
   {
     if (!AcquireAdvisoryLock(lock))
     {
@@ -513,7 +537,7 @@
 
   MySQLDatabase::TransientAdvisoryLock::
   TransientAdvisoryLock(MySQLDatabase&  database,
-                        int32_t lock) :
+                        const std::string& lock) :
     database_(database),
     lock_(lock)
   {
diff -r 3266785d5627 -r 52b3859ee0b7 Framework/MySQL/MySQLDatabase.h
--- a/Framework/MySQL/MySQLDatabase.h	Thu May 09 09:52:11 2019 +0200
+++ b/Framework/MySQL/MySQLDatabase.h	Thu May 09 11:29:17 2019 +0200
@@ -44,7 +44,8 @@
     
     void Close();
 
-    bool RunAdvisoryLockStatement(const std::string& statement);
+    bool RunAdvisoryLockStatement(Query& query,
+                                  const std::string& lock);
 
   public:
     MySQLDatabase(const MySQLParameters& parameters);
@@ -72,11 +73,11 @@
     bool LookupGlobalIntegerVariable(int64_t& value,
                                      const std::string& variable);
 
-    bool AcquireAdvisoryLock(int32_t lock);
+    bool AcquireAdvisoryLock(const std::string& lock);
 
-    bool ReleaseAdvisoryLock(int32_t lock);
+    bool ReleaseAdvisoryLock(const std::string& lock);
 
-    void AdvisoryLock(int32_t lock);
+    void AdvisoryLock(const std::string& lock);
 
     void Execute(const std::string& sql,
                  bool arobaseSeparator);
@@ -104,11 +105,11 @@
     {
     private:
       MySQLDatabase&  database_;
-      int32_t         lock_;
+      std::string     lock_;
 
     public:
       TransientAdvisoryLock(MySQLDatabase&  database,
-                            int32_t lock);
+                            const std::string& lock);
 
       ~TransientAdvisoryLock();
     };
diff -r 3266785d5627 -r 52b3859ee0b7 MySQL/Plugins/MySQLDefinitions.h
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/MySQL/Plugins/MySQLDefinitions.h	Thu May 09 11:29:17 2019 +0200
@@ -0,0 +1,49 @@
+/**
+ * Orthanc - A Lightweight, RESTful DICOM Store
+ * Copyright (C) 2012-2016 Sebastien Jodogne, Medical Physics
+ * Department, University Hospital of Liege, Belgium
+ * Copyright (C) 2017-2019 Osimis S.A., Belgium
+ *
+ * This program is free software: you can redistribute it and/or
+ * modify it under the terms of the GNU Affero General Public License
+ * as published by the Free Software Foundation, either version 3 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Affero General Public License for more details.
+ * 
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ **/
+
+
+#pragma once
+
+#include <stdint.h>
+
+
+/**
+ * This advisory lock is used if the "Lock" option is set to "true",
+ * in order to prevent the execution of two PostgreSQL index plugins
+ * on the same database.
+ **/
+static const char* const MYSQL_LOCK_INDEX = "orthanc_index_lock";
+
+
+/**
+ * This advisory lock is used if the "Lock" option is set to "true",
+ * in order to prevent the execution of two PostgreSQL storage area
+ * plugins on the same database.
+ **/
+static const char* const MYSQL_LOCK_STORAGE = "orthanc_storage_lock";
+
+
+/**
+ * Transient advisory lock to protect the setup of the database,
+ * because concurrent statements like "CREATE TABLE" are not protected
+ * by transactions.
+ * https://groups.google.com/d/msg/orthanc-users/yV3LSTh_TjI/h3PRApJFBAAJ
+ **/
+static const char* const MYSQL_LOCK_DATABASE_SETUP = "orthanc_setup_lock";
diff -r 3266785d5627 -r 52b3859ee0b7 MySQL/Plugins/MySQLIndex.cpp
--- a/MySQL/Plugins/MySQLIndex.cpp	Thu May 09 09:52:11 2019 +0200
+++ b/MySQL/Plugins/MySQLIndex.cpp	Thu May 09 11:29:17 2019 +0200
@@ -24,6 +24,7 @@
 #include "../../Framework/Plugins/GlobalProperties.h"
 #include "../../Framework/MySQL/MySQLDatabase.h"
 #include "../../Framework/MySQL/MySQLTransaction.h"
+#include "MySQLDefinitions.h"
 
 #include <EmbeddedResources.h>  // Auto-generated file
 
@@ -69,23 +70,10 @@
     std::auto_ptr<MySQLDatabase> db(new MySQLDatabase(parameters_));
 
     db->Open();
-    
     db->Execute("SET SESSION TRANSACTION ISOLATION LEVEL SERIALIZABLE", false);
 
-    if (parameters_.HasLock())
     {
-      db->AdvisoryLock(42 /* some arbitrary constant */);
-    }
-
-    /**
-     * Try and acquire a transient advisory lock to protect the setup
-     * of the database, because concurrent statements like "CREATE
-     * TABLE" are not protected by transactions.
-     * https://groups.google.com/d/msg/orthanc-users/yV3LSTh_TjI/h3PRApJFBAAJ
-     **/
-    MySQLDatabase::TransientAdvisoryLock lock(*db, 44 /* some arbitrary constant */);
-    
-    {
+      MySQLDatabase::TransientAdvisoryLock lock(*db, MYSQL_LOCK_DATABASE_SETUP);
       MySQLTransaction t(*db);
 
       db->Execute("ALTER DATABASE " + parameters_.GetDatabase() + 
@@ -183,6 +171,19 @@
 
       t.Commit();
     }
+
+    /**
+     * WARNING: This lock must be acquired after
+     * "MYSQL_LOCK_DATABASE_SETUP" is released. Indeed, in MySQL <
+     * 5.7, it is impossible to acquire more than one lock at a time,
+     * as calling "SELECT GET_LOCK()" releases all the
+     * previously-acquired locks.
+     * https://dev.mysql.com/doc/refman/5.7/en/locking-functions.html
+     **/
+    if (parameters_.HasLock())
+    {
+      db->AdvisoryLock(MYSQL_LOCK_INDEX);
+    }
           
     return db.release();
   }
diff -r 3266785d5627 -r 52b3859ee0b7 MySQL/Plugins/MySQLStorageArea.cpp
--- a/MySQL/Plugins/MySQLStorageArea.cpp	Thu May 09 09:52:11 2019 +0200
+++ b/MySQL/Plugins/MySQLStorageArea.cpp	Thu May 09 11:29:17 2019 +0200
@@ -23,6 +23,7 @@
 
 #include "../../Framework/MySQL/MySQLDatabase.h"
 #include "../../Framework/MySQL/MySQLTransaction.h"
+#include "MySQLDefinitions.h"
 
 #include <Core/Logging.h>
 
@@ -37,20 +38,8 @@
 
     db->Open();
 
-    if (parameters_.HasLock())
     {
-      db->AdvisoryLock(43 /* some arbitrary constant */);
-    }
-
-    /**
-     * Try and acquire a transient advisory lock to protect the setup
-     * of the database, because concurrent statements like "CREATE
-     * TABLE" are not protected by transactions.
-     * https://groups.google.com/d/msg/orthanc-users/yV3LSTh_TjI/h3PRApJFBAAJ
-     **/
-    MySQLDatabase::TransientAdvisoryLock lock(*db, 44 /* some arbitrary constant */);
-    
-    {
+      MySQLDatabase::TransientAdvisoryLock lock(*db, MYSQL_LOCK_DATABASE_SETUP);    
       MySQLTransaction t(*db);
 
       int64_t size;
@@ -82,6 +71,19 @@
       t.Commit();
     }
 
+    /**
+     * WARNING: This lock must be acquired after
+     * "MYSQL_LOCK_DATABASE_SETUP" is released. Indeed, in MySQL <
+     * 5.7, it is impossible to acquire more than one lock at a time,
+     * as calling "SELECT GET_LOCK()" releases all the
+     * previously-acquired locks.
+     * https://dev.mysql.com/doc/refman/5.7/en/locking-functions.html
+     **/
+    if (parameters_.HasLock())
+    {
+      db->AdvisoryLock(MYSQL_LOCK_STORAGE);
+    }
+
     return db.release();
   }
 
diff -r 3266785d5627 -r 52b3859ee0b7 MySQL/UnitTests/UnitTestsMain.cpp
--- a/MySQL/UnitTests/UnitTestsMain.cpp	Thu May 09 09:52:11 2019 +0200
+++ b/MySQL/UnitTests/UnitTestsMain.cpp	Thu May 09 11:29:17 2019 +0200
@@ -70,36 +70,60 @@
   OrthancDatabases::MySQLDatabase db1(globalParameters_);
   db1.Open();
 
-  ASSERT_FALSE(db1.ReleaseAdvisoryLock(43)); // lock counter = 0
-  ASSERT_TRUE(db1.AcquireAdvisoryLock(43));  // lock counter = 1
+  ASSERT_FALSE(db1.ReleaseAdvisoryLock("mylock")); // lock counter = 0
+  ASSERT_TRUE(db1.AcquireAdvisoryLock("mylock"));  // lock counter = 1
 
   // OK, as this is the same connection
-  ASSERT_TRUE(db1.AcquireAdvisoryLock(43));  // lock counter = 2
-  ASSERT_TRUE(db1.ReleaseAdvisoryLock(43));  // lock counter = 1
+  ASSERT_TRUE(db1.AcquireAdvisoryLock("mylock"));  // lock counter = 1
+  ASSERT_TRUE(db1.ReleaseAdvisoryLock("mylock"));  // lock counter = 0
 
   // Try and release twice the lock
-  ASSERT_TRUE(db1.ReleaseAdvisoryLock(43));  // lock counter = 0
-  ASSERT_FALSE(db1.ReleaseAdvisoryLock(43)); // cannot unlock
-  ASSERT_TRUE(db1.AcquireAdvisoryLock(43));  // lock counter = 1
+  ASSERT_FALSE(db1.ReleaseAdvisoryLock("mylock"));  // lock counter = 0
+  ASSERT_TRUE(db1.AcquireAdvisoryLock("mylock"));  // lock counter = 1
 
   {
     OrthancDatabases::MySQLDatabase db2(globalParameters_);
     db2.Open();
 
     // The "db1" is still actively locking
-    ASSERT_FALSE(db2.AcquireAdvisoryLock(43));
+    ASSERT_FALSE(db2.AcquireAdvisoryLock("mylock"));
 
     // Release the "db1" lock
-    ASSERT_TRUE(db1.ReleaseAdvisoryLock(43));
-    ASSERT_FALSE(db1.ReleaseAdvisoryLock(43));
+    ASSERT_TRUE(db1.ReleaseAdvisoryLock("mylock"));
+    ASSERT_FALSE(db1.ReleaseAdvisoryLock("mylock"));
 
     // "db2" can now acquire the lock, but not "db1"
-    ASSERT_TRUE(db2.AcquireAdvisoryLock(43));
-    ASSERT_FALSE(db1.AcquireAdvisoryLock(43));
+    ASSERT_TRUE(db2.AcquireAdvisoryLock("mylock"));
+    ASSERT_FALSE(db1.AcquireAdvisoryLock("mylock"));
   }
 
   // "db2" is closed, "db1" can now acquire the lock
-  ASSERT_TRUE(db1.AcquireAdvisoryLock(43));
+  ASSERT_TRUE(db1.AcquireAdvisoryLock("mylock"));
+}
+
+
+
+/**
+ * WARNING: The following test only succeeds if MySQL >= 5.7. This is
+ * because in MySQL < 5.7, acquiring a lock by calling "SELECT
+ * GET_LOCK()" releases all the previously acquired locks!
+ **/
+TEST(MySQL, DISABLED_Lock3)
+{
+  OrthancDatabases::MySQLDatabase::ClearDatabase(globalParameters_);  
+
+  OrthancDatabases::MySQLDatabase db1(globalParameters_);
+  db1.Open();
+
+  ASSERT_TRUE(db1.AcquireAdvisoryLock("mylock1"));  // lock counter = 1
+  ASSERT_TRUE(db1.AcquireAdvisoryLock("mylock2"));  // lock counter = 1
+
+  {
+    OrthancDatabases::MySQLDatabase db2(globalParameters_);
+    db2.Open();
+
+    ASSERT_FALSE(db2.AcquireAdvisoryLock("mylock1"));
+  }
 }