Add a IDiscoverer wrapper.

This wrapper is responsible for stacking operations and running them in
a thread, rather than spawning a new std::thread each time

include/IDiscoverer.h

@@ -14,8 +14,8 @@ public:
     // We assume the media library will always outlive the discoverers.
     //FIXME: This is currently false since there is no way of interrupting
     //a discoverer thread
-    virtual bool discover( IMediaLibrary* ml, DBConnection dbConn, const std::string& entryPoint ) = 0;
-    virtual void reload( IMediaLibrary* ml, DBConnection dbConn ) = 0;
+    virtual bool discover( const std::string& entryPoint ) = 0;
+    virtual void reload() = 0;
 };
 
 #endif // IDISCOVERER_H

src/CMakeLists.txt

@@ -72,6 +72,7 @@ list(APPEND SRC_LIST ${HEADERS_LIST}
     filesystem/${ARCH_FOLDER}/File.cpp
 
     discoverer/FsDiscoverer.cpp
+    discoverer/DiscovererWorker.cpp
 
     Utils.cpp
 

src/MediaLibrary.cpp

@@ -4,6 +4,7 @@
 #include "AlbumTrack.h"
 #include "Artist.h"
 #include "AudioTrack.h"
+#include "discoverer/DiscovererWorker.h"
 #include "File.h"
 #include "Folder.h"
 #include "MediaLibrary.h"
@@ -49,6 +50,7 @@ const std::vector<std::string> MediaLibrary::supportedAudioExtensions {
 
 MediaLibrary::MediaLibrary()
     : m_parser( new Parser )
+    , m_discoverer( new DiscovererWorker )
 {
 }
 
@@ -107,8 +109,6 @@ bool MediaLibrary::initialize( const std::string& dbPath, const std::string& sna
         addMetadataService( std::move( thumbnailerService ) );
     }
 
-    m_discoverers.emplace_back( new FsDiscoverer( m_fsFactory ) );
-
     sqlite3* dbConnection;
     int res = sqlite3_open( dbPath.c_str(), &dbConnection );
     if ( res != SQLITE_OK )
@@ -134,7 +134,9 @@ bool MediaLibrary::initialize( const std::string& dbPath, const std::string& sna
         LOG_ERROR( "Failed to create database structure" );
         return false;
     }
-    reload();
+    m_discoverer->setCallback( m_callback );
+    m_discoverer->addDiscoverer( std::unique_ptr<IDiscoverer>( new FsDiscoverer( m_fsFactory, this, m_dbConnection ) ) );
+    m_discoverer->reload();
     return true;
 }
 
@@ -313,31 +315,12 @@ void MediaLibrary::addMetadataService(std::unique_ptr<IMetadataService> service)
 
 void MediaLibrary::reload()
 {
-    //FIXME: Create a proper wrapper to handle discoverer threading
-    std::thread t([this] {
-        //FIXME: This will crash if the media library gets deleted while we
-        //are discovering.
-        for ( auto& d : m_discoverers )
-            d->reload( this, this->m_dbConnection );
-    });
-    t.detach();
+    m_discoverer->reload();
 }
 
 void MediaLibrary::discover( const std::string &entryPoint )
 {
-    std::thread t([this, entryPoint] {
-        //FIXME: This will crash if the media library gets deleted while we
-        //are discovering.
-        if ( m_callback != nullptr )
-            m_callback->onDiscoveryStarted( entryPoint );
-
-        for ( auto& d : m_discoverers )
-            d->discover( this, this->m_dbConnection, entryPoint );
-
-        if ( m_callback != nullptr )
-            m_callback->onDiscoveryCompleted( entryPoint );
-    });
-    t.detach();
+    m_discoverer->discover( entryPoint );
 }
 
 const std::string& MediaLibrary::snapshotPath() const

src/MediaLibrary.h

@@ -2,6 +2,7 @@
 #define MEDIALIBRARY_H
 
 class Parser;
+class DiscovererWorker;
 
 #include <sqlite3.h>
 
@@ -63,7 +64,6 @@ class MediaLibrary : public IMediaLibrary
     private:
         std::shared_ptr<sqlite3> m_dbConnection;
         std::shared_ptr<factory::IFileSystem> m_fsFactory;
-        std::vector<std::unique_ptr<IDiscoverer>> m_discoverers;
         std::string m_snapshotPath;
         IMediaLibraryCb* m_callback;
 
@@ -77,5 +77,8 @@ class MediaLibrary : public IMediaLibrary
         // likely to require a valid MediaLibrary, which would be compromised if some fields have already been
         // deleted/destroyed.
         std::unique_ptr<Parser> m_parser;
+        // Same reasoning applies here.
+        //FIXME: Having to maintain a specific ordering sucks, let's use shared_ptr or something
+        std::unique_ptr<DiscovererWorker> m_discoverer;
 };
 #endif // MEDIALIBRARY_H

