Index: components/sync/engine/model_safe_worker.cc |
diff --git a/components/sync/engine/model_safe_worker.cc b/components/sync/engine/model_safe_worker.cc |
index 530f206599673cf1b91239e5d40aeff463b0406c..3c8616e0e927b445701d6d141001a1778ae05cc9 100644 |
--- a/components/sync/engine/model_safe_worker.cc |
+++ b/components/sync/engine/model_safe_worker.cc |
@@ -4,6 +4,9 @@ |
#include "components/sync/engine/model_safe_worker.h" |
+#include <utility> |
+ |
+#include "base/bind.h" |
#include "base/json/json_writer.h" |
#include "base/values.h" |
@@ -69,24 +72,106 @@ std::string ModelSafeGroupToString(ModelSafeGroup group) { |
} |
} |
-ModelSafeWorker::ModelSafeWorker() {} |
+// An object which signals the |work_done_or_abandoned_| event of a |
+// ModelSafeWorker when it is deleted. |
+class ModelSafeWorker::ScopedSignalWorkDoneOrAbandoned { |
skym
2017/04/06 00:34:54
This feels like a generic problem that is being so
fdoray
2017/04/06 18:10:55
Done.
|
+ public: |
+ explicit ScopedSignalWorkDoneOrAbandoned(scoped_refptr<ModelSafeWorker> outer) |
+ : outer_(std::move(outer)) {} |
+ |
+ ScopedSignalWorkDoneOrAbandoned(ScopedSignalWorkDoneOrAbandoned&& other) = |
+ default; |
+ ScopedSignalWorkDoneOrAbandoned& operator=( |
+ ScopedSignalWorkDoneOrAbandoned&& other) = default; |
+ |
+ ~ScopedSignalWorkDoneOrAbandoned() { |
+ if (outer_) |
skym
2017/04/06 00:34:54
Why's this if check needed?
fdoray
2017/04/06 18:10:55
n/a
|
+ outer_->work_done_or_abandoned_.Signal(); |
+ } |
+ |
+ private: |
+ scoped_refptr<ModelSafeWorker> outer_; |
+ |
+ DISALLOW_COPY_AND_ASSIGN(ScopedSignalWorkDoneOrAbandoned); |
+}; |
+ |
+ModelSafeWorker::ModelSafeWorker() |
+ : work_done_or_abandoned_(base::WaitableEvent::ResetPolicy::AUTOMATIC, |
+ base::WaitableEvent::InitialState::NOT_SIGNALED) { |
+} |
ModelSafeWorker::~ModelSafeWorker() {} |
void ModelSafeWorker::RequestStop() { |
- // Set stop flag. This prevents any *further* tasks from being posted to |
- // worker threads (see DoWorkAndWaitUntilDone below), but note that one may |
- // already be posted. |
- stopped_.Set(); |
+ base::AutoLock auto_lock(lock_); |
+ |
+ // Set stop flag to prevent any *further* WorkCallback from starting to run |
+ // (note that one may alreay be running). |
+ stopped_ = true; |
+ |
+ // If no work is running, unblock DoWorkAndWaitUntilDone(). If work is |
+ // running, it is unsafe to return from DoWorkAndWaitUntilDone(). |
+ // ScopedSignalWorkDoneOrAbandoned will take care of signaling the event when |
+ // the work is done. |
+ if (!is_work_running_) |
+ work_done_or_abandoned_.Signal(); |
} |
SyncerError ModelSafeWorker::DoWorkAndWaitUntilDone(const WorkCallback& work) { |
- if (stopped_.IsSet()) |
- return CANNOT_DO_WORK; |
- return DoWorkAndWaitUntilDoneImpl(work); |
+ { |
+ // It is important to check |stopped_| and reset |work_done_or_abandoned_| |
+ // atomically to prevent this race: |
+ // |
+ // Thread Action |
+ // Sync Sees that |stopped_| is false. |
+ // UI Calls RequestStop(). Signals |work_done_or_abandoned_|. |
+ // Sync Resets |work_done_or_abandoned_|. |
+ // Waits on |work_done_or_abandoned_| forever since the task may not |
+ // run after RequestStop() is called. |
+ base::AutoLock auto_lock(lock_); |
+ if (stopped_) |
+ return CANNOT_DO_WORK; |
+ DCHECK(!is_work_running_); |
+ work_done_or_abandoned_.Reset(); |
+ } |
+ |
+ SyncerError error = UNSET; |
+ bool did_run = false; |
+ ScheduleWork(base::Bind(&ModelSafeWorker::DoWork, this, work, |
+ base::Passed(ScopedSignalWorkDoneOrAbandoned(this)), |
+ base::Unretained(&error), |
+ base::Unretained(&did_run))); |
+ |
+ // Unblocked when the task runs or is deleted or when RequestStop() is called |
+ // before the task starts running. |
+ work_done_or_abandoned_.Wait(); |
+ |
+ return did_run ? error : CANNOT_DO_WORK; |
} |
-bool ModelSafeWorker::IsStopped() { |
- return stopped_.IsSet(); |
+void ModelSafeWorker::DoWork( |
+ const WorkCallback& work, |
+ ScopedSignalWorkDoneOrAbandoned scoped_signal_work_done_or_abandoned, |
+ SyncerError* error, |
+ bool* did_run) { |
+ { |
+ base::AutoLock auto_lock(lock_); |
+ if (stopped_) |
+ return; |
+ |
+ // Set |is_work_running_| to make sure that DoWorkAndWaitUntilDone() doesn't |
+ // return while the work is running. |
+ DCHECK(!is_work_running_); |
+ is_work_running_ = true; |
+ } |
+ |
+ *error = work.Run(); |
+ *did_run = true; |
+ |
+ { |
+ base::AutoLock auto_lock(lock_); |
+ DCHECK(is_work_running_); |
+ is_work_running_ = false; |
+ } |
} |
} // namespace syncer |