# HG changeset patch # User Sebastien Jodogne # Date 1557339673 -7200 # Node ID cc3dc759c989dd9084c1f5fa7d57fd95a854a9b6 # Parent b1d7d255fb73448e0c82e21302e09e63358199c6 Added an advisory lock to avoid race conditions during database setup diff -r b1d7d255fb73 -r cc3dc759c989 Framework/PostgreSQL/PostgreSQLDatabase.cpp --- 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 #include +#include 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(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(lock) + ")"); + } + + + bool PostgreSQLDatabase::ReleaseAdvisoryLock(int32_t lock) + { + return RunAdvisoryLockStatement( + "select pg_advisory_unlock(" + + boost::lexical_cast(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_); + } } diff -r b1d7d255fb73 -r cc3dc759c989 Framework/PostgreSQL/PostgreSQLDatabase.h --- 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(); + }; }; } diff -r b1d7d255fb73 -r cc3dc759c989 PostgreSQL/NEWS --- 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) ======================== diff -r b1d7d255fb73 -r cc3dc759c989 PostgreSQL/Plugins/PostgreSQLIndex.cpp --- 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(); diff -r b1d7d255fb73 -r cc3dc759c989 PostgreSQL/Plugins/PostgreSQLStorageArea.cpp --- 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(); diff -r b1d7d255fb73 -r cc3dc759c989 PostgreSQL/UnitTests/PostgreSQLTests.cpp --- 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 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 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)); +}