# HG changeset patch
# User Sebastien Jodogne <s.jodogne@gmail.com>
# Date 1557342558 -7200
# Node ID e26690365c2501ab54bd92f9a33acb1475436e94
# Parent  cc3dc759c989dd9084c1f5fa7d57fd95a854a9b6
MySQL: Added an advisory lock to avoid race conditions during database setup

diff -r cc3dc759c989 -r e26690365c25 Framework/MySQL/MySQLDatabase.cpp
--- a/Framework/MySQL/MySQLDatabase.cpp	Wed May 08 20:21:13 2019 +0200
+++ b/Framework/MySQL/MySQLDatabase.cpp	Wed May 08 21:09:18 2019 +0200
@@ -35,6 +35,7 @@
 #include <mysqld_error.h>
 
 #include <memory>
+#include <boost/thread.hpp>
 
 namespace OrthancDatabases
 {
@@ -289,32 +290,45 @@
   }
 
 
+  bool MySQLDatabase::RunAdvisoryLockStatement(const std::string& s)
+  {
+    Query query(s, false);
+    MySQLStatement statement(*this, query);
+
+    MySQLTransaction t(*this);
+    Dictionary args;
+
+    std::auto_ptr<IResult> result(t.Execute(statement, args));
+
+    bool 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)
+  {
+    return RunAdvisoryLockStatement("SELECT GET_LOCK('Lock" +
+                                    boost::lexical_cast<std::string>(lock) + "', 0);");
+  }
+  
+
+  bool MySQLDatabase::ReleaseAdvisoryLock(int32_t lock)
+  {
+    return RunAdvisoryLockStatement("SELECT RELEASE_LOCK('Lock" +
+                                    boost::lexical_cast<std::string>(lock) + "');");
+  }
+
+
   void MySQLDatabase::AdvisoryLock(int32_t lock)
   {
-    try
-    {
-      Query query("SELECT GET_LOCK('Lock" +
-                  boost::lexical_cast<std::string>(lock) + "', 0);", false);
-      MySQLStatement statement(*this, query);
-
-      MySQLTransaction t(*this);
-      Dictionary args;
-
-      std::auto_ptr<IResult> result(t.Execute(statement, args));
-
-      if (result->IsDone() ||
-          result->GetField(0).GetType() != ValueType_Integer64 ||
-          dynamic_cast<const Integer64Value&>(result->GetField(0)).GetValue() != 1)
-      {
-        throw Orthanc::OrthancException(Orthanc::ErrorCode_Database);
-      }
-
-      t.Commit();
-    }
-    catch (Orthanc::OrthancException&)
+    if (!AcquireAdvisoryLock(lock))
     {
       LOG(ERROR) << "The MySQL database is locked by another instance of Orthanc";
-      Close();
       throw Orthanc::OrthancException(Orthanc::ErrorCode_Database);
     }
   }
@@ -495,4 +509,39 @@
 
     return true;
   }
+
+
+  MySQLDatabase::TransientAdvisoryLock::
+  TransientAdvisoryLock(MySQLDatabase&  database,
+                        int32_t lock) :
+    database_(database),
+    lock_(lock)
+  {
+    bool locked = true;
+    
+    for (unsigned int i = 0; i < 10; i++)
+    {
+      if (database_.AcquireAdvisoryLock(lock_))
+      {
+        locked = false;
+        break;
+      }
+      else
+      {
+        boost::this_thread::sleep(boost::posix_time::milliseconds(500));
+      }
+    }
+
+    if (locked)
+    {
+      LOG(ERROR) << "Cannot acquire a transient advisory lock";
+      throw Orthanc::OrthancException(Orthanc::ErrorCode_Plugin);
+    }    
+  }
+    
+
+  MySQLDatabase::TransientAdvisoryLock::~TransientAdvisoryLock()
+  {
+    database_.ReleaseAdvisoryLock(lock_);
+  }
 }
diff -r cc3dc759c989 -r e26690365c25 Framework/MySQL/MySQLDatabase.h
--- a/Framework/MySQL/MySQLDatabase.h	Wed May 08 20:21:13 2019 +0200
+++ b/Framework/MySQL/MySQLDatabase.h	Wed May 08 21:09:18 2019 +0200
@@ -44,6 +44,8 @@
     
     void Close();
 
