The ctkCmdLineModuleManager is now thread-safe.

The directory watcher now concurrently adds new modules to the manager.

Libs/CommandLineModules/Core/ctkCmdLineModuleBackend.h

@@ -40,6 +40,15 @@ struct CTK_CMDLINEMODULECORE_EXPORT ctkCmdLineModuleBackend
 
   virtual QList<QString> schemes() const = 0;
 
+  /**
+   * @brief Get the XML parameter description from the given location.
+   * @param location The location URL specifying the module.
+   * @return The raw XML parameter description.
+   *
+   * This method may be concurrently called by the ctkCmdLineModuleManager and
+   * must be thread-safe.
+   *
+   */
   virtual QByteArray rawXmlDescription(const QUrl& location) = 0;
 
 protected:

Libs/CommandLineModules/Core/ctkCmdLineModuleDirectoryWatcher.cpp

@@ -30,10 +30,78 @@
 #include <QFileInfo>
 #include <QUrl>
 #include <QDebug>
+#include <QtConcurrentMap>
 
 #include <iostream>
 
 //-----------------------------------------------------------------------------
+// A function object for concurrently adding modules
+namespace {
+struct AddModule
+{
+  typedef ctkCmdLineModuleReference result_type;
+
+  AddModule(ctkCmdLineModuleManager* manager, bool debug = false)
+    : ModuleManager(manager), Debug(debug)
+  {}
+
+  ctkCmdLineModuleReference operator()(const QString& moduleLocation)
+  {
+    try
+    {
+      return this->ModuleManager->registerModule(QUrl::fromLocalFile(moduleLocation));
+    }
+    catch (const ctkException& e)
+    {
+      if (this->Debug)
+      {
+        qDebug() << e;
+      }
+      return ctkCmdLineModuleReference();
+    }
+    catch (...)
+    {
+      if (this->Debug)
+      {
+        qDebug() << "Registering module" << moduleLocation << "failed with an unknown exception.";
+      }
+      return ctkCmdLineModuleReference();
+    }
+  }
+
+  ctkCmdLineModuleManager* ModuleManager;
+  bool Debug;
+};
+}
+
+//-----------------------------------------------------------------------------
+// A function object for concurrently removing modules
+namespace {
+struct RemoveModule
+{
+  typedef bool result_type;
+
+  RemoveModule(ctkCmdLineModuleManager* manager)
+    : ModuleManager(manager)
+  {}
+
+  bool operator()(const QString& moduleLocation)
+  {
+    ctkCmdLineModuleReference ref = this->ModuleManager->moduleReference(QUrl::fromLocalFile(moduleLocation));
+    if (ref)
+    {
+      this->ModuleManager->unregisterModule(ref);
+      return true;
+    }
+    return false;
+  }
+
+  ctkCmdLineModuleManager* ModuleManager;
+};
+}
+
+
+//-----------------------------------------------------------------------------
 // ctkCmdLineModuleDirectoryWatcher methods
 
 //-----------------------------------------------------------------------------
@@ -240,6 +308,9 @@ void ctkCmdLineModuleDirectoryWatcherPrivate::setModuleReferences(const QStringL
   QString path;
   QStringList currentlyWatchedDirectories = this->directories();
 
+  QStringList modulesToUnload;
+  QStringList modulesToLoad;
+
   // First remove modules from current directories that are no longer in the requested "directories" list.
   foreach (path, currentlyWatchedDirectories)
   {
@@ -250,7 +321,7 @@ void ctkCmdLineModuleDirectoryWatcherPrivate::setModuleReferences(const QStringL
       QString filename;
       foreach (filename, currentlyWatchedFiles)
       {
-        this->unloadModule(filename);
+        modulesToUnload << filename;
       }
     }
   }
@@ -269,7 +340,7 @@ void ctkCmdLineModuleDirectoryWatcherPrivate::setModuleReferences(const QStringL
       {
         if (!executablesInDirectory.contains(executable))
         {
-          this->unloadModule(executable);
+          modulesToUnload << executable;
         }
       }
 
@@ -277,7 +348,7 @@ void ctkCmdLineModuleDirectoryWatcherPrivate::setModuleReferences(const QStringL
       {
         if (!currentlyWatchedFiles.contains(executable))
         {
-          this->loadModule(executable);
+          modulesToLoad << executable;
         }
       }
     }
@@ -289,10 +360,13 @@ void ctkCmdLineModuleDirectoryWatcherPrivate::setModuleReferences(const QStringL
       QString executable;
       foreach (executable, executables)
       {
-        this->loadModule(executable);
+        modulesToLoad << executable;
       }
     }
   }
+
+  this->unloadModules(modulesToUnload);
+  this->loadModules(modulesToLoad);
 }
 
 
