Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2628313004:
Add TaskScheduler::JoinForTesting(). (Closed)
DescriptionAdd TaskScheduler::JoinForTesting().
Before this CL, there was no way to unregister a TaskScheduler
registered by TaskScheduler::CreateAndSetSimpleTaskScheduler()
(TaskSchedulerImpl::JoinForTesting() must be called before deleting a
TaskSchedulerImpl, but can only be called when the suggested reclaim
time is TimeDelta::Max()). This is impractical when unit testing a
class that calls TaskScheduler::CreateAndSetSimpleTaskScheduler()
(came up in https://codereview.chromium.org/2610493002/).
This CL:
- Allows TaskSchedulerImpl::JoinForTesting() to be called on a
TaskSchedulerImpl instance whose suggested reclaim time is not
TimeDelta::Max().
- Adds TaskScheduler::JoinForTesting(), so that no cast is required
to join the current TaskScheduler instance.
- Documents that TaskScheduler::JoinForTesting() must be called
before destroying a TaskScheduler (so that callers don't have to
look at task_scheduler_impl.h to learn that).
BUG=553459
Review-Url: https://codereview.chromium.org/2628313004
Cr-Commit-Position: refs/heads/master@{#445915}
Committed: https://chromium.googlesource.com/chromium/src/+/7bba05ed69f2a2fdf3aca587dec0c9f148bdc469
Patch Set 1 #Patch Set 2 : fix test error #
Total comments: 11
Patch Set 3 : CR robliao #13 #
Total comments: 7
Patch Set 4 : CR gab #18 #Patch Set 5 : CR robliao #26 (DisallowWorkerDetachment) #Patch Set 6 : self-review #Patch Set 7 : self-review #
Total comments: 4
Patch Set 8 : CR robliao #34 (reword comments) #Patch Set 9 : self-review #
Messages
Total messages: 45 (25 generated)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
On 2017/01/13 19:25:52, fdoray wrote: > PTAL lg at first sight but TaskScheduler test is failing?
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/13 23:45:48, gab wrote: > On 2017/01/13 19:25:52, fdoray wrote: > > PTAL > > lg at first sight but TaskScheduler test is failing? Fixed. PTAnL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2628313004/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2628313004/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker.cc:240: // Join outside the lock. Nit: Remove this comment as it is self evident. https://codereview.chromium.org/2628313004/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker.cc:242: thread->Join(); Previously, JoinForTesting holding the lock would ensure that thread_ was valid for the entirety of the join. Now, WakeUp could plausibly be called during the join, creating a new thread in the process. Seems like WakeUp should check should_exit_for_testing_? https://codereview.chromium.org/2628313004/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.h (left): https://codereview.chromium.org/2628313004/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.h:106: void DisallowWorkerDetachmentForTesting(); From what I can tell, we need to disallow detachment to properly handle joining. Without this, workers may detach during the join. This means task scheduler threads could still be running around after TaskScheduler joining (which is undesirable for tests because tests that do leak checking after test completion). If threads detach before joining, they are less likely to hang around because Joining is a "long" process".
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAnL https://codereview.chromium.org/2628313004/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2628313004/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker.cc:240: // Join outside the lock. On 2017/01/17 19:01:27, robliao wrote: > Nit: Remove this comment as it is self evident. Done. https://codereview.chromium.org/2628313004/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker.cc:242: thread->Join(); On 2017/01/17 19:01:27, robliao wrote: > Previously, JoinForTesting holding the lock would ensure that thread_ was valid > for the entirety of the join. > > Now, WakeUp could plausibly be called during the join, creating a new thread in > the process. Seems like WakeUp should check should_exit_for_testing_? Done. https://codereview.chromium.org/2628313004/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.h (left): https://codereview.chromium.org/2628313004/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.h:106: void DisallowWorkerDetachmentForTesting(); On 2017/01/17 19:01:27, robliao wrote: > From what I can tell, we need to disallow detachment to properly handle joining. > > Without this, workers may detach during the join. This means task scheduler > threads could still be running around after TaskScheduler joining (which is > undesirable for tests because tests that do leak checking after test > completion). After JoinForTesting() returns, the thread is either: - Joined, or, - Past "detached_thread = outer_->Detach();" in SchedulerWorker::Thread::ThreadMain() I don't think it matters if a thread is still running after SchedulerWorker::Join has returned if we know that it will exit shortly after and that it can't run tasks anymore. It shouldn't break LSan because LSan simply checks that all memory is referenced by global variables or variables on the stack. There will be unit tests for code that initializes TaskScheduler in a process (required for https://codereview.chromium.org/2610493002/). There should be an easy way to unregister TaskScheduler at the end of these tests (i.e. shouldn't have to call DisallowWorkerDetachmentForTesting() from somewhere "safe"). > > If threads detach before joining, they are less likely to hang around because > Joining is a "long" process".
https://codereview.chromium.org/2628313004/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.h (left): https://codereview.chromium.org/2628313004/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.h:106: void DisallowWorkerDetachmentForTesting(); On 2017/01/17 20:43:13, fdoray wrote: > On 2017/01/17 19:01:27, robliao wrote: > > From what I can tell, we need to disallow detachment to properly handle > joining. > > > > Without this, workers may detach during the join. This means task scheduler > > threads could still be running around after TaskScheduler joining (which is > > undesirable for tests because tests that do leak checking after test > > completion). > > After JoinForTesting() returns, the thread is either: > - Joined, or, > - Past "detached_thread = outer_->Detach();" in > SchedulerWorker::Thread::ThreadMain() > > I don't think it matters if a thread is still running after > SchedulerWorker::Join has returned if we know that it will exit shortly after > and that it can't run tasks anymore. It shouldn't break LSan because LSan simply > checks that all memory is referenced by global variables or variables on the > stack. > > There will be unit tests for code that initializes TaskScheduler in a process > (required for https://codereview.chromium.org/2610493002/). There should be an > easy way to unregister TaskScheduler at the end of these tests (i.e. shouldn't > have to call DisallowWorkerDetachmentForTesting() from somewhere "safe"). > > > > > If threads detach before joining, they are less likely to hang around because > > Joining is a "long" process". > Can JoinForTesting call DisallowWorkerDetachmentForTesting as part of its process? That way callers don't have to worry about this.
https://codereview.chromium.org/2628313004/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2628313004/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker.cc:239: // Wake-up the thread. It will see that |should_exit_for_testing_| is set s/Wake-up the thread/Make sure the thread is awake/ https://codereview.chromium.org/2628313004/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker.cc:240: // and exit sortly after. s/sortly/shortly/ https://codereview.chromium.org/2628313004/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2628313004/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:43: // returned. ScopedTaskScheduler doesn't honor the second sentence here so I guess it's not quite correct (i.e. it's correct for TaskSchedulerImpl but not all TaskSchedulers..?)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAnL https://codereview.chromium.org/2628313004/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.h (left): https://codereview.chromium.org/2628313004/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.h:106: void DisallowWorkerDetachmentForTesting(); On 2017/01/17 21:09:03, robliao wrote: > On 2017/01/17 20:43:13, fdoray wrote: > > On 2017/01/17 19:01:27, robliao wrote: > > > From what I can tell, we need to disallow detachment to properly handle > > joining. > > > > > > Without this, workers may detach during the join. This means task scheduler > > > threads could still be running around after TaskScheduler joining (which is > > > undesirable for tests because tests that do leak checking after test > > > completion). > > > > After JoinForTesting() returns, the thread is either: > > - Joined, or, > > - Past "detached_thread = outer_->Detach();" in > > SchedulerWorker::Thread::ThreadMain() > > > > I don't think it matters if a thread is still running after > > SchedulerWorker::Join has returned if we know that it will exit shortly after > > and that it can't run tasks anymore. It shouldn't break LSan because LSan > simply > > checks that all memory is referenced by global variables or variables on the > > stack. > > > > There will be unit tests for code that initializes TaskScheduler in a process > > (required for https://codereview.chromium.org/2610493002/). There should be an > > easy way to unregister TaskScheduler at the end of these tests (i.e. shouldn't > > have to call DisallowWorkerDetachmentForTesting() from somewhere "safe"). > > > > > > > > If threads detach before joining, they are less likely to hang around > because > > > Joining is a "long" process". > > > > Can JoinForTesting call DisallowWorkerDetachmentForTesting as part of its > process? That way callers don't have to worry about this. The hard part is making sure that DisallowWorkerDetachmentForTesting is called at the right time (see https://cs.chromium.org/chromium/src/base/task_scheduler/scheduler_worker_poo...). With the current CL, you can call JoinForTesting() whenever you want. https://codereview.chromium.org/2628313004/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2628313004/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker.cc:239: // Wake-up the thread. It will see that |should_exit_for_testing_| is set On 2017/01/17 21:24:25, gab wrote: > s/Wake-up the thread/Make sure the thread is awake/ Done. https://codereview.chromium.org/2628313004/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker.cc:240: // and exit sortly after. On 2017/01/17 21:24:25, gab wrote: > s/sortly/shortly/ Done. https://codereview.chromium.org/2628313004/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2628313004/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:43: // returned. On 2017/01/17 21:24:25, gab wrote: > ScopedTaskScheduler doesn't honor the second sentence here so I guess it's not > quite correct (i.e. it's correct for TaskSchedulerImpl but not all > TaskSchedulers..?) Changed ScopedTaskScheduler. I don't want callers of Create(Simple|Default)TaskScheduler to have to look at the impl to know that they must call JoinForTesting() before unregistering a TaskScheduler.
lgtm https://codereview.chromium.org/2628313004/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2628313004/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:43: // returned. On 2017/01/17 23:51:00, fdoray wrote: > On 2017/01/17 21:24:25, gab wrote: > > ScopedTaskScheduler doesn't honor the second sentence here so I guess it's not > > quite correct (i.e. it's correct for TaskSchedulerImpl but not all > > TaskSchedulers..?) > > Changed ScopedTaskScheduler. I don't want callers of > Create(Simple|Default)TaskScheduler to have to look at the impl to know that > they must call JoinForTesting() before unregistering a TaskScheduler. sgtm
robliao: PTAnL
https://codereview.chromium.org/2628313004/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.h (left): https://codereview.chromium.org/2628313004/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.h:106: void DisallowWorkerDetachmentForTesting(); On 2017/01/17 23:51:00, fdoray wrote: > On 2017/01/17 21:09:03, robliao wrote: > > On 2017/01/17 20:43:13, fdoray wrote: > > > On 2017/01/17 19:01:27, robliao wrote: > > > > From what I can tell, we need to disallow detachment to properly handle > > > joining. > > > > > > > > Without this, workers may detach during the join. This means task > scheduler > > > > threads could still be running around after TaskScheduler joining (which > is > > > > undesirable for tests because tests that do leak checking after test > > > > completion). > > > > > > After JoinForTesting() returns, the thread is either: > > > - Joined, or, > > > - Past "detached_thread = outer_->Detach();" in > > > SchedulerWorker::Thread::ThreadMain() > > > > > > I don't think it matters if a thread is still running after > > > SchedulerWorker::Join has returned if we know that it will exit shortly > after > > > and that it can't run tasks anymore. It shouldn't break LSan because LSan > > simply > > > checks that all memory is referenced by global variables or variables on the > > > stack. > > > > > > There will be unit tests for code that initializes TaskScheduler in a > process > > > (required for https://codereview.chromium.org/2610493002/). There should be > an > > > easy way to unregister TaskScheduler at the end of these tests (i.e. > shouldn't > > > have to call DisallowWorkerDetachmentForTesting() from somewhere "safe"). > > > > > > > > > > > If threads detach before joining, they are less likely to hang around > > because > > > > Joining is a "long" process". > > > > > > > Can JoinForTesting call DisallowWorkerDetachmentForTesting as part of its > > process? That way callers don't have to worry about this. > > The hard part is making sure that DisallowWorkerDetachmentForTesting is called > at the right time (see > https://cs.chromium.org/chromium/src/base/task_scheduler/scheduler_worker_poo...). > With the current CL, you can call JoinForTesting() whenever you want. With this code change, you can now call it at any time. Previously, we would could deadlock during the detach call because we hold the lock during the entirety of a Join call. This is no longer the case with this code as we do not hold the lock during Join. This means an attempt to Detach during a Join call will simply return nullptr. An attempt to Join during a detach similar will cause the Join to immediately return.
https://codereview.chromium.org/2628313004/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.h (left): https://codereview.chromium.org/2628313004/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.h:106: void DisallowWorkerDetachmentForTesting(); On 2017/01/18 00:58:19, robliao wrote: > On 2017/01/17 23:51:00, fdoray wrote: > > On 2017/01/17 21:09:03, robliao wrote: > > > On 2017/01/17 20:43:13, fdoray wrote: > > > > On 2017/01/17 19:01:27, robliao wrote: > > > > > From what I can tell, we need to disallow detachment to properly handle > > > > joining. > > > > > > > > > > Without this, workers may detach during the join. This means task > > scheduler > > > > > threads could still be running around after TaskScheduler joining (which > > is > > > > > undesirable for tests because tests that do leak checking after test > > > > > completion). > > > > > > > > After JoinForTesting() returns, the thread is either: > > > > - Joined, or, > > > > - Past "detached_thread = outer_->Detach();" in > > > > SchedulerWorker::Thread::ThreadMain() > > > > > > > > I don't think it matters if a thread is still running after > > > > SchedulerWorker::Join has returned if we know that it will exit shortly > > after > > > > and that it can't run tasks anymore. It shouldn't break LSan because LSan > > > simply > > > > checks that all memory is referenced by global variables or variables on > the > > > > stack. > > > > > > > > There will be unit tests for code that initializes TaskScheduler in a > > process > > > > (required for https://codereview.chromium.org/2610493002/). There should > be > > an > > > > easy way to unregister TaskScheduler at the end of these tests (i.e. > > shouldn't > > > > have to call DisallowWorkerDetachmentForTesting() from somewhere "safe"). > > > > > > > > > > > > > > If threads detach before joining, they are less likely to hang around > > > because > > > > > Joining is a "long" process". > > > > > > > > > > Can JoinForTesting call DisallowWorkerDetachmentForTesting as part of its > > > process? That way callers don't have to worry about this. > > > > The hard part is making sure that DisallowWorkerDetachmentForTesting is called > > at the right time (see > > > https://cs.chromium.org/chromium/src/base/task_scheduler/scheduler_worker_poo...). > > With the current CL, you can call JoinForTesting() whenever you want. > > With this code change, you can now call it at any time. > > Previously, we would could deadlock during the detach call because we hold the > lock during the entirety of a Join call. This is no longer the case with this > code as we do not hold the lock during Join. This means an attempt to Detach > during a Join call will simply return nullptr. > > An attempt to Join during a detach similar will cause the Join to immediately > return. What would be the benefit of calling DisallowWorkerDetachmentForTesting(); from JoinForTesting()? JoinForTesting() already prevents detachment when it takes ownership of |thread_|.
https://codereview.chromium.org/2628313004/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.h (left): https://codereview.chromium.org/2628313004/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.h:106: void DisallowWorkerDetachmentForTesting(); On 2017/01/19 16:37:40, fdoray wrote: > On 2017/01/18 00:58:19, robliao wrote: > > On 2017/01/17 23:51:00, fdoray wrote: > > > On 2017/01/17 21:09:03, robliao wrote: > > > > On 2017/01/17 20:43:13, fdoray wrote: > > > > > On 2017/01/17 19:01:27, robliao wrote: > > > > > > From what I can tell, we need to disallow detachment to properly > handle > > > > > joining. > > > > > > > > > > > > Without this, workers may detach during the join. This means task > > > scheduler > > > > > > threads could still be running around after TaskScheduler joining > (which > > > is > > > > > > undesirable for tests because tests that do leak checking after test > > > > > > completion). > > > > > > > > > > After JoinForTesting() returns, the thread is either: > > > > > - Joined, or, > > > > > - Past "detached_thread = outer_->Detach();" in > > > > > SchedulerWorker::Thread::ThreadMain() > > > > > > > > > > I don't think it matters if a thread is still running after > > > > > SchedulerWorker::Join has returned if we know that it will exit shortly > > > after > > > > > and that it can't run tasks anymore. It shouldn't break LSan because > LSan > > > > simply > > > > > checks that all memory is referenced by global variables or variables on > > the > > > > > stack. > > > > > > > > > > There will be unit tests for code that initializes TaskScheduler in a > > > process > > > > > (required for https://codereview.chromium.org/2610493002/). There should > > be > > > an > > > > > easy way to unregister TaskScheduler at the end of these tests (i.e. > > > shouldn't > > > > > have to call DisallowWorkerDetachmentForTesting() from somewhere > "safe"). > > > > > > > > > > > > > > > > > If threads detach before joining, they are less likely to hang around > > > > because > > > > > > Joining is a "long" process". > > > > > > > > > > > > > Can JoinForTesting call DisallowWorkerDetachmentForTesting as part of its > > > > process? That way callers don't have to worry about this. > > > > > > The hard part is making sure that DisallowWorkerDetachmentForTesting is > called > > > at the right time (see > > > > > > https://cs.chromium.org/chromium/src/base/task_scheduler/scheduler_worker_poo...). > > > With the current CL, you can call JoinForTesting() whenever you want. > > > > With this code change, you can now call it at any time. > > > > Previously, we would could deadlock during the detach call because we hold the > > lock during the entirety of a Join call. This is no longer the case with this > > code as we do not hold the lock during Join. This means an attempt to Detach > > during a Join call will simply return nullptr. > > > > An attempt to Join during a detach similar will cause the Join to immediately > > return. > > What would be the benefit of calling DisallowWorkerDetachmentForTesting(); from > JoinForTesting()? JoinForTesting() already prevents detachment when it takes > ownership of |thread_|. The intent here would be to first do a disallow detachment pass first and then perform the subsequent joins. This reduces the likelihood that threads are detaching during the joining of any single thread and allows us to join and clear threads than we otherwise would have.
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
robliao@: PTAnL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
lgtm + comments. Thanks! https://codereview.chromium.org/2628313004/diff/120001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker.h (right): https://codereview.chromium.org/2628313004/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker.h:119: // Note: A thread which detaches itself before JoinForTesting() is called may This might be clearer: A thread that detaches before JoinForTesting() is called may still be running after JoinForTesting() returns. https://codereview.chromium.org/2628313004/diff/120001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_pool_impl.h (right): https://codereview.chromium.org/2628313004/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_pool_impl.h:102: // reduce the chances of having a thread that detaches itself before Possible revision: If the suggested reclaim time is not TimeDelta::Max(), the test must call this before JoinForTesting() to reduce the chance of thread detachment during the process of joining all of the threads, and as a result, threads running after JoinForTesting().
https://codereview.chromium.org/2628313004/diff/120001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker.h (right): https://codereview.chromium.org/2628313004/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker.h:119: // Note: A thread which detaches itself before JoinForTesting() is called may On 2017/01/24 18:20:18, robliao wrote: > This might be clearer: A thread that detaches before JoinForTesting() is called > may still be running after JoinForTesting() returns. Done. https://codereview.chromium.org/2628313004/diff/120001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_pool_impl.h (right): https://codereview.chromium.org/2628313004/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_pool_impl.h:102: // reduce the chances of having a thread that detaches itself before On 2017/01/24 18:20:18, robliao wrote: > Possible revision: > If the suggested reclaim time is not TimeDelta::Max(), the test must call this > before JoinForTesting() to reduce the chance of thread detachment during the > process of joining all of the threads, and as a result, threads running after > JoinForTesting(). Done.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, robliao@chromium.org Link to the patchset: https://codereview.chromium.org/2628313004/#ps160001 (title: "self-review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by fdoray@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1485305698882920,
"parent_rev": "16bf4cd0ddcf07b44549008e585e029e6f40bcbc", "commit_rev":
"7bba05ed69f2a2fdf3aca587dec0c9f148bdc469"}
Message was sent while issue was closed.
Description was changed from ========== Add TaskScheduler::JoinForTesting(). Before this CL, there was no way to unregister a TaskScheduler registered by TaskScheduler::CreateAndSetSimpleTaskScheduler() (TaskSchedulerImpl::JoinForTesting() must be called before deleting a TaskSchedulerImpl, but can only be called when the suggested reclaim time is TimeDelta::Max()). This is impractical when unit testing a class that calls TaskScheduler::CreateAndSetSimpleTaskScheduler() (came up in https://codereview.chromium.org/2610493002/). This CL: - Allows TaskSchedulerImpl::JoinForTesting() to be called on a TaskSchedulerImpl instance whose suggested reclaim time is not TimeDelta::Max(). - Adds TaskScheduler::JoinForTesting(), so that no cast is required to join the current TaskScheduler instance. - Documents that TaskScheduler::JoinForTesting() must be called before destroying a TaskScheduler (so that callers don't have to look at task_scheduler_impl.h to learn that). BUG=553459 ========== to ========== Add TaskScheduler::JoinForTesting(). Before this CL, there was no way to unregister a TaskScheduler registered by TaskScheduler::CreateAndSetSimpleTaskScheduler() (TaskSchedulerImpl::JoinForTesting() must be called before deleting a TaskSchedulerImpl, but can only be called when the suggested reclaim time is TimeDelta::Max()). This is impractical when unit testing a class that calls TaskScheduler::CreateAndSetSimpleTaskScheduler() (came up in https://codereview.chromium.org/2610493002/). This CL: - Allows TaskSchedulerImpl::JoinForTesting() to be called on a TaskSchedulerImpl instance whose suggested reclaim time is not TimeDelta::Max(). - Adds TaskScheduler::JoinForTesting(), so that no cast is required to join the current TaskScheduler instance. - Documents that TaskScheduler::JoinForTesting() must be called before destroying a TaskScheduler (so that callers don't have to look at task_scheduler_impl.h to learn that). BUG=553459 Review-Url: https://codereview.chromium.org/2628313004 Cr-Commit-Position: refs/heads/master@{#445915} Committed: https://chromium.googlesource.com/chromium/src/+/7bba05ed69f2a2fdf3aca587dec0... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/7bba05ed69f2a2fdf3aca587dec0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
