Index: base/task_scheduler/scheduler_worker.cc |
diff --git a/base/task_scheduler/scheduler_worker.cc b/base/task_scheduler/scheduler_worker.cc |
index 1d6cfee84775207f3338e3b21316d4e8ec0794ad..78398efd58df6f28584892ac4ddc8c410ddb4ade 100644 |
--- a/base/task_scheduler/scheduler_worker.cc |
+++ b/base/task_scheduler/scheduler_worker.cc |
@@ -25,8 +25,8 @@ class SchedulerWorker::Thread : public PlatformThread::Delegate { |
public: |
~Thread() override = default; |
- static std::unique_ptr<Thread> Create(SchedulerWorker* outer) { |
- std::unique_ptr<Thread> thread(new Thread(outer)); |
+ static std::unique_ptr<Thread> Create(scoped_refptr<SchedulerWorker> outer) { |
+ std::unique_ptr<Thread> thread(new Thread(std::move(outer))); |
thread->Initialize(); |
if (thread->thread_handle_.is_null()) |
return nullptr; |
@@ -38,7 +38,7 @@ class SchedulerWorker::Thread : public PlatformThread::Delegate { |
// Set if this thread was detached. |
std::unique_ptr<Thread> detached_thread; |
- outer_->delegate_->OnMainEntry(outer_); |
+ outer_->delegate_->OnMainEntry(outer_.get()); |
// A SchedulerWorker starts out waiting for work. |
WaitForWork(); |
@@ -51,8 +51,7 @@ class SchedulerWorker::Thread : public PlatformThread::Delegate { |
} |
#endif |
- while (!outer_->task_tracker_->IsShutdownComplete() && |
- !outer_->should_exit_for_testing_.IsSet()) { |
+ while (!outer_->ShouldExit()) { |
DCHECK(outer_); |
#if defined(OS_MACOSX) |
@@ -62,10 +61,11 @@ class SchedulerWorker::Thread : public PlatformThread::Delegate { |
UpdateThreadPriority(GetDesiredThreadPriority()); |
// Get the sequence containing the next task to execute. |
- scoped_refptr<Sequence> sequence = outer_->delegate_->GetWork(outer_); |
+ scoped_refptr<Sequence> sequence = |
+ outer_->delegate_->GetWork(outer_.get()); |
if (!sequence) { |
- if (outer_->delegate_->CanDetach(outer_)) { |
- detached_thread = outer_->Detach(); |
+ if (outer_->delegate_->CanDetach(outer_.get())) { |
+ detached_thread = outer_->DetachThreadObject(DetachNotify::DELEGATE); |
if (detached_thread) { |
outer_ = nullptr; |
DCHECK_EQ(detached_thread.get(), this); |
@@ -106,6 +106,12 @@ class SchedulerWorker::Thread : public PlatformThread::Delegate { |
// stuck forever. |
DCHECK(!detached_thread || !IsWakeUpPending()) << |
"This thread was detached and woken up at the same time."; |
+ |
+ // To maintain the condition that the thread always cleans itself up except |
+ // in Join(), we go ahead and grab the thread object here. We don't want to |
gab
2017/02/21 19:01:04
What makes Join() not hit this code?
robliao
2017/02/21 22:26:57
Join() nulls out |thread_| which means DetachThrea
gab
2017/02/22 18:02:23
I see and JoinForTesting() also checks for null |t
robliao
2017/02/22 20:43:39
I've enumerated the conditions, but we should keep
|
+ // notify the delegate since it's not a detachment. |
+ if (!detached_thread) |
+ detached_thread = outer_->DetachThreadObject(DetachNotify::SILENT); |
} |
void Join() { PlatformThread::Join(thread_handle_); } |
@@ -115,8 +121,8 @@ class SchedulerWorker::Thread : public PlatformThread::Delegate { |
bool IsWakeUpPending() { return wake_up_event_.IsSignaled(); } |
private: |
- Thread(SchedulerWorker* outer) |
- : outer_(outer), |
+ Thread(scoped_refptr<SchedulerWorker> outer) |
+ : outer_(std::move(outer)), |
wake_up_event_(WaitableEvent::ResetPolicy::MANUAL, |
WaitableEvent::InitialState::NOT_SIGNALED), |
current_thread_priority_(GetDesiredThreadPriority()) { |
@@ -175,7 +181,7 @@ class SchedulerWorker::Thread : public PlatformThread::Delegate { |
PlatformThreadHandle thread_handle_; |
- SchedulerWorker* outer_; |
+ scoped_refptr<SchedulerWorker> outer_; |
// Event signaled to wake up this thread. |
WaitableEvent wake_up_event_; |
@@ -187,15 +193,15 @@ class SchedulerWorker::Thread : public PlatformThread::Delegate { |
DISALLOW_COPY_AND_ASSIGN(Thread); |
}; |
-std::unique_ptr<SchedulerWorker> SchedulerWorker::Create( |
+scoped_refptr<SchedulerWorker> SchedulerWorker::Create( |
ThreadPriority priority_hint, |
std::unique_ptr<Delegate> delegate, |
TaskTracker* task_tracker, |
InitialState initial_state, |
SchedulerBackwardCompatibility backward_compatibility) { |
- auto worker = |
- WrapUnique(new SchedulerWorker(priority_hint, std::move(delegate), |
- task_tracker, backward_compatibility)); |
+ scoped_refptr<SchedulerWorker> worker( |
+ new SchedulerWorker(priority_hint, std::move(delegate), task_tracker, |
+ backward_compatibility)); |
// Creation happens before any other thread can reference this one, so no |
// synchronization is necessary. |
if (initial_state == SchedulerWorker::InitialState::ALIVE) { |
@@ -208,17 +214,10 @@ std::unique_ptr<SchedulerWorker> SchedulerWorker::Create( |
return worker; |
} |
-SchedulerWorker::~SchedulerWorker() { |
- // It is unexpected for |thread_| to be alive and for SchedulerWorker to |
- // destroy since SchedulerWorker owns the delegate needed by |thread_|. |
- // For testing, this generally means JoinForTesting was not called. |
- DCHECK(!thread_); |
-} |
- |
void SchedulerWorker::WakeUp() { |
AutoSchedulerLock auto_lock(thread_lock_); |
- DCHECK(!should_exit_for_testing_.IsSet()); |
+ DCHECK(!join_called_for_testing_.IsSet()); |
if (!thread_) |
CreateThreadAssertSynchronized(); |
@@ -228,8 +227,8 @@ void SchedulerWorker::WakeUp() { |
} |
void SchedulerWorker::JoinForTesting() { |
- DCHECK(!should_exit_for_testing_.IsSet()); |
- should_exit_for_testing_.Set(); |
+ DCHECK(!join_called_for_testing_.IsSet()); |
+ join_called_for_testing_.Set(); |
std::unique_ptr<Thread> thread; |
@@ -238,7 +237,7 @@ void SchedulerWorker::JoinForTesting() { |
if (thread_) { |
// Make sure the thread is awake. It will see that |
- // |should_exit_for_testing_| is set and exit shortly after. |
+ // |join_called_for_testing_| is set and exit shortly after. |
thread_->WakeUp(); |
thread = std::move(thread_); |
} |
@@ -253,6 +252,18 @@ bool SchedulerWorker::ThreadAliveForTesting() const { |
return !!thread_; |
} |
+void SchedulerWorker::Cleanup() { |
+ // should_exit_ is synchronized with thread_ for writes here so that we can |
gab
2017/02/21 19:01:04
|should_exit_|
|thread_|
robliao
2017/02/21 22:26:57
Done.
|
+ // maintain access |thread_| for wakeup. Otherwise, the thread may take away |
gab
2017/02/21 19:01:04
"maintain alive" or just "access" ?
robliao
2017/02/21 22:26:57
Lost a preposition there. Added a "to" -> maintain
|
+ // |thread_| for destruction. |
+ AutoSchedulerLock auto_lock(thread_lock_); |
+ DCHECK(!should_exit_.IsSet()); |
+ if (thread_) { |
+ should_exit_.Set(); |
+ thread_->WakeUp(); |
+ } |
+} |
+ |
SchedulerWorker::SchedulerWorker( |
ThreadPriority priority_hint, |
std::unique_ptr<Delegate> delegate, |
@@ -270,12 +281,20 @@ SchedulerWorker::SchedulerWorker( |
DCHECK(task_tracker_); |
} |
-std::unique_ptr<SchedulerWorker::Thread> SchedulerWorker::Detach() { |
+SchedulerWorker::~SchedulerWorker() { |
+ // It is unexpected for |thread_| to be alive and for SchedulerWorker to |
+ // destroy since SchedulerWorker owns the delegate needed by |thread_|. |
+ // For testing, this generally means JoinForTesting was not called. |
+ DCHECK(!thread_); |
+} |
+ |
+std::unique_ptr<SchedulerWorker::Thread> SchedulerWorker::DetachThreadObject( |
+ DetachNotify detach_notify) { |
AutoSchedulerLock auto_lock(thread_lock_); |
// Do not detach if the thread is being joined. |
if (!thread_) { |
- DCHECK(should_exit_for_testing_.IsSet()); |
+ DCHECK(join_called_for_testing_.IsSet()); |
return nullptr; |
} |
@@ -285,9 +304,11 @@ std::unique_ptr<SchedulerWorker::Thread> SchedulerWorker::Detach() { |
if (thread_->IsWakeUpPending()) |
return nullptr; |
- // Call OnDetach() within the scope of |thread_lock_| to prevent the delegate |
- // from being used concurrently from an old and a new thread. |
- delegate_->OnDetach(); |
+ if (detach_notify == DetachNotify::DELEGATE) { |
+ // Call OnDetach() within the scope of |thread_lock_| to prevent the |
+ // delegate from being used concurrently from an old and a new thread. |
+ delegate_->OnDetach(); |
+ } |
return std::move(thread_); |
} |
@@ -301,5 +322,16 @@ void SchedulerWorker::CreateThreadAssertSynchronized() { |
CreateThread(); |
} |
+bool SchedulerWorker::ShouldExit() { |
+ bool should_exit = |
+ task_tracker_->IsShutdownComplete() || join_called_for_testing_.IsSet(); |
gab
2017/02/22 18:02:24
inline below
robliao
2017/02/22 20:43:39
Done.
|
+ if (should_exit) |
+ return true; |
+ |
+ // We don't need to acquire |thread_lock_| here as the thread will acquire it |
+ // on its own to perform changes to thread_. |
gab
2017/02/22 18:02:24
|thread_|
robliao
2017/02/22 20:43:39
Removed
|
+ return should_exit_.IsSet(); |
gab
2017/02/22 18:02:23
It's unclear why this is checked separately now (i
robliao
2017/02/22 20:43:39
Done.
|
+} |
+ |
} // namespace internal |
} // namespace base |