+    bool RunAdvisoryLockStatement(const std::string& statement);
+
   public:
     MySQLDatabase(const MySQLParameters& parameters);
 
@@ -70,6 +72,10 @@
     bool LookupGlobalIntegerVariable(int64_t& value,
                                      const std::string& variable);
 
+    bool AcquireAdvisoryLock(int32_t lock);
+
+    bool ReleaseAdvisoryLock(int32_t lock);
+
     void AdvisoryLock(int32_t lock);
 
     void Execute(const std::string& sql,
@@ -93,5 +99,18 @@
     static void GlobalFinalization();
 
     static bool IsValidDatabaseIdentifier(const std::string& s);
+
+    class TransientAdvisoryLock
+    {
+    private:
+      MySQLDatabase&  database_;
+      int32_t         lock_;
+
+    public:
+      TransientAdvisoryLock(MySQLDatabase&  database,
+                            int32_t lock);
+
+      ~TransientAdvisoryLock();
+    };
   };
 }
diff -r cc3dc759c989 -r e26690365c25 MySQL/NEWS
--- a/MySQL/NEWS	Wed May 08 20:21:13 2019 +0200
+++ b/MySQL/NEWS	Wed May 08 21:09:18 2019 +0200
@@ -2,6 +2,7 @@
 ===============================
 
 * Implementation of new extensions: LookupResourceAndParent and GetAllMetadata
+* Added an advisory lock to avoid race conditions during database setup
 
 
 Release 2.0 (2019-01-23)
diff -r cc3dc759c989 -r e26690365c25 MySQL/Plugins/MySQLIndex.cpp
--- a/MySQL/Plugins/MySQLIndex.cpp	Wed May 08 20:21:13 2019 +0200
+++ b/MySQL/Plugins/MySQLIndex.cpp	Wed May 08 21:09:18 2019 +0200
@@ -76,6 +76,14 @@
     {
       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 */);
     
     {
       MySQLTransaction t(*db);
diff -r cc3dc759c989 -r e26690365c25 MySQL/Plugins/MySQLStorageArea.cpp
--- a/MySQL/Plugins/MySQLStorageArea.cpp	Wed May 08 20:21:13 2019 +0200
+++ b/MySQL/Plugins/MySQLStorageArea.cpp	Wed May 08 21:09:18 2019 +0200
@@ -42,6 +42,14 @@
       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 */);
+    
     {
       MySQLTransaction t(*db);
 
diff -r cc3dc759c989 -r e26690365c25 MySQL/UnitTests/UnitTestsMain.cpp
--- a/MySQL/UnitTests/UnitTestsMain.cpp	Wed May 08 20:21:13 2019 +0200
+++ b/MySQL/UnitTests/UnitTestsMain.cpp	Wed May 08 21:09:18 2019 +0200
@@ -63,6 +63,46 @@
 }
 
 
+TEST(MySQL, Lock2)
+{
+  OrthancDatabases::MySQLDatabase::ClearDatabase(globalParameters_);  
+
+  OrthancDatabases::MySQLDatabase db1(globalParameters_);
+  db1.Open();
+
+  ASSERT_FALSE(db1.ReleaseAdvisoryLock(43)); // lock counter = 0
+  ASSERT_TRUE(db1.AcquireAdvisoryLock(43));  // 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
+
+  // 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
+
+  {
+    OrthancDatabases::MySQLDatabase db2(globalParameters_);
+    db2.Open();
+
+    // The "db1" is still actively locking
+    ASSERT_FALSE(db2.AcquireAdvisoryLock(43));
+
+    // Release the "db1" lock
+    ASSERT_TRUE(db1.ReleaseAdvisoryLock(43));
+    ASSERT_FALSE(db1.ReleaseAdvisoryLock(43));
+
+    // "db2" can now acquire the lock, but not "db1"
+    ASSERT_TRUE(db2.AcquireAdvisoryLock(43));
+    ASSERT_FALSE(db1.AcquireAdvisoryLock(43));
+  }
+
+  // "db2" is closed, "db1" can now acquire the lock
+  ASSERT_TRUE(db1.AcquireAdvisoryLock(43));
+}
+
+
 static int64_t CountFiles(OrthancDatabases::MySQLDatabase& db)
 {
   OrthancDatabases::Query query("SELECT COUNT(*) FROM StorageArea", true);