Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2891363005:
Add ScopedTaskEnvironment::ExecutionControlMode. (Closed)
DescriptionAdd ScopedTaskEnvironment::ExecutionControlMode.
This enum controls whether tasks posted within the scope of a
ScopedTaskEnvironment can run as they are posted or have to wait
until a call to RunUntilIdle() to run.
BUG=724077
TBR=gab@chromium.org
Review-Url: https://codereview.chromium.org/2891363005
Cr-Original-Commit-Position: refs/heads/master@{#473925}
Committed: https://chromium.googlesource.com/chromium/src/+/fc7b5ec521b29a97e63a7cda5921aa5ed793679a
Review-Url: https://codereview.chromium.org/2891363005
Cr-Commit-Position: refs/heads/master@{#475392}
Committed: https://chromium.googlesource.com/chromium/src/+/b199f1bef099a0b34576f10fd57e713551e39349
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : self-review #
Total comments: 12
Patch Set 4 : CR-robliao-7-comment #Patch Set 5 : fix-build-error #
Total comments: 22
Patch Set 6 : fix build error #Patch Set 7 : CR gab #Patch Set 8 : self-review #
Total comments: 6
Patch Set 9 : rebase #Patch Set 10 : CR-gab-31 #
Depends on Patchset: Messages
Total messages: 63 (37 generated)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm % comment https://codereview.chromium.org/2891363005/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.h (right): https://codereview.chromium.org/2891363005/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.h:49: // |name| is used to label threads and histograms. Comment on |task_tracker|. |task_tracker| can be used for tests that need more execution control. By default, you get the production task tracker.
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...
https://codereview.chromium.org/2891363005/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.h (right): https://codereview.chromium.org/2891363005/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.h:49: // |name| is used to label threads and histograms. On 2017/05/19 20:14:47, robliao wrote: > Comment on |task_tracker|. > |task_tracker| can be used for tests that need more execution control. By > default, you get the production task tracker. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Description was changed from ========== Add ScopedTaskEnvironment::ExecutionControlMode. This enum controls whether tasks posted within the scope of a ScopedTaskEnvironment can run as they are posted or have to wait until a call to RunUntilIdle() to run. BUG=724077 ========== to ========== Add ScopedTaskEnvironment::ExecutionControlMode. This enum controls whether tasks posted within the scope of a ScopedTaskEnvironment can run as they are posted or have to wait until a call to RunUntilIdle() to run. BUG=724077 TBR=gab@chromium.org ==========
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org Link to the patchset: https://codereview.chromium.org/2891363005/#ps100001 (title: "fix build error")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Very nice :), comments below (split across two patch sets as I'd started on Friday...) https://codereview.chromium.org/2891363005/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2891363005/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:392: return subtle::Acquire_Load(&num_pending_undelayed_tasks_); Reason to Acquire_Load? https://codereview.chromium.org/2891363005/diff/40001/base/test/scoped_task_e... File base/test/scoped_task_environment.h (right): https://codereview.chromium.org/2891363005/diff/40001/base/test/scoped_task_e... base/test/scoped_task_environment.h:66: enum class ExecutionControlMode { I'd say "ExecutionMode" is enough (especially because enum class force it to be spelled everywhere) https://codereview.chromium.org/2891363005/diff/40001/base/test/scoped_task_e... base/test/scoped_task_environment.h:67: // Tasks are queued and executed when RunUntilIdle() is called. only executed ^^^^ is explicitly called ^^^^^^^^^^ https://codereview.chromium.org/2891363005/diff/40001/base/test/scoped_task_e... base/test/scoped_task_environment.h:69: // Tasks run as they are posted. + "RunUntilIdle() can still be used to block until done." https://codereview.chromium.org/2891363005/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.h (right): https://codereview.chromium.org/2891363005/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.h:42: using TaskTrackerType = TaskTrackerImpl? Also, move this to an early private: section? https://codereview.chromium.org/2891363005/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.h:51: // production TaskTracker. By default, the production TaskTracker is used. https://codereview.chromium.org/2891363005/diff/80001/base/test/scoped_task_e... File base/test/scoped_task_environment.cc (right): https://codereview.chromium.org/2891363005/diff/80001/base/test/scoped_task_e... base/test/scoped_task_environment.cc:46: void SetTaskQueueEmptyClosure(OnceClosure task_queue_empty_closure); RegisterOnQueueEmptyClosure() ? https://codereview.chromium.org/2891363005/diff/80001/base/test/scoped_task_e... base/test/scoped_task_environment.cc:107: CHECK(task_tracker_->DisallowRunTasks()); #include "base/logging.h" https://codereview.chromium.org/2891363005/diff/80001/base/test/scoped_task_e... base/test/scoped_task_environment.cc:141: PostTask(FROM_HERE, BindOnce(&DoNothing)); bind_helpers.h https://codereview.chromium.org/2891363005/diff/80001/base/test/scoped_task_e... base/test/scoped_task_environment.cc:165: task_observer.task_count() == 1) { This will make the conditional true even if DisallowRunTasks() failed in QUEUED mode while only the closure ran, from the comment that doesn't appear to be the intent? https://codereview.chromium.org/2891363005/diff/80001/base/test/scoped_task_e... base/test/scoped_task_environment.cc:166: return; break; instead of return; (same thing but highlights intent better and is friendlier to eventual code being added after the loop) https://codereview.chromium.org/2891363005/diff/80001/base/test/scoped_task_e... base/test/scoped_task_environment.cc:223: // other than the current one is running. This feels backwards as the check for running is 0, the check for 1 is w.r.t. the undelayed tasks. https://codereview.chromium.org/2891363005/diff/80001/base/test/scoped_task_e... base/test/scoped_task_environment.cc:228: FROM_HERE, std::move(task_queue_empty_closure_)); As of r473378, the QuitClosure is thread-safe :) so you don't need to manually bounce it :) https://codereview.chromium.org/2891363005/diff/80001/base/test/scoped_task_e... File base/test/scoped_task_environment_unittest.cc (right): https://codereview.chromium.org/2891363005/diff/80001/base/test/scoped_task_e... base/test/scoped_task_environment_unittest.cc:102: // Verify that a task posted to an ExecutionControlMode::QUEUED ASYNC https://codereview.chromium.org/2891363005/diff/80001/base/test/scoped_task_e... base/test/scoped_task_environment_unittest.cc:117: // Verify that a task posted to an ExecutionControlMode::QUEUED ASYNC
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495550400849020,
"parent_rev": "85c062fd6ce24e1c031c035c71a28b42b00d9934", "commit_rev":
"fc7b5ec521b29a97e63a7cda5921aa5ed793679a"}
Message was sent while issue was closed.
Description was changed from ========== Add ScopedTaskEnvironment::ExecutionControlMode. This enum controls whether tasks posted within the scope of a ScopedTaskEnvironment can run as they are posted or have to wait until a call to RunUntilIdle() to run. BUG=724077 TBR=gab@chromium.org ========== to ========== Add ScopedTaskEnvironment::ExecutionControlMode. This enum controls whether tasks posted within the scope of a ScopedTaskEnvironment can run as they are posted or have to wait until a call to RunUntilIdle() to run. BUG=724077 TBR=gab@chromium.org Review-Url: https://codereview.chromium.org/2891363005 Cr-Commit-Position: refs/heads/master@{#473925} Committed: https://chromium.googlesource.com/chromium/src/+/fc7b5ec521b29a97e63a7cda5921... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/fc7b5ec521b29a97e63a7cda5921...
Message was sent while issue was closed.
On 2017/05/23 16:13:33, commit-bot: I haz the power wrote: > Committed patchset #6 (id:100001) as > https://chromium.googlesource.com/chromium/src/+/fc7b5ec521b29a97e63a7cda5921... Whoops, hadn't noticed CQ, let's do a follow-up CL for my comments. At least the API is usable for people hitting this issue :)
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2903633003/ by mek@chromium.org. The reason for reverting is: Is causing consistent test failures on the main waterfall in net_unittests URLRequestSimpleJobTest.CancelAfterFirstReadStarted, for example: https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.mac%2FMac10.9_T... [1130:771:0523/094643.091773:30621298248173:FATAL:scoped_task_environment.cc(178)] Check failed: !task_queue_empty_closure_. 0 net_unittests 0x0000000102d4603c base::debug::StackTrace::StackTrace(unsigned long) + 28 1 net_unittests 0x0000000102d5db70 logging::LogMessage::~LogMessage() + 224 2 net_unittests 0x000000010302df97 base::test::ScopedTaskEnvironment::RunUntilIdle() + 183 3 net_unittests 0x000000010205f824 net::URLRequestSimpleJobTest_CancelAfterFirstReadStarted_Test::TestBody() + 260 4 net_unittests 0x00000001022adf56 testing::Test::Run() + 246 5 net_unittests 0x00000001022ae9f0 testing::TestInfo::Run() + 288 6 net_unittests 0x00000001022aef57 testing::TestCase::Run() + 263 7 net_unittests 0x00000001022b5057 testing::internal::UnitTestImpl::RunAllTests() + 871 8 net_unittests 0x00000001022b4cc3 testing::UnitTest::Run() + 163 9 net_unittests 0x00000001030309b3 base::TestSuite::Run() + 163 10 net_unittests 0x000000010303e176 base::LaunchUnitTests(int, char**, base::Callback<int (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) + 134 11 net_unittests 0x0000000101fdf008 main + 440 12 libdyld.dylib 0x00007fff8d4495fd start + 1 13 ??? 0x0000000000000007 0x0 + 7 .
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) identified this CL at revision 473925 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
Description was changed from ========== Add ScopedTaskEnvironment::ExecutionControlMode. This enum controls whether tasks posted within the scope of a ScopedTaskEnvironment can run as they are posted or have to wait until a call to RunUntilIdle() to run. BUG=724077 TBR=gab@chromium.org Review-Url: https://codereview.chromium.org/2891363005 Cr-Commit-Position: refs/heads/master@{#473925} Committed: https://chromium.googlesource.com/chromium/src/+/fc7b5ec521b29a97e63a7cda5921... ========== to ========== Add ScopedTaskEnvironment::ExecutionControlMode. This enum controls whether tasks posted within the scope of a ScopedTaskEnvironment can run as they are posted or have to wait until a call to RunUntilIdle() to run. BUG=724077 TBR=gab@chromium.org Review-Url: https://codereview.chromium.org/2891363005 Cr-Commit-Position: refs/heads/master@{#473925} Committed: https://chromium.googlesource.com/chromium/src/+/fc7b5ec521b29a97e63a7cda5921... ==========
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
gab@: PTAnL https://codereview.chromium.org/2891363005/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2891363005/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:392: return subtle::Acquire_Load(&num_pending_undelayed_tasks_); On 2017/05/23 15:26:13, gab wrote: > Reason to Acquire_Load? I would have liked to Acquire_NoBarrier, but this function doesn't exist. Cannot return |num_pending_undelayed_tasks_| directly because of https://cs.chromium.org/chromium/src/base/atomicops.h?l=19 https://codereview.chromium.org/2891363005/diff/40001/base/test/scoped_task_e... File base/test/scoped_task_environment.h (right): https://codereview.chromium.org/2891363005/diff/40001/base/test/scoped_task_e... base/test/scoped_task_environment.h:66: enum class ExecutionControlMode { On 2017/05/23 15:26:13, gab wrote: > I'd say "ExecutionMode" is enough (especially because enum class force it to be > spelled everywhere) Done. https://codereview.chromium.org/2891363005/diff/40001/base/test/scoped_task_e... base/test/scoped_task_environment.h:67: // Tasks are queued and executed when RunUntilIdle() is called. On 2017/05/23 15:26:13, gab wrote: > only executed > ^^^^ > > is explicitly called > ^^^^^^^^^^ Done. https://codereview.chromium.org/2891363005/diff/40001/base/test/scoped_task_e... base/test/scoped_task_environment.h:69: // Tasks run as they are posted. On 2017/05/23 15:26:13, gab wrote: > + "RunUntilIdle() can still be used to block until done." Done. https://codereview.chromium.org/2891363005/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.h (right): https://codereview.chromium.org/2891363005/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.h:42: using TaskTrackerType = On 2017/05/23 15:26:13, gab wrote: > TaskTrackerImpl? Done. > Also, move this to an early private: section? Cannot be private because it's used by ScopedTaskEnvironment. https://codereview.chromium.org/2891363005/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.h:51: // production TaskTracker. On 2017/05/23 15:26:13, gab wrote: > By default, the production TaskTracker is used. Done. https://codereview.chromium.org/2891363005/diff/80001/base/test/scoped_task_e... File base/test/scoped_task_environment.cc (right): https://codereview.chromium.org/2891363005/diff/80001/base/test/scoped_task_e... base/test/scoped_task_environment.cc:46: void SetTaskQueueEmptyClosure(OnceClosure task_queue_empty_closure); On 2017/05/23 15:26:13, gab wrote: > RegisterOnQueueEmptyClosure() ? Done. https://codereview.chromium.org/2891363005/diff/80001/base/test/scoped_task_e... base/test/scoped_task_environment.cc:107: CHECK(task_tracker_->DisallowRunTasks()); On 2017/05/23 15:26:13, gab wrote: > #include "base/logging.h" Done. https://codereview.chromium.org/2891363005/diff/80001/base/test/scoped_task_e... base/test/scoped_task_environment.cc:141: PostTask(FROM_HERE, BindOnce(&DoNothing)); On 2017/05/23 15:26:13, gab wrote: > bind_helpers.h Done. https://codereview.chromium.org/2891363005/diff/80001/base/test/scoped_task_e... base/test/scoped_task_environment.cc:165: task_observer.task_count() == 1) { On 2017/05/23 15:26:13, gab wrote: > This will make the conditional true even if DisallowRunTasks() failed in QUEUED > mode while only the closure ran, from the comment that doesn't appear to be the > intent? Done. https://codereview.chromium.org/2891363005/diff/80001/base/test/scoped_task_e... base/test/scoped_task_environment.cc:166: return; On 2017/05/23 15:26:13, gab wrote: > break; instead of return; > > (same thing but highlights intent better and is friendlier to eventual code > being added after the loop) Done. https://codereview.chromium.org/2891363005/diff/80001/base/test/scoped_task_e... base/test/scoped_task_environment.cc:223: // other than the current one is running. On 2017/05/23 15:26:13, gab wrote: > This feels backwards as the check for running is 0, the check for 1 is w.r.t. > the undelayed tasks. Done. https://codereview.chromium.org/2891363005/diff/80001/base/test/scoped_task_e... base/test/scoped_task_environment.cc:228: FROM_HERE, std::move(task_queue_empty_closure_)); On 2017/05/23 15:26:13, gab wrote: > As of r473378, the QuitClosure is thread-safe :) so you don't need to manually > bounce it :) Done. https://codereview.chromium.org/2891363005/diff/80001/base/test/scoped_task_e... File base/test/scoped_task_environment_unittest.cc (right): https://codereview.chromium.org/2891363005/diff/80001/base/test/scoped_task_e... base/test/scoped_task_environment_unittest.cc:102: // Verify that a task posted to an ExecutionControlMode::QUEUED On 2017/05/23 15:26:14, gab wrote: > ASYNC Done. https://codereview.chromium.org/2891363005/diff/80001/base/test/scoped_task_e... base/test/scoped_task_environment_unittest.cc:117: // Verify that a task posted to an ExecutionControlMode::QUEUED On 2017/05/23 15:26:14, gab wrote: > ASYNC Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm w/ comments/questions https://codereview.chromium.org/2891363005/diff/140001/base/test/scoped_task_... File base/test/scoped_task_environment.cc (right): https://codereview.chromium.org/2891363005/diff/140001/base/test/scoped_task_... base/test/scoped_task_environment.cc:167: (execution_control_mode_ == ExecutionMode::ASYNC || != QUEUED (to match comment logic) https://codereview.chromium.org/2891363005/diff/140001/base/test/scoped_task_... base/test/scoped_task_environment.cc:230: GetNumPendingUndelayedTasksForTesting() == 1 && queue_empty_closure_) { Why is the number of pending tasks 1 and not 0? https://codereview.chromium.org/2891363005/diff/140001/base/test/scoped_task_... File base/test/scoped_task_environment_unittest.cc (right): https://codereview.chromium.org/2891363005/diff/140001/base/test/scoped_task_... base/test/scoped_task_environment_unittest.cc:102: // Verify that a task posted to an ExecutionMode::Async ScopedTaskEnvironment ASYNC (caps)
https://codereview.chromium.org/2891363005/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2891363005/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:392: return subtle::Acquire_Load(&num_pending_undelayed_tasks_); On 2017/05/25 19:17:33, fdoray wrote: > On 2017/05/23 15:26:13, gab wrote: > > Reason to Acquire_Load? > > I would have liked to Acquire_NoBarrier, but this function doesn't exist. Cannot > return |num_pending_undelayed_tasks_| directly because of > https://cs.chromium.org/chromium/src/base/atomicops.h?l=19 As mentioned offline there is NoBarrier_Load
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 unchecked by fdoray@chromium.org
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
https://codereview.chromium.org/2891363005/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2891363005/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_tracker.cc:392: return subtle::Acquire_Load(&num_pending_undelayed_tasks_); On 2017/05/25 19:29:29, gab wrote: > On 2017/05/25 19:17:33, fdoray wrote: > > On 2017/05/23 15:26:13, gab wrote: > > > Reason to Acquire_Load? > > > > I would have liked to Acquire_NoBarrier, but this function doesn't exist. > Cannot > > return |num_pending_undelayed_tasks_| directly because of > > https://cs.chromium.org/chromium/src/base/atomicops.h?l=19 > > As mentioned offline there is NoBarrier_Load Done. https://codereview.chromium.org/2891363005/diff/140001/base/test/scoped_task_... File base/test/scoped_task_environment.cc (right): https://codereview.chromium.org/2891363005/diff/140001/base/test/scoped_task_... base/test/scoped_task_environment.cc:167: (execution_control_mode_ == ExecutionMode::ASYNC || On 2017/05/25 19:28:57, gab wrote: > != QUEUED (to match comment logic) Done. https://codereview.chromium.org/2891363005/diff/140001/base/test/scoped_task_... base/test/scoped_task_environment.cc:230: GetNumPendingUndelayedTasksForTesting() == 1 && queue_empty_closure_) { On 2017/05/25 19:28:57, gab wrote: > Why is the number of pending tasks 1 and not 0? The current task is still "pending". The count is decremented after PerformRunTask() returns. https://codereview.chromium.org/2891363005/diff/140001/base/test/scoped_task_... File base/test/scoped_task_environment_unittest.cc (right): https://codereview.chromium.org/2891363005/diff/140001/base/test/scoped_task_... base/test/scoped_task_environment_unittest.cc:102: // Verify that a task posted to an ExecutionMode::Async ScopedTaskEnvironment On 2017/05/25 19:28:57, gab wrote: > ASYNC (caps) Done.
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 unchecked by fdoray@chromium.org
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2891363005/#ps180001 (title: "CR-gab-31")
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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": 180001, "attempt_start_ts": 1496094314452090,
"parent_rev": "0f2a25964b96c4333d57da6d7fb38afef4bd02fc", "commit_rev":
"b199f1bef099a0b34576f10fd57e713551e39349"}
Message was sent while issue was closed.
Description was changed from ========== Add ScopedTaskEnvironment::ExecutionControlMode. This enum controls whether tasks posted within the scope of a ScopedTaskEnvironment can run as they are posted or have to wait until a call to RunUntilIdle() to run. BUG=724077 TBR=gab@chromium.org Review-Url: https://codereview.chromium.org/2891363005 Cr-Commit-Position: refs/heads/master@{#473925} Committed: https://chromium.googlesource.com/chromium/src/+/fc7b5ec521b29a97e63a7cda5921... ========== to ========== Add ScopedTaskEnvironment::ExecutionControlMode. This enum controls whether tasks posted within the scope of a ScopedTaskEnvironment can run as they are posted or have to wait until a call to RunUntilIdle() to run. BUG=724077 TBR=gab@chromium.org Review-Url: https://codereview.chromium.org/2891363005 Cr-Original-Commit-Position: refs/heads/master@{#473925} Committed: https://chromium.googlesource.com/chromium/src/+/fc7b5ec521b29a97e63a7cda5921... Review-Url: https://codereview.chromium.org/2891363005 Cr-Commit-Position: refs/heads/master@{#475392} Committed: https://chromium.googlesource.com/chromium/src/+/b199f1bef099a0b34576f10fd57e... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/b199f1bef099a0b34576f10fd57e...
Message was sent while issue was closed.
Hi fdoray and gab! We've been seeing hangs/timeouts in iOS unittests since about when this CL landed. Does this CL actually change any behavior, or does it just add the enum without enabling the new mode? Is it possible that a broken unittest could be waiting for a task without ever calling RunUntilIdle(), resulting in the task never being run and the test waiting forever?
Message was sent while issue was closed.
On 2017/06/06 15:02:39, rohitrao (ping after 24h) wrote: > Hi fdoray and gab! > > We've been seeing hangs/timeouts in iOS unittests since about when this CL > landed. Does this CL actually change any behavior, or does it just add the enum > without enabling the new mode? > > Is it possible that a broken unittest could be waiting for a task without ever > calling RunUntilIdle(), resulting in the task never being run and the test > waiting forever? This CL is almost a no-op when the new mode isn't enabled. It is *almost* a no-op because MessageLoop task observers are invoked one more time than before when ScopedTaskEnvironment::RunUntilIdle is called. Do you have specific examples of test timeouts?
Message was sent while issue was closed.
On 2017/06/06 17:08:04, fdoray wrote: > On 2017/06/06 15:02:39, rohitrao (ping after 24h) wrote: > > Hi fdoray and gab! > > > > We've been seeing hangs/timeouts in iOS unittests since about when this CL > > landed. Does this CL actually change any behavior, or does it just add the > enum > > without enabling the new mode? > > > > Is it possible that a broken unittest could be waiting for a task without ever > > calling RunUntilIdle(), resulting in the task never being run and the test > > waiting forever? > > This CL is almost a no-op when the new mode isn't enabled. It is *almost* a > no-op because MessageLoop task observers are invoked one more time than before > when ScopedTaskEnvironment::RunUntilIdle is called. > > Do you have specific examples of test timeouts? To close the loop on this, this is tracked in https://bugs.chromium.org/p/chromium/issues/detail?id=728216. The underlying issue turned out to be a race condition in ScopedTaskEnvironment::RunUntilIdle(). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
