Mercurial > hg > orthanc

changeset 4594:d494b4f1103e db-changes

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
removed the global database mutex from ServerIndex and StatelessDatabaseOperations
author Sebastien Jodogne <s.jodogne@gmail.com>
date Tue, 16 Mar 2021 12:43:43 +0100
parents 60a860942c5e
children cc64385593ef
files OrthancServer/Plugins/Engine/OrthancPluginDatabase.cpp OrthancServer/Plugins/Engine/OrthancPluginDatabase.h OrthancServer/Sources/Database/SQLiteDatabaseWrapper.cpp OrthancServer/Sources/Database/SQLiteDatabaseWrapper.h OrthancServer/Sources/Database/StatelessDatabaseOperations.cpp OrthancServer/Sources/Database/StatelessDatabaseOperations.h OrthancServer/Sources/ServerIndex.cpp
diffstat 7 files changed, 104 insertions(+), 51 deletions(-) [+]
line wrap: on
line diff
--- 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<int64_t, ResourceType>     AnswerResource;
     typedef std::map<MetadataType, std::string>  AnswerMetadata;
 
-    OrthancPluginDatabase&  that_;
-    IDatabaseListener&      listener_;
-
-    _OrthancPluginDatabaseAnswerType type_;
+    OrthancPluginDatabase&               that_;
+    boost::recursive_mutex::scoped_lock  lock_;
+    IDatabaseListener&                   listener_;
+    _OrthancPluginDatabaseAnswerType     type_;
 
     std::list<std::string>         answerStrings_;
     std::list<int32_t>             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";
+    }
   }
 }
--- 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 <boost/thread/recursive_mutex.hpp>
+
 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
     {
--- 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<SignalRemainingAncestor*>(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<SignalRemainingAncestor*>(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)
   {
--- 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 <boost/thread/mutex.hpp>
+
 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<ServerIndexChange>& 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
     {
--- 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();
--- 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 <boost/thread/mutex.hpp>   // TODO - REMOVE
+#include <boost/shared_ptr.hpp>
 
 
 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<ITransactionContextFactory>  factory_;
     unsigned int                                 maxRetries_;
     boost::shared_ptr<MainDicomTagsRegistry>     mainDicomTagsRegistry_;  // "shared_ptr" because of PImpl
--- 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_);
     }
   };