Make QgsTaskManager handle threading of tasks

python/core/qgstaskmanager.sip

@@ -14,11 +14,10 @@ class QgsTask : QObject
     //! Status of tasks
     enum TaskStatus
     {
+      Queued, /*!< Task is queued and has not begun */
       Running, /*!< Task is currently running */
       Complete, /*!< Task successfully completed */
       Terminated, /*!< Task was terminated or errored */
-      // Paused,
-      // Queued,
     };
 
     /** Constructor for QgsTask.
@@ -26,10 +25,12 @@ class QgsTask : QObject
      */
     QgsTask( const QString& description = QString() );
 
-    //! Will be called when task has been terminated, either through
-    //! user interaction or other reason (eg application exit)
-    //! @note derived classes must ensure they call the base method
-    virtual void terminate();
+    //! Starts the task.
+    void start();
+
+    //! Notifies the task that it should terminate.
+    //! @see isCancelled()
+    void terminate();
 
     //! Returns true if the task is active, ie it is not complete and has
     //! not been terminated.
@@ -58,6 +59,11 @@ class QgsTask : QObject
     //! completed() or stopped()
     void statusChanged( int status );
 
+    //! Will be emitted by task to indicate its commencement.
+    //! @note derived classes should not emit this signal directly, it will automatically
+    //! be emitted when the task begins
+    void begun();
+
     //! Will be emitted by task to indicate its completion.
     //! @note derived classes should not emit this signal directly, instead they should call
     //! completed()
@@ -69,7 +75,7 @@ class QgsTask : QObject
     //! stopped()//!
     void taskStopped();
 
-  protected:
+  public slots:
 
     //! Sets the task's current progress. Should be called whenever the
     //! task wants to update it's progress. Calling will automatically emit the progressChanged
@@ -85,6 +91,17 @@ class QgsTask : QObject
     //! reason other than successful completion.
     //! Calling will automatically emit the statusChanged and taskStopped signals.
     void stopped();
+
+  protected:
+
+    //! Derived tasks must implement a run() method. This method will be called when the
+    //! task commences (ie via calling start() ).
+    virtual void run() = 0;
+
+    //! Will return true if task should terminate ASAP. Derived classes run() methods
+    //! should periodically check this and terminate in a safe manner.
+    bool isCancelled() const;
+
 };
 
 /** \ingroup core
@@ -151,6 +168,9 @@ class QgsTaskManager : QObject
      */
     long taskId( QgsTask* task ) const;
 
+    //! Instructs all tasks tracked by the manager to terminate.
+    void terminateAll();
+
   signals:
 
     //! Will be emitted when a task reports a progress change

src/core/qgstaskmanager.cpp

@@ -16,7 +16,7 @@
  ***************************************************************************/
 
 #include "qgstaskmanager.h"
-#include <QMutex>
+#include <QtConcurrentRun>
 
 
 //
@@ -26,10 +26,24 @@
 QgsTask::QgsTask( const QString &name )
     : QObject()
     , mDescription( name )
-    , mStatus( Running )
+    , mStatus( Queued )
     , mProgress( 0.0 )
+    , mShouldTerminate( false )
 {}
 
