From 1616166fbca4986619ec25deb708d34342a3ebf2 Mon Sep 17 00:00:00 2001 From: itsmattkc Date: Fri, 9 Aug 2019 12:31:02 +1000 Subject: [PATCH] worked on thread safety with tasks --- app/common/CMakeLists.txt | 2 ++ app/common/threadedobject.cpp | 37 ++++++++++++++++++++++++++++++++ app/common/threadedobject.h | 25 ++++++++++++++++++++++ app/project/item/item.cpp | 10 --------- app/project/item/item.h | 9 +++----- app/task/import/import.cpp | 11 +++++----- app/task/probe/probe.cpp | 4 ++-- app/task/task.cpp | 26 ++++++++++++++++++++++- app/task/task.h | 40 ++++++++++++++++++++++++++++++++--- 9 files changed, 137 insertions(+), 27 deletions(-) create mode 100644 app/common/threadedobject.cpp create mode 100644 app/common/threadedobject.h diff --git a/app/common/CMakeLists.txt b/app/common/CMakeLists.txt index b5c3c0a38..549c803ee 100644 --- a/app/common/CMakeLists.txt +++ b/app/common/CMakeLists.txt @@ -26,6 +26,8 @@ set(OLIVE_SOURCES common/qobjectlistcast.h common/qtversionabstraction.h common/qtversionabstraction.cpp + common/threadedobject.h + common/threadedobject.cpp common/timecodefunctions.h common/timecodefunctions.cpp PARENT_SCOPE diff --git a/app/common/threadedobject.cpp b/app/common/threadedobject.cpp new file mode 100644 index 000000000..a80ff44fc --- /dev/null +++ b/app/common/threadedobject.cpp @@ -0,0 +1,37 @@ +#include "threadedobject.h" + +ThreadedObject::ThreadedObject() +{ +} + +void ThreadedObject::LockDeletes() +{ + threadobj_delete_lock_++; +} + +void ThreadedObject::UnlockDeletes() +{ + Q_ASSERT(AreDeletesLocked()); + + threadobj_delete_lock_--; +} + +bool ThreadedObject::AreDeletesLocked() +{ + return (threadobj_delete_lock_ > 0); +} + +void ThreadedObject::LockMutex() +{ + threadobj_main_lock_.lock(); +} + +void ThreadedObject::UnlockMutex() +{ + threadobj_main_lock_.unlock(); +} + +bool ThreadedObject::TryLockMutex(int timeout) +{ + return threadobj_main_lock_.tryLock(timeout); +} diff --git a/app/common/threadedobject.h b/app/common/threadedobject.h new file mode 100644 index 000000000..709c146e5 --- /dev/null +++ b/app/common/threadedobject.h @@ -0,0 +1,25 @@ +#ifndef THREADEDOBJECT_H +#define THREADEDOBJECT_H + +#include + +class ThreadedObject +{ +public: + ThreadedObject(); + + void LockMutex(); + void UnlockMutex(); + bool TryLockMutex(int timeout = 0); + + void LockDeletes(); + void UnlockDeletes(); + bool AreDeletesLocked(); + +private: + QMutex threadobj_main_lock_; + + QAtomicInt threadobj_delete_lock_; +}; + +#endif // THREADEDOBJECT_H diff --git a/app/project/item/item.cpp b/app/project/item/item.cpp index aad058551..0017001c0 100644 --- a/app/project/item/item.cpp +++ b/app/project/item/item.cpp @@ -157,13 +157,3 @@ bool Item::ChildExistsWithNameInternal(const QString &name, Item *folder) return false; } - -void Item::Lock() -{ - mutex_.lock(); -} - -void Item::Unlock() -{ - mutex_.unlock(); -} diff --git a/app/project/item/item.h b/app/project/item/item.h index f1036f5dc..9368d5295 100644 --- a/app/project/item/item.h +++ b/app/project/item/item.h @@ -27,6 +27,8 @@ #include #include +#include "common/threadedobject.h" + class Item; using ItemPtr = std::shared_ptr; @@ -36,7 +38,7 @@ using ItemPtr = std::shared_ptr; * Project objects implement a parent-child hierarchy of Items that can be used throughout the Project. The Item class * itself is abstract and will need to be subclassed to be used in a Project. */ -class Item +class Item : public ThreadedObject { public: enum Type { @@ -100,9 +102,6 @@ public: bool ChildExistsWithName(const QString& name); - void Lock(); - void Unlock(); - private: bool ChildExistsWithNameInternal(const QString& name, Item* folder); @@ -116,8 +115,6 @@ private: QString tooltip_; - QMutex mutex_; - }; #endif // ITEM_H diff --git a/app/task/import/import.cpp b/app/task/import/import.cpp index d308440f3..ddf8bfc62 100644 --- a/app/task/import/import.cpp +++ b/app/task/import/import.cpp @@ -41,11 +41,13 @@ ImportTask::ImportTask(ProjectViewModel *model, Folder *parent, const QStringLis urls_(urls), parent_(parent) { - set_text(tr("Importing %1 files").arg (urls.size())); + set_text(tr("Importing %1 files").arg(urls.size())); } bool ImportTask::Action() { + parent_->LockDeletes(); + QUndoCommand* command = new QUndoCommand(); Import(urls_, parent_, command); @@ -54,12 +56,11 @@ bool ImportTask::Action() // the undo command executes the final import anyway) if (cancelled()) { delete command; - - return true; + } else { + olive::undo_stack.push(command); } - // FIXME: This should be run in the main thread somehow - olive::undo_stack.push(command); + parent_->UnlockDeletes(); return true; } diff --git a/app/task/probe/probe.cpp b/app/task/probe/probe.cpp index 60948af63..297b16bee 100644 --- a/app/task/probe/probe.cpp +++ b/app/task/probe/probe.cpp @@ -34,11 +34,11 @@ ProbeTask::ProbeTask(FootagePtr footage) : bool ProbeTask::Action() { - footage_->Lock(); + footage_->LockDeletes(); Decoder::ProbeMedia(footage_.get()); - footage_->Unlock(); + footage_->UnlockDeletes(); return true; } diff --git a/app/task/task.cpp b/app/task/task.cpp index 860997683..618e9f242 100644 --- a/app/task/task.cpp +++ b/app/task/task.cpp @@ -58,6 +58,12 @@ bool Task::Start() cancelled_ = false; + // Run Prologue() function + if (!Prologue()) { + set_status(kError); + return false; + } + set_status(kWorking); thread_.start(); @@ -65,11 +71,21 @@ bool Task::Start() return true; } +bool Task::Prologue() +{ + return true; +} + bool Task::Action() { return true; } +bool Task::Epilogue() +{ + return true; +} + const Task::Status &Task::status() { return status_; @@ -151,8 +167,16 @@ void Task::set_status(const Task::Status &status) void Task::ThreadComplete() { // thread_.result() will be set to the return value of Action() + bool succeeded = thread_.result(); - if (thread_.result()) { + // Run the Prologue() function for any final tasks + // User cancelling is not considered an error, so we need to check it too + if (succeeded && !cancelled()) { + succeeded = Epilogue(); + } + + // If everything succeeded, we set the status accordingly + if (succeeded) { set_status(kFinished); } else { set_status(kError); diff --git a/app/task/task.h b/app/task/task.h index 856cc8305..2fc7a86bd 100644 --- a/app/task/task.h +++ b/app/task/task.h @@ -89,10 +89,26 @@ public: bool Start(); /** - * @brief The main Task function + * @brief Perform opening tasks before main Task thread begins * - * Action() is the function that gets called once another thread has been created. This function should be overridden - * in subclasses. + * If a Task needs to perform any actions in the main thread before starting the Task's thread, (e.g. copying or + * altering information) this function should be overridden and those actions should be performed here. It's + * guaranteed that Prologue() will run in the main thread, and as such, functions here should remain as minimal as + * possible as to not block the main thread for a noticeable amount of time. If your Task does not need any such + * actions, you don't need to override this. + * + * @return + * + * TRUE if the prologue was successful and we can start the Task now. If Prologue returns FALSE, the thread is never + * created and Action()/Epilogue() are never run. + */ + virtual bool Prologue(); + + /** + * @brief The main Task function which is run in a separate thread + * + * Action() is the function that gets called once the separate thread has been created. This function should be + * overridden in subclasses. * * It's also recommended to emit ProgressChanged() throughout your Action() so that any attached ProgressBars can * show accurate progress information. @@ -105,6 +121,24 @@ public: */ virtual bool Action(); + /** + * @brief Perform any closing Tasks in the main thread after the Task thread finishes + * + * It's likely your Task modifies data used throughout the program in some way, and to prevent race conditions, it's + * recommended to work with "copies" of that data in Action() (which is run in separate thread) and never + * access/modify any data used in other threads. Then, after the Action() thread is complete, that data can be used to + * "apply" that data in the main thread here. + * + * As this runs in the main thread, these functions shouldn't be kept fairly minimal to prevent blocking the main + * thread. + * + * @return + * + * TRUE if the Epilogue completed successfully. FALSE if not. A FALSE result here is considered a complete failure + * of the Task, even though the bulk of the processing has been performed in Action(). + */ + virtual bool Epilogue(); + /** * @brief Current status of the Task *