changeset 134:cc3dc759c989

Added an advisory lock to avoid race conditions during database setup
author Sebastien Jodogne <s.jodogne@gmail.com>
date Wed, 08 May 2019 20:21:13 +0200
parents b1d7d255fb73
children e26690365c25
files Framework/PostgreSQL/PostgreSQLDatabase.cpp Framework/PostgreSQL/PostgreSQLDatabase.h PostgreSQL/NEWS PostgreSQL/Plugins/PostgreSQLIndex.cpp PostgreSQL/Plugins/PostgreSQLStorageArea.cpp PostgreSQL/UnitTests/PostgreSQLTests.cpp
diffstat 6 files changed, 142 insertions(+), 7 deletions(-) [+]
line wrap: on
line diff
--- a/Framework/PostgreSQL/PostgreSQLDatabase.cpp	Fri Mar 01 12:56:13 2019 +0100
+++ b/Framework/PostgreSQL/PostgreSQLDatabase.cpp	Wed May 08 20:21:13 2019 +0200
@@ -31,6 +31,7 @@
 #include <Core/OrthancException.h>
 
 #include <boost/lexical_cast.hpp>
+#include <boost/thread.hpp>
 
 
 namespace OrthancDatabases
@@ -109,23 +110,47 @@
   }
 
 
-  void PostgreSQLDatabase::AdvisoryLock(int32_t lock)
+  bool PostgreSQLDatabase::RunAdvisoryLockStatement(const std::string& statement)
   {
     PostgreSQLTransaction transaction(*this);
 
-    Query query("select pg_try_advisory_lock(" + 
-                boost::lexical_cast<std::string>(lock) + ");", false);
+    Query query(statement, false);
     PostgreSQLStatement s(*this, query);
 
     PostgreSQLResult result(s);
-    if (result.IsDone() ||
-        !result.GetBoolean(0))
+
+    bool success = (!result.IsDone() &&
+                    result.GetBoolean(0));
+
+    transaction.Commit();
+
+    return success;
+  }
+  
+
+  bool PostgreSQLDatabase::AcquireAdvisoryLock(int32_t lock)
+  {
+    return RunAdvisoryLockStatement(
+      "select pg_try_advisory_lock(" + 
+      boost::lexical_cast<std::string>(lock) + ")");
+  }
+
+
+  bool PostgreSQLDatabase::ReleaseAdvisoryLock(int32_t lock)
+  {
+    return RunAdvisoryLockStatement(
+      "select pg_advisory_unlock(" + 
+      boost::lexical_cast<std::string>(lock) + ")");
+  }
+
+
+  void PostgreSQLDatabase::AdvisoryLock(int32_t lock)
+  {
+    if (!AcquireAdvisoryLock(lock))
     {
       LOG(ERROR) << "The PostgreSQL database is locked by another instance of Orthanc";
       throw Orthanc::OrthancException(Orthanc::ErrorCode_Database);
     }
-
-    transaction.Commit();
   }
 
 
@@ -243,4 +268,39 @@
       return new PostgreSQLTransaction(*this);
     }
   }
+
+
+  PostgreSQLDatabase::TransientAdvisoryLock::TransientAdvisoryLock(
+    PostgreSQLDatabase&  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);
+    }    
+  }
+    
+
+  PostgreSQLDatabase::TransientAdvisoryLock::~TransientAdvisoryLock()
+  {
+    database_.ReleaseAdvisoryLock(lock_);
+  }
 }
--- a/Framework/PostgreSQL/PostgreSQLDatabase.h	Fri Mar 01 12:56:13 2019 +0100
+++ b/Framework/PostgreSQL/PostgreSQLDatabase.h	Wed May 08 20:21:13 2019 +0200
@@ -43,6 +43,8 @@
 
     void Close();
 
+    bool RunAdvisoryLockStatement(const std::string& statement);
+
   public:
     PostgreSQLDatabase(const PostgreSQLParameters& parameters) :
     parameters_(parameters),
@@ -54,6 +56,10 @@
 
     void Open();
 
+    bool AcquireAdvisoryLock(int32_t lock);
+
+    bool ReleaseAdvisoryLock(int32_t lock);
+
     void AdvisoryLock(int32_t lock);
 
     void Execute(const std::string& sql);
@@ -70,5 +76,18 @@
     virtual IPrecompiledStatement* Compile(const Query& query);
 
     virtual ITransaction* CreateTransaction(bool isImplicit);
+
+    class TransientAdvisoryLock
+    {
+    private:
+      PostgreSQLDatabase&  database_;
+      int32_t              lock_;
+
+    public:
+      TransientAdvisoryLock(PostgreSQLDatabase&  database,
+                            int32_t lock);
+
+      ~TransientAdvisoryLock();
+    };
   };
 }
--- a/PostgreSQL/NEWS	Fri Mar 01 12:56:13 2019 +0100
+++ b/PostgreSQL/NEWS	Wed May 08 20:21:13 2019 +0200
@@ -1,6 +1,8 @@
 Pending changes in the mainline
 ===============================
 
+* Added an advisory lock to avoid race conditions during database setup
+
 
 Release 3.2 (2019-03-01)
 ========================
--- a/PostgreSQL/Plugins/PostgreSQLIndex.cpp	Fri Mar 01 12:56:13 2019 +0100
+++ b/PostgreSQL/Plugins/PostgreSQLIndex.cpp	Wed May 08 20:21:13 2019 +0200
@@ -74,6 +74,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
+     **/
+    PostgreSQLDatabase::TransientAdvisoryLock lock(*db, 44 /* some arbitrary constant */);
+
     if (clearAll_)
     {
       db->ClearAll();
--- a/PostgreSQL/Plugins/PostgreSQLStorageArea.cpp	Fri Mar 01 12:56:13 2019 +0100
+++ b/PostgreSQL/Plugins/PostgreSQLStorageArea.cpp	Wed May 08 20:21:13 2019 +0200
@@ -40,6 +40,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
+     **/
+    PostgreSQLDatabase::TransientAdvisoryLock lock(*db, 44 /* some arbitrary constant */);
+
     if (clearAll_)
     {
       db->ClearAll();
--- a/PostgreSQL/UnitTests/PostgreSQLTests.cpp	Fri Mar 01 12:56:13 2019 +0100
+++ b/PostgreSQL/UnitTests/PostgreSQLTests.cpp	Wed May 08 20:21:13 2019 +0200
@@ -513,3 +513,41 @@
   ASSERT_NE(r1.instanceId, r2.instanceId);
 }
 #endif
+
+
+TEST(PostgreSQL, Lock2)
+{
+  std::auto_ptr<PostgreSQLDatabase> db1(CreateTestDatabase());
+  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
+
+  {
+    std::auto_ptr<PostgreSQLDatabase> db2(CreateTestDatabase());
+    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));
+}