+void QgsTask::start()
+{
+  mStatus = Running;
+  emit statusChanged( Running );
+  emit begun();
+  run();
+}
+
+void QgsTask::terminate()
+{
+  mShouldTerminate = true;
+}
+
 void QgsTask::setProgress( double progress )
 {
   mProgress = progress;
@@ -74,28 +88,33 @@ QgsTaskManager::QgsTaskManager( QObject* parent )
 
 QgsTaskManager::~QgsTaskManager()
 {
-  QMap< long, QgsTask* >::const_iterator it = mTasks.constBegin();
+  //first tell all tasks to cancel
+  terminateAll();
+
+  //then clean them up, including waiting for them to terminate
+  QMap< long, TaskInfo >::const_iterator it = mTasks.constBegin();
   for ( ; it != mTasks.constEnd(); ++it )
   {
-    cleanupAndDeleteTask( it.value() );
+    cleanupAndDeleteTask( it.value().task );
   }
 }
 
 long QgsTaskManager::addTask( QgsTask* task )
 {
-  static QMutex sAddMutex( QMutex::Recursive );
-  QMutexLocker locker( &sAddMutex );
-
   mTasks.insert( mNextTaskId, task );
+
   connect( task, SIGNAL( progressChanged( double ) ), this, SLOT( taskProgressChanged( double ) ) );
   connect( task, SIGNAL( statusChanged( int ) ), this, SLOT( taskStatusChanged( int ) ) );
+
+  mTasks[ mNextTaskId ].future = QtConcurrent::run( task, &QgsTask::start );
+
   emit taskAdded( mNextTaskId );
   return mNextTaskId++;
 }
 
 bool QgsTaskManager::deleteTask( long id )
 {
-  QgsTask* task = mTasks.value( id );
+  QgsTask* task = mTasks.value( id ).task;
   return deleteTask( task );
 }
 
@@ -107,9 +126,9 @@ bool QgsTaskManager::deleteTask( QgsTask *task )
   bool result = cleanupAndDeleteTask( task );
 
   // remove from internal task list
-  for ( QMap< long, QgsTask* >::iterator it = mTasks.begin(); it != mTasks.end(); )
+  for ( QMap< long, TaskInfo >::iterator it = mTasks.begin(); it != mTasks.end(); )
   {
-    if ( it.value() == task )
+    if ( it.value().task == task )
       it = mTasks.erase( it );
     else
       ++it;
@@ -120,12 +139,17 @@ bool QgsTaskManager::deleteTask( QgsTask *task )
 
 QgsTask*QgsTaskManager::task( long id ) const
 {
-  return mTasks.value( id );
+  return mTasks.value( id ).task;
 }
 
 QList<QgsTask*> QgsTaskManager::tasks() const
 {
-  return mTasks.values();
+  QList< QgsTask* > list;
+  for ( QMap< long, TaskInfo >::const_iterator it = mTasks.constBegin(); it != mTasks.constEnd(); ++it )
+  {
+    list << it.value().task;
+  }
+  return list;
 }
 
 long QgsTaskManager::taskId( QgsTask *task ) const
@@ -133,15 +157,28 @@ long QgsTaskManager::taskId( QgsTask *task ) const
   if ( !task )
     return -1;
 
-  QMap< long, QgsTask* >::const_iterator it = mTasks.constBegin();
+  QMap< long, TaskInfo >::const_iterator it = mTasks.constBegin();
   for ( ; it != mTasks.constEnd(); ++it )
   {
-    if ( it.value() == task )
+    if ( it.value().task == task )
       return it.key();
   }
   return -1;
 }
 
+void QgsTaskManager::terminateAll()
+{
+  QMap< long, TaskInfo >::iterator it = mTasks.begin();
+  for ( ; it != mTasks.end(); ++it )
+  {
+    QgsTask* task = it.value().task;
+    if ( task->isActive() )
+    {
+      task->terminate();
+    }
+  }
+}
+
 void QgsTaskManager::taskProgressChanged( double progress )
 {
   QgsTask* task = qobject_cast< QgsTask* >( sender() );
@@ -174,6 +211,17 @@ bool QgsTaskManager::cleanupAndDeleteTask( QgsTask *task )
   if ( task->isActive() )
     task->terminate();
 
+  // wait for task to terminate
+  QMap< long, TaskInfo >::iterator it = mTasks.begin();
+  for ( ; it != mTasks.end(); ++it )
+  {
+    if ( it.value().task == task )
+    {
+      it.value().future.waitForFinished();
+      break;
+    }
+  }
+
   emit taskAboutToBeDeleted( taskId( task ) );
 
   task->deleteLater();

src/core/qgstaskmanager.h

@@ -21,10 +21,12 @@
 #include <QObject>
 #include <QMap>
 #include <QAbstractItemModel>
+#include <QFuture>
 
 /** \ingroup core
  * \class QgsTask
- * \brief Interface class for long running background tasks which will be handled by a QgsTaskManager
+ * \brief Interface class for long running background tasks. Tasks can be controlled directly,
+ * or added to a QgsTaskManager for automatic management.
  * \note Added in version 2.16
  */
 class CORE_EXPORT QgsTask : public QObject
@@ -36,11 +38,10 @@ class CORE_EXPORT QgsTask : public QObject
     //! Status of tasks
     enum TaskStatus
     {
+      Queued, /*!< Task is queued and has not begun */
       Running, /*!< Task is currently running */
       Complete, /*!< Task successfully completed */
       Terminated, /*!< Task was terminated or errored */
-      // Paused,
-      // Queued,
     };
 
     /** Constructor for QgsTask.
@@ -48,13 +49,12 @@ class CORE_EXPORT QgsTask : public QObject
      */
     QgsTask( const QString& description = QString() );
 
-    //! Will be called when task has been terminated, either through
-    //! user interaction or other reason (eg application exit)
-    //! @note derived classes must ensure they call the base method
-    virtual void terminate()
-    {
-      stopped();
-    }
+    //! Starts the task.
+    void start();
+
+    //! Notifies the task that it should terminate.
+    //! @see isCancelled()
+    void terminate();
 
     //! Returns true if the task is active, ie it is not complete and has
     //! not been terminated.
@@ -83,6 +83,11 @@ class CORE_EXPORT QgsTask : public QObject
     //! completed() or stopped()
     void statusChanged( int status );
 
+    //! Will be emitted by task to indicate its commencement.
+    //! @note derived classes should not emit this signal directly, it will automatically
+    //! be emitted when the task begins
+    void begun();
+
     //! Will be emitted by task to indicate its completion.
     //! @note derived classes should not emit this signal directly, instead they should call
     //! completed()
@@ -94,7 +99,7 @@ class CORE_EXPORT QgsTask : public QObject
     //! stopped()//!
     void taskStopped();
 
-  protected:
+  public slots:
 
     //! Sets the task's current progress. Should be called whenever the
     //! task wants to update it's progress. Calling will automatically emit the progressChanged
@@ -111,11 +116,22 @@ class CORE_EXPORT QgsTask : public QObject
     //! Calling will automatically emit the statusChanged and taskStopped signals.
     void stopped();
 
+  protected:
+
+    //! Derived tasks must implement a run() method. This method will be called when the
+    //! task commences (ie via calling start() ).
+    virtual void run() = 0;
+
+    //! Will return true if task should terminate ASAP. Derived classes run() methods
+    //! should periodically check this and terminate in a safe manner.
+    bool isCancelled() const { return mShouldTerminate; }
+
   private:
 
     QString mDescription;
     TaskStatus mStatus;
     double mProgress;
+    bool mShouldTerminate;
 
 };
 
@@ -143,7 +159,8 @@ class CORE_EXPORT QgsTaskManager : public QObject
     virtual ~QgsTaskManager();
 
     /** Adds a task to the manager. Ownership of the task is transferred
-     * to the manager.
+     * to the manager, and the task manager will be responsible for starting
+     * the task.
      * @param task task to add
      * @returns unique task ID
      */
@@ -182,6 +199,9 @@ class CORE_EXPORT QgsTaskManager : public QObject
      */
     long taskId( QgsTask* task ) const;
 
+    //! Instructs all tasks tracked by the manager to terminate.
+    void terminateAll();
+
   signals:
 
     //! Will be emitted when a task reports a progress change
@@ -210,7 +230,17 @@ class CORE_EXPORT QgsTaskManager : public QObject
   private:
 
     static QgsTaskManager *sInstance;
-    QMap< long, QgsTask* > mTasks;
+
+    struct TaskInfo
+    {
+      TaskInfo( QgsTask* task = nullptr )
+          : task( task )
+      {}
+      QgsTask* task;
+      QFuture< void > future;
+    };
+
+    QMap< long, TaskInfo > mTasks;
 
     //! Tracks the next unique task ID
     long mNextTaskId;

src/gui/qgstaskmanagerwidget.cpp

@@ -321,6 +321,9 @@ void QgsTaskStatusDelegate::paint( QPainter *painter, const QStyleOptionViewItem
   QIcon icon;
   switch ( static_cast< QgsTask::TaskStatus >( index.data().toInt() ) )
   {
+    case QgsTask::Queued:
+      icon = QgsApplication::getThemeIcon( "/mIconFieldTime.svg" );
+      break;
     case QgsTask::Running:
       icon = QgsApplication::getThemeIcon( "/mActionRefresh.png" );
       break;

tests/src/core/testqgstaskmanager.cpp

@@ -26,12 +26,21 @@ class TestTask : public QgsTask
 
   public:
 
-    TestTask( const QString& desc = QString() ) : QgsTask( desc ) {}
+    TestTask( const QString& desc = QString() ) : QgsTask( desc ), runCalled( false ) {}
 
     void emitProgressChanged( double progress ) { setProgress( progress ); }
     void emitTaskStopped() { stopped(); }
     void emitTaskCompleted() { completed(); }
 
+    bool runCalled;
+
+  protected:
+
+    void run() override
+    {
+      runCalled = true;
+    }
+
 };
 
 class TestTerminationTask : public TestTask
@@ -45,6 +54,13 @@ class TestTerminationTask : public TestTask
       //make sure task has been terminated by manager prior to deletion
       Q_ASSERT( status() == QgsTask::Terminated );
     }
+
+  protected:
+
+    void run() override
+    {
+      QTest::qSleep( 1000 );
+    }
 };
 
 