src/discoverer/DiscovererWorker.cpp

+#include "DiscovererWorker.h"
+
+#include "logging/Logger.h"
+
+DiscovererWorker::DiscovererWorker()
+    : m_run( true )
+{
+}
+
+DiscovererWorker::~DiscovererWorker()
+{
+    {
+        std::unique_lock<std::mutex> lock( m_mutex );
+        while ( m_entryPoints.empty() == false )
+            m_entryPoints.pop();
+        m_run = false;
+    }
+    m_cond.notify_all();
+    m_thread.join();
+}
+
+void DiscovererWorker::addDiscoverer( std::unique_ptr<IDiscoverer> discoverer )
+{
+    m_discoverers.push_back( std::move( discoverer ) );
+}
+
+void DiscovererWorker::setCallback(IMediaLibraryCb* cb)
+{
+    m_cb = cb;
+}
+
+bool DiscovererWorker::discover( const std::string& entryPoint )
+{
+    if ( entryPoint.length() == 0 )
+        return false;
+    enqueue( entryPoint );
+    return true;
+}
+
+void DiscovererWorker::reload()
+{
+    enqueue( "" );
+}
+
+void DiscovererWorker::enqueue( const std::string& entryPoint )
+{
+    std::unique_lock<std::mutex> lock( m_mutex );
+
+    m_entryPoints.emplace( entryPoint );
+    if ( m_thread.get_id() == std::thread::id{} )
+        m_thread = std::thread( &DiscovererWorker::run, this );
+    // Since we just added an element, let's not check for size == 0 :)
+    else if ( m_entryPoints.size() == 1 )
+        m_cond.notify_all();
+}
+
+void DiscovererWorker::run()
+{
+    while ( m_run == true )
+    {
+        std::string entryPoint;
+        {
+            std::unique_lock<std::mutex> lock( m_mutex );
+            if ( m_entryPoints.size() == 0 )
+            {
+                m_cond.wait( lock, [this]() { return m_entryPoints.size() > 0 || m_run == false ; } );
+                if ( m_run == false )
+                    break;
+            }
+            entryPoint = m_entryPoints.front();
+            m_entryPoints.pop();
+        }
+        if ( entryPoint.length() > 0 )
+        {
+            if ( m_cb != nullptr )
+                m_cb->onDiscoveryStarted( entryPoint );
+            for ( auto& d : m_discoverers )
+            {
+                // Assume only one discoverer can handle an entrypoint.
+                if ( d->discover( entryPoint ) == true )
+                    break;
+                if ( m_run == false )
+                    break;
+            }
+            if ( m_cb != nullptr )
+                m_cb->onDiscoveryCompleted( entryPoint );
+        }
+        else
+        {
+            for ( auto& d : m_discoverers )
+            {
+                d->reload();
+                if ( m_run == false )
+                    break;
+            }
+        }
+    }
+    LOG_INFO( "Exiting DiscovererWorker thread" );
+}
+

src/discoverer/DiscovererWorker.h

+#pragma once
+
+#include <atomic>
+#include <condition_variable>
+#include <memory>
+#include <mutex>
+#include <queue>
+#include <string>
+#include <thread>
+#include <vector>
+
+#include "IDiscoverer.h"
+
+class DiscovererWorker : public IDiscoverer
+{
+public:
+    DiscovererWorker();
+    virtual ~DiscovererWorker();
+    void addDiscoverer( std::unique_ptr<IDiscoverer> discoverer );
+    void setCallback( IMediaLibraryCb* cb );
+
+    virtual bool discover( const std::string& entryPoint ) override;
+    virtual void reload() override;
+
+private:
+    void enqueue( const std::string& entryPoint );
+    void run();
+
+private:
+    std::thread m_thread;
+    std::queue<std::string> m_entryPoints;
+    std::mutex m_mutex;
+    std::condition_variable m_cond;
+    std::atomic_bool m_run;
+    std::vector<std::unique_ptr<IDiscoverer>> m_discoverers;
+    IMediaLibraryCb* m_cb;
+};
+

