Merge pull request #655 from Kicer86/crash_fix

Fix crash in PhotosAnalyzer when closing project during analysis

src/database/database_tools/implementation/photos_analyzer.cpp

@@ -33,6 +33,7 @@
 using namespace std::chrono_literals;
 using namespace std::placeholders;
 using namespace PhotosAnalyzerConsts;
+using namespace Qt::Literals::StringLiterals;
 
 
 namespace
@@ -196,13 +197,15 @@ namespace
 PhotosAnalyzerImpl::PhotosAnalyzerImpl(ICoreFactoryAccessor* coreFactory, Database::IDatabase& database):
     m_taskQueue(coreFactory->getTaskExecutor()),
     m_mediaInformation(coreFactory),
-    m_database(database),
+    m_database(database.attach(u"Photos Analyzer"_s)),
     m_tasksView(nullptr),
     m_viewTask(nullptr)
 {
     //check for not fully initialized photos in database
     //TODO: use independent updaters here (issue #102)
 
+    m_database->onClose(std::bind(&PhotosAnalyzerImpl::stop, this));
+
     // GeometryLoaded < 1
     Database::FilterPhotosWithFlags geometryFilter;
     geometryFilter.comparison[Photo::FlagsE::GeometryLoaded] = Database::ComparisonOp::Less;
@@ -244,7 +247,7 @@ PhotosAnalyzerImpl::PhotosAnalyzerImpl(ICoreFactoryAccessor* coreFactory, Databa
     Database::GroupFilter filters = {noExifOrGeometryFilter, noPhashFilterGroup};
     filters.mode = Database::LogicalOp::Or;
 
-    m_database.exec([this, filters](Database::IBackend& backend)
+    m_database->db().exec([this, filters](Database::IBackend& backend)
     {
         auto photos = backend.photoOperator().getPhotos(filters);
 
@@ -297,7 +300,7 @@ void PhotosAnalyzerImpl::addPhotos(const std::vector<Photo::Id>& ids)
         // Construct DatabaseQueue as a shared pointer used to be passed to all tasks.
         // When last task is done, custom destructor below will do all necessary cleanups.
         storage = std::shared_ptr<Database::DatabaseQueue>(
-            new Database::DatabaseQueue(m_database),
+            new Database::DatabaseQueue(m_database->db()),
             [this, _ = std::move(storageLock)](Database::DatabaseQueue* queue)
         {
             // This lambda may be called from ~PhotosAnalyzerImpl as a result of stop().
@@ -335,6 +338,8 @@ void PhotosAnalyzerImpl::stop()
 
     // make sure storage is unlocked
     std::lock_guard _(m_storageMutex);
+
+    m_database.reset();
 }
 
 
@@ -348,7 +353,6 @@ void PhotosAnalyzerImpl::finishProgressBar()
 }
 
 
-
 ///////////////////////////////////////////////////////////////////////////////
 
 

src/database/database_tools/implementation/photos_analyzer_p.hpp

@@ -53,7 +53,7 @@ class PhotosAnalyzerImpl: public QObject
         QMetaObject::Connection m_backendConnection;
         std::weak_ptr<Database::DatabaseQueue> m_storageQueue;
         std::mutex m_storageMutex;
-        Database::IDatabase& m_database;
+        std::unique_ptr<Database::IClient> m_database;
         ITasksView* m_tasksView;
         IViewTask* m_viewTask;
         int m_maxPhotos;

src/database/idatabase.hpp

@@ -91,7 +91,7 @@ namespace Database
          *
          * @return object blocking database's destruction
          */
-        [[nodiscard]] virtual std::unique_ptr<IClient> attach(const QString &) = 0;
+        [[nodiscard]] virtual std::unique_ptr<IClient> attach(QStringView) = 0;
 
         struct ITask
         {

src/database/implementation/async_database.cpp

@@ -120,8 +120,9 @@ namespace Database
     ///////////////////////////////////////////////////////////////////////////
 
 
-    AsyncDatabase::Client::Client(AsyncDatabase& _db)
-        : m_db(_db)
+    AsyncDatabase::Client::Client(AsyncDatabase& _db, QStringView name)
+        : m_name(name.toString())
+        , m_db(_db)
     {
 
     }
@@ -152,6 +153,11 @@ namespace Database
     }
 
 
+    const QString& AsyncDatabase::Client::name() const
+    {
+        return m_name;
+    }
+
     ///////////////////////////////////////////////////////////////////////////
 
 
@@ -198,22 +204,21 @@ namespace Database
     void AsyncDatabase::close()
     {
         // close clients
-        std::unique_lock lk(m_clientsMutex);
         m_acceptClients = false;
 
         sendOnCloseNotification();
-        waitForClients(lk);
+        waitForClients();
 
         // finish tasks
         stopExecutor();
     }
 
 
-    std::unique_ptr<IClient> AsyncDatabase::attach(const QString& /*name*/)
+    std::unique_ptr<IClient> AsyncDatabase::attach(QStringView name)
     {
         if (m_acceptClients)
         {
-            auto observer = std::make_unique<Client>(*this);
+            auto observer = std::make_unique<Client>(*this, name);
 
             std::lock_guard _(m_clientsMutex);
             m_clients.insert(observer.get());
@@ -241,16 +246,30 @@ namespace Database
 
     void AsyncDatabase::sendOnCloseNotification()
     {
-        assert(m_clientsMutex.try_lock() == false);     // m_clientsMutex should be locked by caller
+        assert(m_acceptClients == false);
 
-        for(auto& client: m_clients)
+        std::unique_lock lk(m_clientsMutex);
+
+        // client may be removed during the loop below, so work on a copy
+        const auto clients = m_clients;
+
+        for(auto& client: clients)
+        {
+            m_logger->debug("Sending close notification to " + client->name());
             client->callOnClose();
+        }
     }
 
 
-    void AsyncDatabase::waitForClients(std::unique_lock<std::mutex>& clientsLock)
+    void AsyncDatabase::waitForClients()
     {
-        m_clientsChangeCV.wait(clientsLock, [this]()
+        std::unique_lock lk(m_clientsMutex);
+
+        m_logger->debug("Active clients:");
+        for(auto& client: m_clients)
+            m_logger->debug(client->name());
+
+        m_clientsChangeCV.wait(lk, [this]()
         {
             return m_clients.empty();
         });

src/database/implementation/async_database.hpp

@@ -53,40 +53,42 @@ namespace Database
             virtual void init(const ProjectInfo &, const Callback<const BackendStatus &> &) override;
             virtual void close() override;
 
-            std::unique_ptr<IClient> attach(const QString &) override;
+            std::unique_ptr<IClient> attach(QStringView name) override;
 
         private:
             class Client: public IClient
             {
             public:
-                explicit Client(AsyncDatabase &);
+                explicit Client(AsyncDatabase &, QStringView);
                 ~Client() override;
 
                 IDatabase& db() override;
                 void onClose(const std::function<void()> &) override;
 
                 void callOnClose() const;
+                const QString& name() const;
 
             private:
+                const QString m_name;
                 std::function<void()> m_onClose;
                 AsyncDatabase& m_db;
             };
 
             friend class Client;
 
             std::set<Client *> m_clients;
-            std::mutex m_clientsMutex;
+            std::recursive_mutex m_clientsMutex;
             std::unique_ptr<ILogger> m_logger;
             std::unique_ptr<IBackend> m_backend;
             std::unique_ptr<Executor> m_executor;
-            std::condition_variable m_clientsChangeCV;
+            std::condition_variable_any m_clientsChangeCV;
             std::thread m_thread;
             bool m_acceptClients = true;
             bool m_working;
 
             void addTask(std::unique_ptr<IDatabase::ITask> &&);
             void sendOnCloseNotification();
-            void waitForClients(std::unique_lock<std::mutex> &);
+            void waitForClients();
             void stopExecutor();
             void remove(Client *);
     };

src/gui/desktop/models/series_model.cpp

@@ -16,6 +16,7 @@
 
 
 using namespace std::placeholders;
+using namespace Qt::Literals::StringLiterals;
 
 
 ENUM_ROLES_SETUP(SeriesModel::Roles);
@@ -101,7 +102,7 @@ void SeriesModel::loadData(const std::stop_token& stopToken, StoppableTaskCallba
 {
     runOn(
         m_core->getTaskExecutor(),
-        [core = m_core, logger = m_logger->subLogger("SeriesDetector"), dbClient = m_project->getDatabase().attach("SeriesDetector"), callback, stopToken]() mutable
+        [core = m_core, logger = m_logger->subLogger("SeriesDetector"), dbClient = m_project->getDatabase().attach(u"SeriesDetector"_s), callback, stopToken]() mutable
         {
             IExifReaderFactory& exif = core->getExifReaderFactory();
 

src/gui/desktop/ui/mainwindow.cpp

@@ -246,8 +246,6 @@ void MainWindow::closeProject()
 {
     if (m_currentPrj)
     {
-        m_currentPrj->closeDatabase();
-
         // Move m_currentPrj to a temporary place, so m_currentPrj is null and all tools will change theirs state basing on this.
         // Project object will be destroyed at the end of this routine
         auto prj = std::move(m_currentPrj);
@@ -257,6 +255,7 @@ void MainWindow::closeProject()
 
         updateGui();
 
+        prj->closeDatabase();
         QDir::setSearchPaths("prj", QStringList() );
     }
 }

src/gui/desktop/utils/batch_face_detector.cpp

@@ -10,6 +10,8 @@
 
 #include <iostream>
 
+using namespace Qt::Literals::StringLiterals;
+
 
 BatchFaceDetector::BatchFaceDetector()
     : m_faceEditor(make_lazy_ptr<FaceEditor>([this]{ return std::make_unique<FaceEditor>(*this->db(), *this->core(), *this->m_logger); }))
@@ -65,6 +67,7 @@ void BatchFaceDetector::setCore(ICoreFactoryAccessor* core)
 void BatchFaceDetector::setDB(Database::IDatabase* db)
 {
     m_dbClient = db->attach(tr("Batch face detector"));
+
     if (m_dbClient)
     {
         m_dbClient->onClose([this]()

src/gui/desktop/utils/groups_manager.cpp

@@ -71,6 +71,7 @@ void GroupsManager::group(Database::IDatabase& database, QPromise<void>&& promis
     database.exec([groups, db_promise = std::move(promise)](Database::IBackend& backend) mutable
     {
         const std::size_t groupSize = groups.size();
+        assert(groupSize > 0);
 
         db_promise.start();
         db_promise.setProgressRange(0, groupSize - 1);

src/gui/desktop/utils/people_editor.cpp

@@ -37,6 +37,7 @@
 
 
 using namespace Database::CommonGeneralFlags;
+using namespace Qt::Literals::StringLiterals;
 
 
 namespace
@@ -171,8 +172,6 @@ class Recognizer: public IRecognizePerson
 
         void fetchPeopleAndFingerprints(Database::IDatabase& db)
         {
-            typedef std::tuple<std::vector<Person::Fingerprint>, std::vector<Person::Id>> Result;
-
             evaluate(db, [this](Database::IBackend& backend)
             {
                 std::vector<Person::Fingerprint> people_fingerprints;
@@ -217,7 +216,7 @@ std::vector<std::unique_ptr<IFace>> FaceEditor::getFacesFor(const Photo::Id& id)
 
     auto faces = findFaces(*image, id);
 
-    std::shared_ptr<Database::IClient> dbClient = m_db.attach("FaceEditor");
+    std::shared_ptr<Database::IClient> dbClient = m_db.attach(u"FaceEditor"_s);
     std::vector<std::unique_ptr<IFace>> result;
 
     std::ranges::transform(faces.get<Photo::Field::People>(), std::back_inserter(result), [id = faces.getId(), &image, &dbClient, recognizer = m_recognizer](const auto& personData)

src/unit_tests_utils/mock_database.hpp

@@ -12,7 +12,7 @@ struct DatabaseMock: Database::IDatabase
 {
     MOCK_METHOD(Database::IBackend&, backend, (), (override));
     MOCK_METHOD(void, execute, (std::unique_ptr<Database::IDatabase::ITask> &&), (override));
-    MOCK_METHOD(std::unique_ptr<Database::IClient>, attach, (const QString &), (override));
+    MOCK_METHOD(std::unique_ptr<Database::IClient>, attach, (QStringView), (override));
 };
 
 #endif