Skip to content

Commit

Permalink
Don't crash when a null task is added to task manager
Browse files Browse the repository at this point in the history
  • Loading branch information
@nyalldawson
nyalldawson committed May 25, 2018
1 parent 595ecce commit 43928d8
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 24 deletions.
4 changes: 2 additions & 2 deletions python/core/auto_generated/qgstaskmanager.sip.in
View file
Expand Up @@ -331,7 +331,7 @@ the task. The priority argument can be used to control the run queue's
order of execution, with larger numbers
taking precedence over lower priority numbers.

:return: unique task ID
:return: unique task ID, or 0 if task could not be added
%End

long addTask( const TaskDefinition &task /Transfer/, int priority = 0 );
Expand All @@ -342,7 +342,7 @@ manager will be responsible for starting the task. The priority argument can
be used to control the run queue's order of execution, with larger numbers
taking precedence over lower priority numbers.

:return: unique task ID
:return: unique task ID, or 0 if task could not be added
%End

QgsTask *task( long id ) const;
Expand Down
3 changes: 3 additions & 0 deletions src/core/qgstaskmanager.cpp
View file
Expand Up @@ -390,6 +390,9 @@ long QgsTaskManager::addTask( const QgsTaskManager::TaskDefinition &definition,

long QgsTaskManager::addTaskPrivate( QgsTask *task, QgsTaskList dependencies, bool isSubTask, int priority )
{
if ( !task )
return 0;

long taskId = mNextTaskId++;

mTaskMutex->lock();
Expand Down
6 changes: 3 additions & 3 deletions src/core/qgstaskmanager.h
View file
Expand Up @@ -405,7 +405,7 @@ class CORE_EXPORT QgsTaskManager : public QObject
* the task. The priority argument can be used to control the run queue's
* order of execution, with larger numbers
* taking precedence over lower priority numbers.
* \returns unique task ID
* \returns unique task ID, or 0 if task could not be added
*/
long addTask( QgsTask *task SIP_TRANSFER, int priority = 0 );

Expand All @@ -415,7 +415,7 @@ class CORE_EXPORT QgsTaskManager : public QObject
* manager will be responsible for starting the task. The priority argument can
* be used to control the run queue's order of execution, with larger numbers
* taking precedence over lower priority numbers.
* \returns unique task ID
* \returns unique task ID, or 0 if task could not be added
*/
long addTask( const TaskDefinition &task SIP_TRANSFER, int priority = 0 );

Expand Down Expand Up @@ -573,7 +573,7 @@ class CORE_EXPORT QgsTaskManager : public QObject
QMap< long, QgsWeakMapLayerPointerList > mLayerDependencies;

//! Tracks the next unique task ID
long mNextTaskId = 0;
long mNextTaskId = 1;

//! List of active (queued or running) tasks. Includes subtasks.
QSet< QgsTask * > mActiveTasks;
Expand Down
41 changes: 22 additions & 19 deletions tests/src/core/testqgstaskmanager.cpp
View file
Expand Up @@ -329,14 +329,17 @@ void TestQgsTaskManager::addTask()

QSignalSpy spy( &manager, &QgsTaskManager::taskAdded );

// null task
QVERIFY( !manager.addTask( nullptr ) );

//add a task
CancelableTask *task = new CancelableTask();
long id = manager.addTask( task );
QCOMPARE( id, 0L );
QCOMPARE( id, 1L );
QCOMPARE( manager.tasks().count(), 1 );
QCOMPARE( manager.count(), 1 );
QCOMPARE( spy.count(), 1 );
QCOMPARE( spy.last().at( 0 ).toLongLong(), 0LL );
QCOMPARE( spy.last().at( 0 ).toLongLong(), 1LL );
while ( !task->isActive() )
{
QCoreApplication::processEvents();
Expand All @@ -346,18 +349,18 @@ void TestQgsTaskManager::addTask()
QCOMPARE( task->status(), QgsTask::Running );

//retrieve task
QCOMPARE( manager.task( 0L ), task );
QCOMPARE( manager.task( 1L ), task );
QCOMPARE( manager.tasks().at( 0 ), task );

//add a second task
CancelableTask *task2 = new CancelableTask();
id = manager.addTask( task2 );
QCOMPARE( id, 1L );
QCOMPARE( id, 2L );
QCOMPARE( manager.tasks().count(), 2 );
QCOMPARE( manager.count(), 2 );
QCOMPARE( manager.task( 0L ), task );
QCOMPARE( manager.task( 1L ), task );
QVERIFY( manager.tasks().contains( task ) );
QCOMPARE( manager.task( 1L ), task2 );
QCOMPARE( manager.task( 2L ), task2 );
QVERIFY( manager.tasks().contains( task2 ) );
while ( !task2->isActive() )
{
Expand All @@ -368,7 +371,7 @@ void TestQgsTaskManager::addTask()
QCOMPARE( task2->status(), QgsTask::Running );

QCOMPARE( spy.count(), 2 );
QCOMPARE( spy.last().at( 0 ).toLongLong(), 1LL );
QCOMPARE( spy.last().at( 0 ).toLongLong(), 2LL );

task->cancel();
task2->cancel();
Expand Down Expand Up @@ -649,8 +652,8 @@ void TestQgsTaskManager::taskId()
TestTask *task3 = new TestTask();

QCOMPARE( manager.taskId( nullptr ), -1L );
QCOMPARE( manager.taskId( task ), 0L );
QCOMPARE( manager.taskId( task2 ), 1L );
QCOMPARE( manager.taskId( task ), 1L );
QCOMPARE( manager.taskId( task2 ), 2L );
QCOMPARE( manager.taskId( task3 ), -1L );

delete task3;
Expand Down Expand Up @@ -725,15 +728,15 @@ void TestQgsTaskManager::progressChanged()

QCOMPARE( task->progress(), 50.0 );
QCOMPARE( spy.count(), 1 );
QCOMPARE( spy.last().at( 0 ).toLongLong(), 0LL );
QCOMPARE( spy.last().at( 0 ).toLongLong(), 1LL );
QCOMPARE( spy.last().at( 1 ).toDouble(), 50.0 );
//multiple running tasks, so finalTaskProgressChanged(double) should not be emitted
QCOMPARE( spy2.count(), 0 );

task2->emitProgressChanged( 75.0 );
QCOMPARE( task2->progress(), 75.0 );
QCOMPARE( spy.count(), 2 );
QCOMPARE( spy.last().at( 0 ).toLongLong(), 1LL );
QCOMPARE( spy.last().at( 0 ).toLongLong(), 2LL );
QCOMPARE( spy.last().at( 1 ).toDouble(), 75.0 );
QCOMPARE( spy2.count(), 0 );

Expand Down Expand Up @@ -799,7 +802,7 @@ void TestQgsTaskManager::statusChanged()
flushEvents();

QCOMPARE( spy.count(), 1 );
QCOMPARE( spy.last().at( 0 ).toLongLong(), 1LL );
QCOMPARE( spy.last().at( 0 ).toLongLong(), 2LL );
QCOMPARE( static_cast< QgsTask::TaskStatus >( spy.last().at( 1 ).toInt() ), QgsTask::Running );

task->terminate();
Expand All @@ -809,7 +812,7 @@ void TestQgsTaskManager::statusChanged()
}
flushEvents();
QCOMPARE( spy.count(), 2 );
QCOMPARE( spy.last().at( 0 ).toLongLong(), 0LL );
QCOMPARE( spy.last().at( 0 ).toLongLong(), 1LL );
QCOMPARE( static_cast< QgsTask::TaskStatus >( spy.last().at( 1 ).toInt() ), QgsTask::Terminated );

task2->finish();
Expand All @@ -819,7 +822,7 @@ void TestQgsTaskManager::statusChanged()
}
flushEvents();
QCOMPARE( spy.count(), 3 );
QCOMPARE( spy.last().at( 0 ).toLongLong(), 1LL );
QCOMPARE( spy.last().at( 0 ).toLongLong(), 2LL );
QCOMPARE( static_cast< QgsTask::TaskStatus >( spy.last().at( 1 ).toInt() ), QgsTask::Complete );
}

Expand Down Expand Up @@ -1126,7 +1129,7 @@ void TestQgsTaskManager::managerWithSubTasks()
QCOMPARE( spyProgress.count(), 0 );
subTask->emitProgressChanged( 50 );
QCOMPARE( spyProgress.count(), 1 );
QCOMPARE( spyProgress.last().at( 0 ).toLongLong(), 0LL );
QCOMPARE( spyProgress.last().at( 0 ).toLongLong(), 1LL );
// subTask itself is 50% done, so with it's child task it's sitting at overall 25% done
// (one task 50%, one task not started)
// parent task has two tasks (itself + subTask), and subTask is 25% done.... so parent
Expand All @@ -1135,11 +1138,11 @@ void TestQgsTaskManager::managerWithSubTasks()

subsubTask->emitProgressChanged( 100 );
QCOMPARE( spyProgress.count(), 2 );
QCOMPARE( spyProgress.last().at( 0 ).toLongLong(), 0LL );
QCOMPARE( spyProgress.last().at( 0 ).toLongLong(), 1LL );
QCOMPARE( spyProgress.last().at( 1 ).toInt(), 38 );
parent->emitProgressChanged( 50 );
QCOMPARE( spyProgress.count(), 3 );
QCOMPARE( spyProgress.last().at( 0 ).toLongLong(), 0LL );
QCOMPARE( spyProgress.last().at( 0 ).toLongLong(), 1LL );
QCOMPARE( spyProgress.last().at( 1 ).toInt(), 63 );

//manager should not emit statusChanged signals for subtasks
Expand All @@ -1152,7 +1155,7 @@ void TestQgsTaskManager::managerWithSubTasks()
}
flushEvents();
QCOMPARE( statusSpy.count(), 1 );
QCOMPARE( statusSpy.last().at( 0 ).toLongLong(), 0LL );
QCOMPARE( statusSpy.last().at( 0 ).toLongLong(), 1LL );
QCOMPARE( static_cast< QgsTask::TaskStatus >( statusSpy.last().at( 1 ).toInt() ), QgsTask::Running );

subTask->finish();
Expand All @@ -1170,7 +1173,7 @@ void TestQgsTaskManager::managerWithSubTasks()
}
flushEvents();
QCOMPARE( statusSpy.count(), 2 );
QCOMPARE( statusSpy.last().at( 0 ).toLongLong(), 0LL );
QCOMPARE( statusSpy.last().at( 0 ).toLongLong(), 1LL );
QCOMPARE( static_cast< QgsTask::TaskStatus >( statusSpy.last().at( 1 ).toInt() ), QgsTask::Complete );


Expand Down

0 comments on commit 43928d8

Please sign in to comment.