src/discoverer/FsDiscoverer.cpp

@@ -7,7 +7,9 @@
 #include <queue>
 #include <iostream>
 
-FsDiscoverer::FsDiscoverer(std::shared_ptr<factory::IFileSystem> fsFactory)
+FsDiscoverer::FsDiscoverer( std::shared_ptr<factory::IFileSystem> fsFactory, IMediaLibrary* ml, DBConnection dbConn )
+    : m_ml( ml )
+    , m_dbConn( dbConn )
 {
     if ( fsFactory != nullptr )
         m_fsFactory = fsFactory;
@@ -15,14 +17,14 @@ FsDiscoverer::FsDiscoverer(std::shared_ptr<factory::IFileSystem> fsFactory)
         m_fsFactory.reset( new factory::FileSystemDefaultFactory );
 }
 
-bool FsDiscoverer::discover( IMediaLibrary* ml, DBConnection dbConn, const std::string &entryPoint )
+bool FsDiscoverer::discover( const std::string &entryPoint )
 {
     // Assume :// denotes a scheme that isn't a file path, and refuse to discover it.
     if ( entryPoint.find( "://" ) != std::string::npos )
         return false;
 
     {
-        auto f = Folder::fetch( dbConn, entryPoint );
+        auto f = Folder::fetch( m_dbConn, entryPoint );
         // If the folder exists, we assume it is up to date
         if ( f != nullptr )
             return true;
@@ -38,31 +40,31 @@ bool FsDiscoverer::discover( IMediaLibrary* ml, DBConnection dbConn, const std::
         LOG_ERROR("Failed to create an IDirectory for ", entryPoint, ": ", ex.what());
         return false;
     }
-    auto f = Folder::create( dbConn, fsDir.get(), 0 );
+    auto f = Folder::create( m_dbConn, fsDir.get(), 0 );
     if ( f == nullptr )
         return false;
-    checkSubfolders( ml, dbConn, fsDir.get(), f );
+    checkSubfolders( fsDir.get(), f );
     return true;
 }
 
-void FsDiscoverer::reload( IMediaLibrary *ml, DBConnection dbConn )
+void FsDiscoverer::reload()
 {
     //FIXME: This should probably be in a sql transaction
     //FIXME: This shouldn't be done for "removable"/network files
     static const std::string req = "SELECT * FROM " + policy::FolderTable::Name
             + " WHERE id_parent IS NULL";
-    auto rootFolders = sqlite::Tools::fetchAll<Folder, IFolder>( dbConn, req );
+    auto rootFolders = sqlite::Tools::fetchAll<Folder, IFolder>( m_dbConn, req );
     for ( const auto f : rootFolders )
     {
         auto folder = m_fsFactory->createDirectory( f->path() );
         if ( folder->lastModificationDate() == f->lastModificationDate() )
             continue;
-        checkSubfolders( ml, dbConn, folder.get(), f );
+        checkSubfolders( folder.get(), f );
         f->setLastModificationDate( folder->lastModificationDate() );
     }
 }
 
-bool FsDiscoverer::checkSubfolders( IMediaLibrary* ml, DBConnection dbConn, fs::IDirectory* folder, FolderPtr parentFolder )
+bool FsDiscoverer::checkSubfolders( fs::IDirectory* folder, FolderPtr parentFolder )
 {
     // From here we can have:
     // - New subfolder(s)
@@ -75,7 +77,7 @@ bool FsDiscoverer::checkSubfolders( IMediaLibrary* ml, DBConnection dbConn, fs::
     // Load the folders we already know of:
     static const std::string req = "SELECT * FROM " + policy::FolderTable::Name
             + " WHERE id_parent = ?";
-    auto subFoldersInDB = sqlite::Tools::fetchAll<Folder, IFolder>( dbConn, req, parentFolder->id() );
+    auto subFoldersInDB = sqlite::Tools::fetchAll<Folder, IFolder>( m_dbConn, req, parentFolder->id() );
     for ( const auto& subFolderPath : folder->dirs() )
     {
         auto it = std::find_if( begin( subFoldersInDB ), end( subFoldersInDB ), [subFolderPath](const std::shared_ptr<IFolder>& f) {
@@ -99,25 +101,25 @@ bool FsDiscoverer::checkSubfolders( IMediaLibrary* ml, DBConnection dbConn, fs::
             continue;
         }
         // This folder was modified, let's recurse
-        checkSubfolders( ml, dbConn, subFolder.get(), *it );
-        checkFiles( ml, dbConn, subFolder.get(), *it );
+        checkSubfolders( subFolder.get(), *it );
+        checkFiles( subFolder.get(), *it );
         (*it)->setLastModificationDate( subFolder->lastModificationDate() );
         subFoldersInDB.erase( it );
     }
     // Now all folders we had in DB but haven't seen from the FS must have been deleted.
     for ( auto f : subFoldersInDB )
     {
-        std::cout << "Folder " << f->path() << " not found in FS, deleting it" << std::endl;
-        ml->deleteFolder( f );
+        LOG_INFO( "Folder ", f->path(), " not found in FS, deleting it" );
+        m_ml->deleteFolder( f );
     }
     return true;
 }
 
-void FsDiscoverer::checkFiles( IMediaLibrary *ml, DBConnection dbConn, fs::IDirectory* folder, FolderPtr parentFolder )
+void FsDiscoverer::checkFiles( fs::IDirectory* folder, FolderPtr parentFolder )
 {
     static const std::string req = "SELECT * FROM " + policy::FileTable::Name
             + " WHERE folder_id = ?";
-    auto files = sqlite::Tools::fetchAll<File, IFile>( dbConn, req, parentFolder->id() );
+    auto files = sqlite::Tools::fetchAll<File, IFile>( m_dbConn, req, parentFolder->id() );
     for ( const auto& filePath : folder->files() )
     {        
         auto it = std::find_if( begin( files ), end( files ), [filePath](const std::shared_ptr<IFile>& f) {
@@ -125,7 +127,7 @@ void FsDiscoverer::checkFiles( IMediaLibrary *ml, DBConnection dbConn, fs::IDire
         });
         if ( it == end( files ) )
         {
-            ml->addFile( filePath, parentFolder );
+            m_ml->addFile( filePath, parentFolder );
             continue;
         }
         auto file = m_fsFactory->createFile( filePath );
@@ -135,12 +137,12 @@ void FsDiscoverer::checkFiles( IMediaLibrary *ml, DBConnection dbConn, fs::IDire
             files.erase( it );
             continue;
         }
-        ml->deleteFile( filePath );
-        ml->addFile( filePath, parentFolder );
+        m_ml->deleteFile( filePath );
+        m_ml->addFile( filePath, parentFolder );
         files.erase( it );
     }
     for ( auto file : files )
     {
-        ml->deleteFile( file );
+        m_ml->deleteFile( file );
     }
 }

src/discoverer/FsDiscoverer.h

@@ -9,15 +9,17 @@
 class FsDiscoverer : public IDiscoverer
 {
 public:
-    FsDiscoverer( std::shared_ptr<factory::IFileSystem> fsFactory );
-    virtual bool discover(IMediaLibrary *ml, DBConnection dbConn, const std::string &entryPoint ) override;
-    virtual void reload( IMediaLibrary* ml, DBConnection dbConn ) override;
+    FsDiscoverer( std::shared_ptr<factory::IFileSystem> fsFactory, IMediaLibrary *ml, DBConnection dbConn );
+    virtual bool discover(const std::string &entryPoint ) override;
+    virtual void reload() override;
 
 private:
-    bool checkSubfolders(IMediaLibrary *ml, DBConnection dbConn, fs::IDirectory *folder, FolderPtr parentFolder );
-    void checkFiles(IMediaLibrary *ml, DBConnection dbConn, fs::IDirectory *folder, FolderPtr parentFolder );
+    bool checkSubfolders( fs::IDirectory *folder, FolderPtr parentFolder );
+    void checkFiles( fs::IDirectory *folder, FolderPtr parentFolder );
 
 private:
+    IMediaLibrary* m_ml;
+    DBConnection m_dbConn;
     std::shared_ptr<factory::IFileSystem> m_fsFactory;
 };