@@ -93,18 +109,28 @@ void TestQgsTaskManager::cleanup()
 void TestQgsTaskManager::task()
 {
   QScopedPointer< TestTask > task( new TestTask( "desc" ) );
-  QCOMPARE( task->status(), QgsTask::Running );
+  QCOMPARE( task->status(), QgsTask::Queued );
   QCOMPARE( task->description(), QString( "desc" ) );
+  QVERIFY( !task->isActive() );
+
+  QSignalSpy startedSpy( task.data(), SIGNAL( begun() ) );
+  QSignalSpy statusSpy( task.data(), SIGNAL( statusChanged( int ) ) );
+
+  task->start();
+  QCOMPARE( task->status(), QgsTask::Running );
   QVERIFY( task->isActive() );
+  QVERIFY( task->runCalled );
+  QCOMPARE( startedSpy.count(), 1 );
+  QCOMPARE( statusSpy.count(), 1 );
+  QCOMPARE( static_cast< QgsTask::TaskStatus >( statusSpy.last().at( 0 ).toInt() ), QgsTask::Running );
 
   //test that calling stopped sets correct state
   QSignalSpy stoppedSpy( task.data(), SIGNAL( taskStopped() ) );
-  QSignalSpy statusSpy( task.data(), SIGNAL( statusChanged( int ) ) );
   task->emitTaskStopped();
   QCOMPARE( task->status(), QgsTask::Terminated );
   QVERIFY( !task->isActive() );
   QCOMPARE( stoppedSpy.count(), 1 );
-  QCOMPARE( statusSpy.count(), 1 );
+  QCOMPARE( statusSpy.count(), 2 );
   QCOMPARE( static_cast< QgsTask::TaskStatus >( statusSpy.last().at( 0 ).toInt() ), QgsTask::Terminated );
 
   //test that calling completed sets correct state
@@ -211,6 +237,9 @@ void TestQgsTaskManager::taskTerminationBeforeDelete()
   TestTask* task = new TestTerminationTask();
   manager->addTask( task );
 
+  //SHOULD NOT BE NEEDED...
+  task->start();
+
   // if task is not terminated assert will trip
   delete manager;
   QApplication::sendPostedEvents( nullptr, QEvent::DeferredDelete );
@@ -273,13 +302,18 @@ void TestQgsTaskManager::statusChanged()
 
   QSignalSpy spy( &manager, SIGNAL( statusChanged( long, int ) ) );
 
-  task->emitTaskStopped();
+  task->start();
   QCOMPARE( spy.count(), 1 );
   QCOMPARE( spy.last().at( 0 ).toLongLong(), 0LL );
+  QCOMPARE( static_cast< QgsTask::TaskStatus >( spy.last().at( 1 ).toInt() ), QgsTask::Running );
+
+  task->emitTaskStopped();
+  QCOMPARE( spy.count(), 2 );
+  QCOMPARE( spy.last().at( 0 ).toLongLong(), 0LL );
   QCOMPARE( static_cast< QgsTask::TaskStatus >( spy.last().at( 1 ).toInt() ), QgsTask::Terminated );
 
   task2->emitTaskCompleted();
-  QCOMPARE( spy.count(), 2 );
+  QCOMPARE( spy.count(), 3 );
   QCOMPARE( spy.last().at( 0 ).toLongLong(), 1LL );
   QCOMPARE( static_cast< QgsTask::TaskStatus >( spy.last().at( 1 ).toInt() ), QgsTask::Complete );
 }