# HG changeset patch # User Sebastien Jodogne # Date 1615895023 -3600 # Node ID d494b4f1103e8a14fa9ac1df190922ca40ccd199 # Parent 60a860942c5e9be6520308d1091849f37d011abc removed the global database mutex from ServerIndex and StatelessDatabaseOperations diff -r 60a860942c5e -r d494b4f1103e OrthancServer/Plugins/Engine/OrthancPluginDatabase.cpp --- a/OrthancServer/Plugins/Engine/OrthancPluginDatabase.cpp Mon Mar 15 18:22:53 2021 +0100 +++ b/OrthancServer/Plugins/Engine/OrthancPluginDatabase.cpp Tue Mar 16 12:43:43 2021 +0100 @@ -60,10 +60,10 @@ typedef std::pair AnswerResource; typedef std::map AnswerMetadata; - OrthancPluginDatabase& that_; - IDatabaseListener& listener_; - - _OrthancPluginDatabaseAnswerType type_; + OrthancPluginDatabase& that_; + boost::recursive_mutex::scoped_lock lock_; + IDatabaseListener& listener_; + _OrthancPluginDatabaseAnswerType type_; std::list answerStrings_; std::list answerInt32_; @@ -228,6 +228,7 @@ explicit Transaction(OrthancPluginDatabase& that, IDatabaseListener& listener) : that_(that), + lock_(that.mutex_), listener_(listener), type_(_OrthancPluginDatabaseAnswerType_None), answerDoneIgnored_(false) @@ -1508,7 +1509,10 @@ void OrthancPluginDatabase::Open() { - CheckSuccess(backend_.open(payload_)); + { + boost::recursive_mutex::scoped_lock lock(mutex_); + CheckSuccess(backend_.open(payload_)); + } VoidDatabaseListener listener; @@ -1537,6 +1541,13 @@ } + void OrthancPluginDatabase::Close() + { + boost::recursive_mutex::scoped_lock lock(mutex_); + CheckSuccess(backend_.close(payload_)); + } + + IDatabaseWrapper::ITransaction* OrthancPluginDatabase::StartTransaction(TransactionType type, IDatabaseListener& listener) { @@ -1596,9 +1607,15 @@ void OrthancPluginDatabase::AnswerReceived(const _OrthancPluginDatabaseAnswer& answer) { + boost::recursive_mutex::scoped_lock lock(mutex_); + if (activeTransaction_ != NULL) { activeTransaction_->AnswerReceived(answer); } + else + { + LOG(WARNING) << "Received an answer from the database index plugin, but not transaction is active"; + } } } diff -r 60a860942c5e -r d494b4f1103e OrthancServer/Plugins/Engine/OrthancPluginDatabase.h --- a/OrthancServer/Plugins/Engine/OrthancPluginDatabase.h Mon Mar 15 18:22:53 2021 +0100 +++ b/OrthancServer/Plugins/Engine/OrthancPluginDatabase.h Tue Mar 16 12:43:43 2021 +0100 @@ -44,13 +44,34 @@ #include "../Include/orthanc/OrthancCDatabasePlugin.h" #include "PluginsErrorDictionary.h" +#include + namespace Orthanc { + /** + * This class is for backward compatibility with database plugins + * that don't use the primitives introduced in Orthanc 1.10.0 to + * deal with concurrent read-only transactions. + * + * In Orthanc <= 1.9.1, Orthanc assumed that at most 1 single thread + * was accessing the database plugin at anytime, in order to match + * the SQLite model. Read-write accesses assumed the plugin to run + * the SQL statement "START TRANSACTION SERIALIZABLE" so as to be + * able to rollback the modifications. Read-only accesses didn't + * start a transaction, as they were protected by the global mutex. + **/ class OrthancPluginDatabase : public IDatabaseWrapper { private: class Transaction; + /** + * We need a "recursive_mutex" because of "AnswerReceived()" that + * is called by the "answer" primitives of the database SDK once a + * transaction is running. + **/ + boost::recursive_mutex mutex_; + SharedLibrary& library_; PluginsErrorDictionary& errorDictionary_; OrthancPluginDatabaseBackend backend_; @@ -77,10 +98,7 @@ virtual void Open() ORTHANC_OVERRIDE; - virtual void Close() ORTHANC_OVERRIDE - { - CheckSuccess(backend_.close(payload_)); - } + virtual void Close() ORTHANC_OVERRIDE; const SharedLibrary& GetSharedLibrary() const { diff -r 60a860942c5e -r d494b4f1103e OrthancServer/Sources/Database/SQLiteDatabaseWrapper.cpp --- a/OrthancServer/Sources/Database/SQLiteDatabaseWrapper.cpp Mon Mar 15 18:22:53 2021 +0100 +++ b/OrthancServer/Sources/Database/SQLiteDatabaseWrapper.cpp Tue Mar 16 12:43:43 2021 +0100 @@ -303,14 +303,17 @@ } } - IDatabaseListener& listener_; - SignalRemainingAncestor& signalRemainingAncestor_; + boost::mutex::scoped_lock lock_; + IDatabaseListener& listener_; + SignalRemainingAncestor& signalRemainingAncestor_; public: - TransactionBase(SQLite::Connection& db, + TransactionBase(boost::mutex& mutex, + SQLite::Connection& db, IDatabaseListener& listener, SignalRemainingAncestor& signalRemainingAncestor) : UnitTestsTransaction(db), + lock_(mutex), listener_(listener), signalRemainingAncestor_(signalRemainingAncestor) { @@ -1161,7 +1164,7 @@ public: ReadWriteTransaction(SQLiteDatabaseWrapper& that, IDatabaseListener& listener) : - TransactionBase(that.db_, listener, *that.signalRemainingAncestor_), + TransactionBase(that.mutex_, that.db_, listener, *that.signalRemainingAncestor_), that_(that), transaction_(new SQLite::Transaction(that_.db_)) { @@ -1215,7 +1218,7 @@ public: ReadOnlyTransaction(SQLiteDatabaseWrapper& that, IDatabaseListener& listener) : - TransactionBase(that.db_, listener, *that.signalRemainingAncestor_), + TransactionBase(that.mutex_, that.db_, listener, *that.signalRemainingAncestor_), that_(that) { if (that_.activeTransaction_ != NULL) @@ -1274,27 +1277,31 @@ void SQLiteDatabaseWrapper::Open() { - if (signalRemainingAncestor_ != NULL) { - throw OrthancException(ErrorCode_BadSequenceOfCalls); // Cannot open twice - } - - signalRemainingAncestor_ = dynamic_cast(db_.Register(new SignalRemainingAncestor)); - db_.Register(new SignalFileDeleted(*this)); - db_.Register(new SignalResourceDeleted(*this)); - - db_.Execute("PRAGMA ENCODING=\"UTF-8\";"); + boost::mutex::scoped_lock lock(mutex_); - // Performance tuning of SQLite with PRAGMAs - // http://www.sqlite.org/pragma.html - db_.Execute("PRAGMA SYNCHRONOUS=NORMAL;"); - db_.Execute("PRAGMA JOURNAL_MODE=WAL;"); - db_.Execute("PRAGMA LOCKING_MODE=EXCLUSIVE;"); - db_.Execute("PRAGMA WAL_AUTOCHECKPOINT=1000;"); - //db_.Execute("PRAGMA TEMP_STORE=memory"); + if (signalRemainingAncestor_ != NULL) + { + throw OrthancException(ErrorCode_BadSequenceOfCalls); // Cannot open twice + } + + signalRemainingAncestor_ = dynamic_cast(db_.Register(new SignalRemainingAncestor)); + db_.Register(new SignalFileDeleted(*this)); + db_.Register(new SignalResourceDeleted(*this)); + + db_.Execute("PRAGMA ENCODING=\"UTF-8\";"); - // Make "LIKE" case-sensitive in SQLite - db_.Execute("PRAGMA case_sensitive_like = true;"); + // Performance tuning of SQLite with PRAGMAs + // http://www.sqlite.org/pragma.html + db_.Execute("PRAGMA SYNCHRONOUS=NORMAL;"); + db_.Execute("PRAGMA JOURNAL_MODE=WAL;"); + db_.Execute("PRAGMA LOCKING_MODE=EXCLUSIVE;"); + db_.Execute("PRAGMA WAL_AUTOCHECKPOINT=1000;"); + //db_.Execute("PRAGMA TEMP_STORE=memory"); + + // Make "LIKE" case-sensitive in SQLite + db_.Execute("PRAGMA case_sensitive_like = true;"); + } VoidDatabaseListener listener; @@ -1351,6 +1358,13 @@ } + void SQLiteDatabaseWrapper::Close() + { + boost::mutex::scoped_lock lock(mutex_); + db_.Close(); + } + + static void ExecuteUpgradeScript(SQLite::Connection& db, ServerResources::FileResourceId script) { @@ -1365,6 +1379,8 @@ void SQLiteDatabaseWrapper::Upgrade(unsigned int targetVersion, IStorageArea& storageArea) { + boost::mutex::scoped_lock lock(mutex_); + if (targetVersion != 6) { throw OrthancException(ErrorCode_IncompatibleDatabaseVersion); @@ -1441,6 +1457,13 @@ } + void SQLiteDatabaseWrapper::FlushToDisk() + { + boost::mutex::scoped_lock lock(mutex_); + db_.FlushToDisk(); + } + + int64_t SQLiteDatabaseWrapper::UnitTestsTransaction::CreateResource(const std::string& publicId, ResourceType type) { diff -r 60a860942c5e -r d494b4f1103e OrthancServer/Sources/Database/SQLiteDatabaseWrapper.h --- a/OrthancServer/Sources/Database/SQLiteDatabaseWrapper.h Mon Mar 15 18:22:53 2021 +0100 +++ b/OrthancServer/Sources/Database/SQLiteDatabaseWrapper.h Tue Mar 16 12:43:43 2021 +0100 @@ -37,6 +37,8 @@ #include "../../../OrthancFramework/Sources/SQLite/Connection.h" +#include + namespace Orthanc { /** @@ -55,10 +57,11 @@ class ReadWriteTransaction; class LookupFormatter; - SQLite::Connection db_; - TransactionBase* activeTransaction_; - SignalRemainingAncestor* signalRemainingAncestor_; - unsigned int version_; + boost::mutex mutex_; + SQLite::Connection db_; + TransactionBase* activeTransaction_; + SignalRemainingAncestor* signalRemainingAncestor_; + unsigned int version_; void GetChangesInternal(std::list& target, bool& done, @@ -79,19 +82,13 @@ virtual void Open() ORTHANC_OVERRIDE; - virtual void Close() ORTHANC_OVERRIDE - { - db_.Close(); - } + virtual void Close() ORTHANC_OVERRIDE; virtual IDatabaseWrapper::ITransaction* StartTransaction(TransactionType type, IDatabaseListener& listener) ORTHANC_OVERRIDE; - virtual void FlushToDisk() ORTHANC_OVERRIDE - { - db_.FlushToDisk(); - } + virtual void FlushToDisk() ORTHANC_OVERRIDE; virtual bool HasFlushToDisk() const ORTHANC_OVERRIDE { diff -r 60a860942c5e -r d494b4f1103e OrthancServer/Sources/Database/StatelessDatabaseOperations.cpp --- a/OrthancServer/Sources/Database/StatelessDatabaseOperations.cpp Mon Mar 15 18:22:53 2021 +0100 +++ b/OrthancServer/Sources/Database/StatelessDatabaseOperations.cpp Tue Mar 16 12:43:43 2021 +0100 @@ -605,14 +605,12 @@ { try { - boost::mutex::scoped_lock lock(databaseMutex_); // TODO - REMOVE - if (readOperations != NULL) { /** * IMPORTANT: In Orthanc <= 1.9.1, there was no transaction * in this case. This was OK because of the presence of the - * global mutex protecting the database. + * global mutex that was protecting the database. **/ Transaction transaction(db_, *factory_, TransactionType_ReadOnly); // TODO - Only if not "TransactionType_Implicit" @@ -682,8 +680,6 @@ void StatelessDatabaseOperations::FlushToDisk() { - boost::mutex::scoped_lock lock(databaseMutex_); - try { db_.FlushToDisk(); diff -r 60a860942c5e -r d494b4f1103e OrthancServer/Sources/Database/StatelessDatabaseOperations.h --- a/OrthancServer/Sources/Database/StatelessDatabaseOperations.h Mon Mar 15 18:22:53 2021 +0100 +++ b/OrthancServer/Sources/Database/StatelessDatabaseOperations.h Tue Mar 16 12:43:43 2021 +0100 @@ -38,7 +38,7 @@ #include "IDatabaseWrapper.h" #include "../DicomInstanceOrigin.h" -#include // TODO - REMOVE +#include namespace Orthanc @@ -86,6 +86,7 @@ { } + // WARNING: This method can be invoked from several threads concurrently virtual ITransactionContext* Create() = 0; }; @@ -405,7 +406,6 @@ class Transaction; IDatabaseWrapper& db_; - boost::mutex databaseMutex_; // TODO - REMOVE std::unique_ptr factory_; unsigned int maxRetries_; boost::shared_ptr mainDicomTagsRegistry_; // "shared_ptr" because of PImpl diff -r 60a860942c5e -r d494b4f1103e OrthancServer/Sources/ServerIndex.cpp --- a/OrthancServer/Sources/ServerIndex.cpp Mon Mar 15 18:22:53 2021 +0100 +++ b/OrthancServer/Sources/ServerIndex.cpp Tue Mar 16 12:43:43 2021 +0100 @@ -242,6 +242,8 @@ virtual ITransactionContext* Create() { + // There can be concurrent calls to this method, which is not an + // issue because we simply create an object return new TransactionContext(context_); } };