@@ -312,41 +386,28 @@ void ctkCmdLineModuleDirectoryWatcherPrivate::updateModuleReferences(const QStri
 
 
 //-----------------------------------------------------------------------------
-ctkCmdLineModuleReference ctkCmdLineModuleDirectoryWatcherPrivate::loadModule(const QString& pathToExecutable)
+QList<ctkCmdLineModuleReference> ctkCmdLineModuleDirectoryWatcherPrivate::loadModules(const QStringList& executables)
 {
-  ctkCmdLineModuleReference ref;
-  try
-  {
-    ref = this->ModuleManager->registerModule(QUrl::fromLocalFile(pathToExecutable));
-  }
-  catch (const ctkIllegalStateException& e)
-  {
-    e.rethrow();
-  }
-  catch (const ctkException& e)
+  QList<ctkCmdLineModuleReference> refs = QtConcurrent::blockingMapped(executables, AddModule(this->ModuleManager, this->Debug));
+
+  for (int i = 0; i < executables.size(); ++i)
   {
-    if (this->Debug)
+    if (refs[i])
     {
-      qWarning() << e;
+      this->MapFileNameToReference[executables[i]] = refs[i];
     }
   }
-
-  if (ref)
-  {
-    this->MapFileNameToReference[pathToExecutable] = ref;
-  }
-  return ref;
+  return refs;
 }
 
 
 //-----------------------------------------------------------------------------
-void ctkCmdLineModuleDirectoryWatcherPrivate::unloadModule(const QString& pathToExecutable)
+void ctkCmdLineModuleDirectoryWatcherPrivate::unloadModules(const QStringList& executables)
 {
-  ctkCmdLineModuleReference ref = this->ModuleManager->moduleReference(QUrl::fromLocalFile(pathToExecutable));
-  if (ref)
+  QtConcurrent::blockingMapped(executables, RemoveModule(this->ModuleManager));
+  foreach(QString executable, executables)
   {
-    this->ModuleManager->unregisterModule(ref);
-    this->MapFileNameToReference.remove(pathToExecutable);
+    this->MapFileNameToReference.remove(executable);
   }
 }
 
@@ -354,7 +415,7 @@ void ctkCmdLineModuleDirectoryWatcherPrivate::unloadModule(const QString& pathTo
 //-----------------------------------------------------------------------------
 void ctkCmdLineModuleDirectoryWatcherPrivate::onFileChanged(const QString& path)
 {
-  ctkCmdLineModuleReference ref = this->loadModule(path);
+  ctkCmdLineModuleReference ref = this->loadModules(QStringList() << path).front();
   if (ref)
   {
     if (this->Debug) qDebug() << "Reloaded " << path;

Libs/CommandLineModules/Core/ctkCmdLineModuleDirectoryWatcher_p.h

@@ -124,17 +124,17 @@ private:
   void updateModuleReferences(const QString &directory);
 
   /**
-   * \brief Uses the ctkCmdLineModuleManager to try and add the executable to the list
+   * \brief Uses the ctkCmdLineModuleManager to try and add the executables to the list
    * of executables, and if successful it is added to this->MapFileNameToReference.
-   * \param pathToExecutable path to an executable file, denoted by its absolute path.
+   * \param executables A list of paths to executable files, denoted by an absolute path.
    */
-  ctkCmdLineModuleReference loadModule(const QString& pathToExecutable);
+  QList<ctkCmdLineModuleReference> loadModules(const QStringList& executables);
 
   /**
-   * \brief Removes the executable from both the ctkCmdLineModuleManager and this->MapFileNameToReference.
-   * \param pathToExecutable path to an executable file, denoted by its absolute path.
+   * \brief Removes the executables from both the ctkCmdLineModuleManager and this->MapFileNameToReference.
+   * \param executables path to an executable file, denoted by its absolute path.
    */
-  void unloadModule(const QString& pathToExecutable);
+  void unloadModules(const QStringList& executables);
 
   QHash<QString, ctkCmdLineModuleReference> MapFileNameToReference;
   ctkCmdLineModuleManager* ModuleManager;

Libs/CommandLineModules/Core/ctkCmdLineModuleManager.cpp

@@ -35,6 +35,7 @@
 #include <QUrl>
 #include <QHash>
 #include <QList>
+#include <QMutex>
 
 #include <QFuture>
 
@@ -44,7 +45,7 @@ struct ctkCmdLineModuleManagerPrivate
     : ValidationMode(mode)
   {}
 
-  void checkBackends(const QUrl& location)
+  void checkBackends_unlocked(const QUrl& location)
   {
     if (!this->SchemeToBackend.contains(location.scheme()))
     {
@@ -52,10 +53,11 @@ struct ctkCmdLineModuleManagerPrivate
     }
   }
 
+  QMutex Mutex;
   QHash<QString, ctkCmdLineModuleBackend*> SchemeToBackend;
   QHash<QUrl, ctkCmdLineModuleReference> LocationToRef;
 
-  ctkCmdLineModuleManager::ValidationMode ValidationMode;
+  const ctkCmdLineModuleManager::ValidationMode ValidationMode;
 };
 
 ctkCmdLineModuleManager::ctkCmdLineModuleManager(ValidationMode validationMode)
@@ -69,9 +71,11 @@ ctkCmdLineModuleManager::~ctkCmdLineModuleManager()
 
 void ctkCmdLineModuleManager::registerBackend(ctkCmdLineModuleBackend *backend)
 {
+  QMutexLocker lock(&d->Mutex);
+
   QList<QString> supportedSchemes = backend->schemes();
 
-  // Check if there is already a backound registerd for any of the
+  // Check if there is already a backend registerd for any of the
   // supported schemes. We only supported one backend per scheme.
   foreach (QString scheme, supportedSchemes)
   {
@@ -91,10 +95,23 @@ void ctkCmdLineModuleManager::registerBackend(ctkCmdLineModuleBackend *backend)
 ctkCmdLineModuleReference
 ctkCmdLineModuleManager::registerModule(const QUrl &location)
 {
-  d->checkBackends(location);
+  QByteArray xml;
+  ctkCmdLineModuleBackend* backend = NULL;
+  {
+    QMutexLocker lock(&d->Mutex);
+
+    d->checkBackends_unlocked(location);
+
+    // If the module is already registered, just return the reference
+    if (d->LocationToRef.contains(location))
+    {
+      return d->LocationToRef[location];
+    }
 
-  QByteArray xml = d->SchemeToBackend[location.scheme()]->rawXmlDescription(location);
+    backend = d->SchemeToBackend[location.scheme()];
+  }
 
+  xml = backend->rawXmlDescription(location);
   if (xml.isEmpty())
   {
     throw ctkInvalidArgumentException(QString("No XML output available from ") + location.toString());
@@ -103,7 +120,7 @@ ctkCmdLineModuleManager::registerModule(const QUrl &location)
   ctkCmdLineModuleReference ref;
   ref.d->Location = location;
   ref.d->RawXmlDescription = xml;
-  ref.d->Backend = d->SchemeToBackend[location.scheme()];
+  ref.d->Backend = backend;
 
   if (d->ValidationMode != SKIP_VALIDATION)
   {
@@ -126,7 +143,16 @@ ctkCmdLineModuleManager::registerModule(const QUrl &location)
     }
   }
 
-  d->LocationToRef[location] = ref;
+  {
+    QMutexLocker lock(&d->Mutex);
+    // Check that we don't have a race condition
+    if (d->LocationToRef.contains(location))
+    {
+      // Another thread registered a module with the same location
+      return d->LocationToRef[location];
+    }
+    d->LocationToRef[location] = ref;
+  }
 
   emit moduleRegistered(ref);
   return ref;
@@ -134,23 +160,33 @@ ctkCmdLineModuleManager::registerModule(const QUrl &location)
 
 void ctkCmdLineModuleManager::unregisterModule(const ctkCmdLineModuleReference& ref)
 {
-  d->LocationToRef.remove(ref.location());
+  {
+    QMutexLocker lock(&d->Mutex);
+    if (!d->LocationToRef.contains(ref.location()))
+    {
+      return;
+    }
+    d->LocationToRef.remove(ref.location());
+  }
   emit moduleUnregistered(ref);
 }
 
 ctkCmdLineModuleReference ctkCmdLineModuleManager::moduleReference(const QUrl &location) const
 {
+  QMutexLocker lock(&d->Mutex);
   return d->LocationToRef[location];
 }
 
 QList<ctkCmdLineModuleReference> ctkCmdLineModuleManager::moduleReferences() const
 {
+  QMutexLocker lock(&d->Mutex);
   return d->LocationToRef.values();
 }
 
 ctkCmdLineModuleFuture ctkCmdLineModuleManager::run(ctkCmdLineModuleFrontend *frontend)
 {
-  d->checkBackends(frontend->location());
+  QMutexLocker lock(&d->Mutex);
+  d->checkBackends_unlocked(frontend->location());
 
   ctkCmdLineModuleFuture future = d->SchemeToBackend[frontend->location().scheme()]->run(frontend);
   frontend->setFuture(future);