Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2839663002:
Instantiate ScopedTaskEnvironment in net unittests. (Closed)
|
Created:
3 years, 8 months ago by fdoray Modified:
3 years, 7 months ago Reviewers:
mmenke CC:
chromium-reviews, cbentzel+watch_chromium.org, bnc+watch_chromium.org, gavinp+disk_chromium.org, fuzzing_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse ScopedTaskEnvironment instead of MessageLoopForIO in net unit tests.
ScopedTaskEnvironment allows usage of ThreadTaskRunnerHandle and
base/task_scheduler/post_task.h within its scope. It should be
instantiated in everytest that uses either of these APIs
(i.e. no test should instantiate a MessageLoop directly).
Switching from MessageLoopForIO to ScopedTaskEnvironment in
NetTestSuite allows Scoped(Async)TaskScheduler to be removed
from individual net unittests.
Motivation for ScopedTaskEnvironment can be found in:
https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3Pkk/edit
BUG=708584
Review-Url: https://codereview.chromium.org/2839663002
Cr-Commit-Position: refs/heads/master@{#471073}
Committed: https://chromium.googlesource.com/chromium/src/+/f33fede528342549ee59c5cc2b8f4f966a3edec8
Patch Set 1 #Patch Set 2 : fix-test-errors #Patch Set 3 : fix-test-errors #Patch Set 4 : fix-test-errors #Patch Set 5 : fix-ios-build #Patch Set 6 : fix-test-error #Patch Set 7 : self-review #
Total comments: 6
Patch Set 8 : rebase-and-CR-mmenke #Patch Set 9 : self-review #
Total comments: 7
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 69 (54 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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 commit-bot@chromium.org
Dry run: 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 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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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 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...)
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 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-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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...
Description was changed from ========== Instantiate ScopedTaskEnvironment in net unittests. BUG= ========== to ========== Use ScopedTaskEnvironment instead of MessageLoopForIO in net unit tests. ScopedTaskEnvironment allows usage of ThreadTaskRunnerHandle and base/task_scheduler/post_task.h within its scope. It should be instantiated in everytest that uses either of these APIs (i.e. no test should instantiate a MessageLoop directly). Motivation for ScopedTaskEnvironment can be found in: https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P... BUG=708584 ==========
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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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 commit-bot@chromium.org
Dry run: 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 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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
fdoray@chromium.org changed reviewers: + mmenke@chromium.org
Please take a look. Thanks.
Description was changed from ========== Use ScopedTaskEnvironment instead of MessageLoopForIO in net unit tests. ScopedTaskEnvironment allows usage of ThreadTaskRunnerHandle and base/task_scheduler/post_task.h within its scope. It should be instantiated in everytest that uses either of these APIs (i.e. no test should instantiate a MessageLoop directly). Motivation for ScopedTaskEnvironment can be found in: https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P... BUG=708584 ========== to ========== Use ScopedTaskEnvironment instead of MessageLoopForIO in net unit tests. ScopedTaskEnvironment allows usage of ThreadTaskRunnerHandle and base/task_scheduler/post_task.h within its scope. It should be instantiated in everytest that uses either of these APIs (i.e. no test should instantiate a MessageLoop directly). Switching from MessageLoopForIO to ScopedTaskEnvironment in NetTestSuite allows Scoped(Async)TaskScheduler to be removed from individual net unittests. Motivation for ScopedTaskEnvironment can be found in: https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P... BUG=708584 ==========
https://codereview.chromium.org/2839663002/diff/120001/net/http/http_network_... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/2839663002/diff/120001/net/http/http_network_... net/http/http_network_transaction_unittest.cc:16792: base::TaskScheduler::GetInstance()->FlushForTesting(); Why are both this and RunUntilIdle needed? https://codereview.chromium.org/2839663002/diff/120001/net/ssl/channel_id_ser... File net/ssl/channel_id_service_unittest.cc (right): https://codereview.chromium.org/2839663002/diff/120001/net/ssl/channel_id_ser... net/ssl/channel_id_service_unittest.cc:330: base::RunLoop().RunUntilIdle(); This means something different than the old code, right? This fails if there's a task that posts a task that does what we're waiting for? https://codereview.chromium.org/2839663002/diff/120001/net/url_request/url_fe... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/2839663002/diff/120001/net/url_request/url_fe... net/url_request/url_fetcher_impl_unittest.cc:537: base::TaskScheduler::GetInstance()->FlushForTesting(); In most of the other tests you do a flush + RunUntilIdle together..Why aren't you doing that here?
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 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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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...
Please take another look https://codereview.chromium.org/2839663002/diff/120001/net/http/http_network_... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/2839663002/diff/120001/net/http/http_network_... net/http/http_network_transaction_unittest.cc:16792: base::TaskScheduler::GetInstance()->FlushForTesting(); On 2017/05/09 15:40:44, mmenke wrote: > Why are both this and RunUntilIdle needed? n/a with latest patch set https://codereview.chromium.org/2839663002/diff/120001/net/ssl/channel_id_ser... File net/ssl/channel_id_service_unittest.cc (right): https://codereview.chromium.org/2839663002/diff/120001/net/ssl/channel_id_ser... net/ssl/channel_id_service_unittest.cc:330: base::RunLoop().RunUntilIdle(); On 2017/05/09 15:40:44, mmenke wrote: > This means something different than the old code, right? This fails if there's > a task that posts a task that does what we're waiting for? Addressed by calling ScopedTaskEnvironment::RunUntilIdle() instead. https://codereview.chromium.org/2839663002/diff/120001/net/url_request/url_fe... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/2839663002/diff/120001/net/url_request/url_fe... net/url_request/url_fetcher_impl_unittest.cc:537: base::TaskScheduler::GetInstance()->FlushForTesting(); On 2017/05/09 15:40:44, mmenke wrote: > In most of the other tests you do a flush + RunUntilIdle together..Why aren't > you doing that here? Because the RunLoop::RunUntilIdle() was not required to make the test pass. Avoided confusion by calling NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() instead. https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe... net/url_request/url_fetcher_impl_unittest.cc:133: task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure()); TaskScheduler tasks run on dedicated thread when they are posted within the scope of a ScopedTaskEnvironment while they run on the main thread when posted within the scope of a ScopedTaskScheduler. Switching from ScopedTaskEnvironment to ScopedTaskScheduler means that OnURLFetchComplete is not called on the main thread and a post task to |task_runner_| is required to quit the run loop on the main thread.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe... net/url_request/url_fetcher_impl_unittest.cc:133: task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure()); On 2017/05/09 21:43:47, fdoray wrote: > TaskScheduler tasks run on dedicated thread when they are posted within the > scope of a ScopedTaskEnvironment while they run on the main thread when posted > within the scope of a ScopedTaskScheduler. Switching from ScopedTaskEnvironment > to ScopedTaskScheduler means that OnURLFetchComplete is not called on the main > thread and a post task to |task_runner_| is required to quit the run loop on the > main thread. I'm not really following, nor do I understand why that affects this test and no others. I'll try to work through it later today, but not sure I'll get to it. Have a number of reviews on my plate today. https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re... File net/url_request/url_request_simple_job_unittest.cc (right): https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re... net/url_request/url_request_simple_job_unittest.cc:224: NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle(); Why do only some RunUntilIdle calls need to be switched? Just skimming over net unittests, there are still a ton of RunUntilIdle calls. Are we changing the behavior of all of them?
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe... net/url_request/url_fetcher_impl_unittest.cc:133: task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure()); On 2017/05/10 15:43:44, mmenke wrote: > On 2017/05/09 21:43:47, fdoray wrote: > > TaskScheduler tasks run on dedicated thread when they are posted within the > > scope of a ScopedTaskEnvironment while they run on the main thread when posted > > within the scope of a ScopedTaskScheduler. Switching from > ScopedTaskEnvironment > > to ScopedTaskScheduler means that OnURLFetchComplete is not called on the main > > thread and a post task to |task_runner_| is required to quit the run loop on > the > > main thread. > > I'm not really following, nor do I understand why that affects this test and no > others. I'll try to work through it later today, but not sure I'll get to it. > Have a number of reviews on my plate today. - run_loop_.Quit() and run_loop_.Run() must be called on the same thread. - This test calls run_loop_.Run() on the main thread. - OnURLFetchComplete() is called as part of a TaskScheduler task. - Within the scope of a ScopedTaskScheduler (before this CL), TaskScheduler tasks are scheduled on the main thread. Therefore, before this CL, OnURLFetchComplete() ran on the same thread as run_loop_.Run() and it was valid to call run_loop_.Quit() inline. - Within the scope of a ScopedTaskEnvironment (with this CL), TaskScheduler tasks are scheduled on a dedicated thread. Therefore, it is no longer valid to call run_loop_.Quit() inline. Instead, we must post a QuitClosure() to the main thread. https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re... File net/url_request/url_request_simple_job_unittest.cc (right): https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re... net/url_request/url_request_simple_job_unittest.cc:224: NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle(); On 2017/05/10 15:43:44, mmenke wrote: > Why do only some RunUntilIdle calls need to be switched? Just skimming over net > unittests, there are still a ton of RunUntilIdle calls. Are we changing the > behavior of all of them? This CL removes ScopedTaskScheduler from individual net/ tests and adds a global ScopedTaskEnvironment. Within the scope of a ScopedTaskScheduler (deprecated): RunLoop().RunUntilIdle() Runs TaskScheduler tasks: Yes Runs main MessageLoop tasks: Yes NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() Runs TaskScheduler tasks: Yes Runs main MessageLoop tasks: Yes Within the scope of a ScopedTaskEnvironment: RunLoop().RunUntilIdle() Runs TaskScheduler tasks: *NO* Runs main MessageLoop tasks: Yes NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() Runs TaskScheduler tasks: *YES* Runs main MessageLoop tasks: Yes Replacing RunLoop().RunUntilIdle() with NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() is not necessary if we don't expect to run TaskScheduler tasks.
On 2017/05/10 16:16:12, fdoray wrote: > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe... > File net/url_request/url_fetcher_impl_unittest.cc (right): > > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe... > net/url_request/url_fetcher_impl_unittest.cc:133: > task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure()); > On 2017/05/10 15:43:44, mmenke wrote: > > On 2017/05/09 21:43:47, fdoray wrote: > > > TaskScheduler tasks run on dedicated thread when they are posted within the > > > scope of a ScopedTaskEnvironment while they run on the main thread when > posted > > > within the scope of a ScopedTaskScheduler. Switching from > > ScopedTaskEnvironment > > > to ScopedTaskScheduler means that OnURLFetchComplete is not called on the > main > > > thread and a post task to |task_runner_| is required to quit the run loop on > > the > > > main thread. > > > > I'm not really following, nor do I understand why that affects this test and > no > > others. I'll try to work through it later today, but not sure I'll get to it. > > Have a number of reviews on my plate today. > > - run_loop_.Quit() and run_loop_.Run() must be called on the same thread. > - This test calls run_loop_.Run() on the main thread. > - OnURLFetchComplete() is called as part of a TaskScheduler task. > - Within the scope of a ScopedTaskScheduler (before this CL), TaskScheduler > tasks are scheduled on the main thread. Therefore, before this CL, > OnURLFetchComplete() ran on the same thread as run_loop_.Run() and it was valid > to call run_loop_.Quit() inline. > - Within the scope of a ScopedTaskEnvironment (with this CL), TaskScheduler > tasks are scheduled on a dedicated thread. Therefore, it is no longer valid to > call run_loop_.Quit() inline. Instead, we must post a QuitClosure() to the main > thread. > > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re... > File net/url_request/url_request_simple_job_unittest.cc (right): > > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re... > net/url_request/url_request_simple_job_unittest.cc:224: > NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle(); > On 2017/05/10 15:43:44, mmenke wrote: > > Why do only some RunUntilIdle calls need to be switched? Just skimming over > net > > unittests, there are still a ton of RunUntilIdle calls. Are we changing the > > behavior of all of them? > > This CL removes ScopedTaskScheduler from individual net/ tests and adds a global > ScopedTaskEnvironment. > > Within the scope of a ScopedTaskScheduler (deprecated): > > RunLoop().RunUntilIdle() > Runs TaskScheduler tasks: Yes > Runs main MessageLoop tasks: Yes > NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() > Runs TaskScheduler tasks: Yes > Runs main MessageLoop tasks: Yes > > Within the scope of a ScopedTaskEnvironment: > > RunLoop().RunUntilIdle() > Runs TaskScheduler tasks: *NO* > Runs main MessageLoop tasks: Yes > NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() > Runs TaskScheduler tasks: *YES* > Runs main MessageLoop tasks: Yes > > Replacing RunLoop().RunUntilIdle() with > NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() is not necessary if we > don't expect to run TaskScheduler tasks. I'm not really seeing the difference here. No net/ production code references TaskScheduler. So why do some net tests need to run TaskScheduler tests, and others are fine with just MessageLoop ones?
On 2017/05/10 16:24:44, mmenke wrote: > On 2017/05/10 16:16:12, fdoray wrote: > > > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe... > > File net/url_request/url_fetcher_impl_unittest.cc (right): > > > > > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe... > > net/url_request/url_fetcher_impl_unittest.cc:133: > > task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure()); > > On 2017/05/10 15:43:44, mmenke wrote: > > > On 2017/05/09 21:43:47, fdoray wrote: > > > > TaskScheduler tasks run on dedicated thread when they are posted within > the > > > > scope of a ScopedTaskEnvironment while they run on the main thread when > > posted > > > > within the scope of a ScopedTaskScheduler. Switching from > > > ScopedTaskEnvironment > > > > to ScopedTaskScheduler means that OnURLFetchComplete is not called on the > > main > > > > thread and a post task to |task_runner_| is required to quit the run loop > on > > > the > > > > main thread. > > > > > > I'm not really following, nor do I understand why that affects this test and > > no > > > others. I'll try to work through it later today, but not sure I'll get to > it. > > > Have a number of reviews on my plate today. > > > > - run_loop_.Quit() and run_loop_.Run() must be called on the same thread. > > - This test calls run_loop_.Run() on the main thread. > > - OnURLFetchComplete() is called as part of a TaskScheduler task. > > - Within the scope of a ScopedTaskScheduler (before this CL), TaskScheduler > > tasks are scheduled on the main thread. Therefore, before this CL, > > OnURLFetchComplete() ran on the same thread as run_loop_.Run() and it was > valid > > to call run_loop_.Quit() inline. > > - Within the scope of a ScopedTaskEnvironment (with this CL), TaskScheduler > > tasks are scheduled on a dedicated thread. Therefore, it is no longer valid to > > call run_loop_.Quit() inline. Instead, we must post a QuitClosure() to the > main > > thread. > > > > > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re... > > File net/url_request/url_request_simple_job_unittest.cc (right): > > > > > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re... > > net/url_request/url_request_simple_job_unittest.cc:224: > > NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle(); > > On 2017/05/10 15:43:44, mmenke wrote: > > > Why do only some RunUntilIdle calls need to be switched? Just skimming over > > net > > > unittests, there are still a ton of RunUntilIdle calls. Are we changing the > > > behavior of all of them? > > > > This CL removes ScopedTaskScheduler from individual net/ tests and adds a > global > > ScopedTaskEnvironment. > > > > Within the scope of a ScopedTaskScheduler (deprecated): > > > > RunLoop().RunUntilIdle() > > Runs TaskScheduler tasks: Yes > > Runs main MessageLoop tasks: Yes > > NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() > > Runs TaskScheduler tasks: Yes > > Runs main MessageLoop tasks: Yes > > > > Within the scope of a ScopedTaskEnvironment: > > > > RunLoop().RunUntilIdle() > > Runs TaskScheduler tasks: *NO* > > Runs main MessageLoop tasks: Yes > > NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() > > Runs TaskScheduler tasks: *YES* > > Runs main MessageLoop tasks: Yes > > > > Replacing RunLoop().RunUntilIdle() with > > NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() is not necessary if > we > > don't expect to run TaskScheduler tasks. > > I'm not really seeing the difference here. No net/ production code references > TaskScheduler. So why do some net tests need to run TaskScheduler tests, and > others are fine with just MessageLoop ones? *Run TaskScheduler tasks, rather
On 2017/05/10 16:25:25, mmenke wrote: > On 2017/05/10 16:24:44, mmenke wrote: > > On 2017/05/10 16:16:12, fdoray wrote: > > > > > > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe... > > > File net/url_request/url_fetcher_impl_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe... > > > net/url_request/url_fetcher_impl_unittest.cc:133: > > > task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure()); > > > On 2017/05/10 15:43:44, mmenke wrote: > > > > On 2017/05/09 21:43:47, fdoray wrote: > > > > > TaskScheduler tasks run on dedicated thread when they are posted within > > the > > > > > scope of a ScopedTaskEnvironment while they run on the main thread when > > > posted > > > > > within the scope of a ScopedTaskScheduler. Switching from > > > > ScopedTaskEnvironment > > > > > to ScopedTaskScheduler means that OnURLFetchComplete is not called on > the > > > main > > > > > thread and a post task to |task_runner_| is required to quit the run > loop > > on > > > > the > > > > > main thread. > > > > > > > > I'm not really following, nor do I understand why that affects this test > and > > > no > > > > others. I'll try to work through it later today, but not sure I'll get to > > it. > > > > Have a number of reviews on my plate today. > > > > > > - run_loop_.Quit() and run_loop_.Run() must be called on the same thread. > > > - This test calls run_loop_.Run() on the main thread. > > > - OnURLFetchComplete() is called as part of a TaskScheduler task. > > > - Within the scope of a ScopedTaskScheduler (before this CL), TaskScheduler > > > tasks are scheduled on the main thread. Therefore, before this CL, > > > OnURLFetchComplete() ran on the same thread as run_loop_.Run() and it was > > valid > > > to call run_loop_.Quit() inline. > > > - Within the scope of a ScopedTaskEnvironment (with this CL), TaskScheduler > > > tasks are scheduled on a dedicated thread. Therefore, it is no longer valid > to > > > call run_loop_.Quit() inline. Instead, we must post a QuitClosure() to the > > main > > > thread. > > > > > > > > > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re... > > > File net/url_request/url_request_simple_job_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re... > > > net/url_request/url_request_simple_job_unittest.cc:224: > > > NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle(); > > > On 2017/05/10 15:43:44, mmenke wrote: > > > > Why do only some RunUntilIdle calls need to be switched? Just skimming > over > > > net > > > > unittests, there are still a ton of RunUntilIdle calls. Are we changing > the > > > > behavior of all of them? > > > > > > This CL removes ScopedTaskScheduler from individual net/ tests and adds a > > global > > > ScopedTaskEnvironment. > > > > > > Within the scope of a ScopedTaskScheduler (deprecated): > > > > > > RunLoop().RunUntilIdle() > > > Runs TaskScheduler tasks: Yes > > > Runs main MessageLoop tasks: Yes > > > NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() > > > Runs TaskScheduler tasks: Yes > > > Runs main MessageLoop tasks: Yes > > > > > > Within the scope of a ScopedTaskEnvironment: > > > > > > RunLoop().RunUntilIdle() > > > Runs TaskScheduler tasks: *NO* > > > Runs main MessageLoop tasks: Yes > > > NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() > > > Runs TaskScheduler tasks: *YES* > > > Runs main MessageLoop tasks: Yes > > > > > > Replacing RunLoop().RunUntilIdle() with > > > NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() is not necessary if > > we > > > don't expect to run TaskScheduler tasks. > > > > I'm not really seeing the difference here. No net/ production code references > > TaskScheduler. So why do some net tests need to run TaskScheduler tests, and > > others are fine with just MessageLoop ones? > > *Run TaskScheduler tasks, rather Yes, net/ production code references TaskScheduler https://cs.chromium.org/search/?q=task_scheduler/post_task.h+file:%5Esrc/net/... Every call to NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() in this CL actually runs a TaskScheduler task that is required for a test to pass.
On 2017/05/10 16:49:14, fdoray wrote: > On 2017/05/10 16:25:25, mmenke wrote: > > On 2017/05/10 16:24:44, mmenke wrote: > > > On 2017/05/10 16:16:12, fdoray wrote: > > > > > > > > > > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe... > > > > File net/url_request/url_fetcher_impl_unittest.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe... > > > > net/url_request/url_fetcher_impl_unittest.cc:133: > > > > task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure()); > > > > On 2017/05/10 15:43:44, mmenke wrote: > > > > > On 2017/05/09 21:43:47, fdoray wrote: > > > > > > TaskScheduler tasks run on dedicated thread when they are posted > within > > > the > > > > > > scope of a ScopedTaskEnvironment while they run on the main thread > when > > > > posted > > > > > > within the scope of a ScopedTaskScheduler. Switching from > > > > > ScopedTaskEnvironment > > > > > > to ScopedTaskScheduler means that OnURLFetchComplete is not called on > > the > > > > main > > > > > > thread and a post task to |task_runner_| is required to quit the run > > loop > > > on > > > > > the > > > > > > main thread. > > > > > > > > > > I'm not really following, nor do I understand why that affects this test > > and > > > > no > > > > > others. I'll try to work through it later today, but not sure I'll get > to > > > it. > > > > > Have a number of reviews on my plate today. > > > > > > > > - run_loop_.Quit() and run_loop_.Run() must be called on the same thread. > > > > - This test calls run_loop_.Run() on the main thread. > > > > - OnURLFetchComplete() is called as part of a TaskScheduler task. > > > > - Within the scope of a ScopedTaskScheduler (before this CL), > TaskScheduler > > > > tasks are scheduled on the main thread. Therefore, before this CL, > > > > OnURLFetchComplete() ran on the same thread as run_loop_.Run() and it was > > > valid > > > > to call run_loop_.Quit() inline. > > > > - Within the scope of a ScopedTaskEnvironment (with this CL), > TaskScheduler > > > > tasks are scheduled on a dedicated thread. Therefore, it is no longer > valid > > to > > > > call run_loop_.Quit() inline. Instead, we must post a QuitClosure() to the > > > main > > > > thread. > > > > > > > > > > > > > > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re... > > > > File net/url_request/url_request_simple_job_unittest.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_re... > > > > net/url_request/url_request_simple_job_unittest.cc:224: > > > > NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle(); > > > > On 2017/05/10 15:43:44, mmenke wrote: > > > > > Why do only some RunUntilIdle calls need to be switched? Just skimming > > over > > > > net > > > > > unittests, there are still a ton of RunUntilIdle calls. Are we changing > > the > > > > > behavior of all of them? > > > > > > > > This CL removes ScopedTaskScheduler from individual net/ tests and adds a > > > global > > > > ScopedTaskEnvironment. > > > > > > > > Within the scope of a ScopedTaskScheduler (deprecated): > > > > > > > > RunLoop().RunUntilIdle() > > > > Runs TaskScheduler tasks: Yes > > > > Runs main MessageLoop tasks: Yes > > > > NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() > > > > Runs TaskScheduler tasks: Yes > > > > Runs main MessageLoop tasks: Yes > > > > > > > > Within the scope of a ScopedTaskEnvironment: > > > > > > > > RunLoop().RunUntilIdle() > > > > Runs TaskScheduler tasks: *NO* > > > > Runs main MessageLoop tasks: Yes > > > > NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() > > > > Runs TaskScheduler tasks: *YES* > > > > Runs main MessageLoop tasks: Yes > > > > > > > > Replacing RunLoop().RunUntilIdle() with > > > > NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() is not necessary > if > > > we > > > > don't expect to run TaskScheduler tasks. > > > > > > I'm not really seeing the difference here. No net/ production code > references > > > TaskScheduler. So why do some net tests need to run TaskScheduler tests, > and > > > others are fine with just MessageLoop ones? > > > > *Run TaskScheduler tasks, rather > > Yes, net/ production code references TaskScheduler > https://cs.chromium.org/search/?q=task_scheduler/post_task.h+file:%5Esrc/net/... > Every call to NetTestSuite::GetScopedTaskEnvironment()->RunUntilIdle() in this > CL actually runs a TaskScheduler task that is required for a test to pass. Ahh, task scheduler methods just don't have the string "TaskScheduler" in them, which was confusing me.
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe... net/url_request/url_fetcher_impl_unittest.cc:133: task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure()); On 2017/05/10 16:16:12, fdoray wrote: > On 2017/05/10 15:43:44, mmenke wrote: > > On 2017/05/09 21:43:47, fdoray wrote: > > > TaskScheduler tasks run on dedicated thread when they are posted within the > > > scope of a ScopedTaskEnvironment while they run on the main thread when > posted > > > within the scope of a ScopedTaskScheduler. Switching from > > ScopedTaskEnvironment > > > to ScopedTaskScheduler means that OnURLFetchComplete is not called on the > main > > > thread and a post task to |task_runner_| is required to quit the run loop on > > the > > > main thread. > > > > I'm not really following, nor do I understand why that affects this test and > no > > others. I'll try to work through it later today, but not sure I'll get to it. > > Have a number of reviews on my plate today. > > - run_loop_.Quit() and run_loop_.Run() must be called on the same thread. > - This test calls run_loop_.Run() on the main thread. > - OnURLFetchComplete() is called as part of a TaskScheduler task. > - Within the scope of a ScopedTaskScheduler (before this CL), TaskScheduler > tasks are scheduled on the main thread. Therefore, before this CL, > OnURLFetchComplete() ran on the same thread as run_loop_.Run() and it was valid > to call run_loop_.Quit() inline. > - Within the scope of a ScopedTaskEnvironment (with this CL), TaskScheduler > tasks are scheduled on a dedicated thread. Therefore, it is no longer valid to > call run_loop_.Quit() inline. Instead, we must post a QuitClosure() to the main > thread. URLFetchers are cross-thread objects. You start them on one thread, it fetches on another thread (Which may be the starting thread), and then they notify of completion on the original thread. The URLFetcher is being started on the main thread, so it should also be notified of completion on the main thread. So if this isn't being called on the main thread, we may have a problem.
On 2017/05/11 15:30:40, mmenke wrote: > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe... > File net/url_request/url_fetcher_impl_unittest.cc (right): > > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe... > net/url_request/url_fetcher_impl_unittest.cc:133: > task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure()); > On 2017/05/10 16:16:12, fdoray wrote: > > On 2017/05/10 15:43:44, mmenke wrote: > > > On 2017/05/09 21:43:47, fdoray wrote: > > > > TaskScheduler tasks run on dedicated thread when they are posted within > the > > > > scope of a ScopedTaskEnvironment while they run on the main thread when > > posted > > > > within the scope of a ScopedTaskScheduler. Switching from > > > ScopedTaskEnvironment > > > > to ScopedTaskScheduler means that OnURLFetchComplete is not called on the > > main > > > > thread and a post task to |task_runner_| is required to quit the run loop > on > > > the > > > > main thread. > > > > > > I'm not really following, nor do I understand why that affects this test and > > no > > > others. I'll try to work through it later today, but not sure I'll get to > it. > > > Have a number of reviews on my plate today. > > > > - run_loop_.Quit() and run_loop_.Run() must be called on the same thread. > > - This test calls run_loop_.Run() on the main thread. > > - OnURLFetchComplete() is called as part of a TaskScheduler task. > > - Within the scope of a ScopedTaskScheduler (before this CL), TaskScheduler > > tasks are scheduled on the main thread. Therefore, before this CL, > > OnURLFetchComplete() ran on the same thread as run_loop_.Run() and it was > valid > > to call run_loop_.Quit() inline. > > - Within the scope of a ScopedTaskEnvironment (with this CL), TaskScheduler > > tasks are scheduled on a dedicated thread. Therefore, it is no longer valid to > > call run_loop_.Quit() inline. Instead, we must post a QuitClosure() to the > main > > thread. > > URLFetchers are cross-thread objects. You start them on one thread, it fetches > on another thread (Which may be the starting thread), and then they notify of > completion on the original thread. > > The URLFetcher is being started on the main thread, so it should also be > notified of completion on the main thread. So if this isn't being called on the > main thread, we may have a problem. (If you want to be precise, substitute "task runner" for "thread")
https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe... net/url_request/url_fetcher_impl_unittest.cc:133: task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure()); On 2017/05/11 15:30:39, mmenke wrote: > On 2017/05/10 16:16:12, fdoray wrote: > > On 2017/05/10 15:43:44, mmenke wrote: > > > On 2017/05/09 21:43:47, fdoray wrote: > > > > TaskScheduler tasks run on dedicated thread when they are posted within > the > > > > scope of a ScopedTaskEnvironment while they run on the main thread when > > posted > > > > within the scope of a ScopedTaskScheduler. Switching from > > > ScopedTaskEnvironment > > > > to ScopedTaskScheduler means that OnURLFetchComplete is not called on the > > main > > > > thread and a post task to |task_runner_| is required to quit the run loop > on > > > the > > > > main thread. > > > > > > I'm not really following, nor do I understand why that affects this test and > > no > > > others. I'll try to work through it later today, but not sure I'll get to > it. > > > Have a number of reviews on my plate today. > > > > - run_loop_.Quit() and run_loop_.Run() must be called on the same thread. > > - This test calls run_loop_.Run() on the main thread. > > - OnURLFetchComplete() is called as part of a TaskScheduler task. > > - Within the scope of a ScopedTaskScheduler (before this CL), TaskScheduler > > tasks are scheduled on the main thread. Therefore, before this CL, > > OnURLFetchComplete() ran on the same thread as run_loop_.Run() and it was > valid > > to call run_loop_.Quit() inline. > > - Within the scope of a ScopedTaskEnvironment (with this CL), TaskScheduler > > tasks are scheduled on a dedicated thread. Therefore, it is no longer valid to > > call run_loop_.Quit() inline. Instead, we must post a QuitClosure() to the > main > > thread. > > URLFetchers are cross-thread objects. You start them on one thread, it fetches > on another thread (Which may be the starting thread), and then they notify of > completion on the original thread. > > The URLFetcher is being started on the main thread, so it should also be > notified of completion on the main thread. So if this isn't being called on the > main thread, we may have a problem. URLFetcherTest.SequencedTaskTest creates a URLFetcher in TaskScheduler. OnURLFetchComplete() will therefore be invoked in TaskScheduler. URLFetcherTest.SequencedTaskTest also calls StartFetcherAndWait(), which ends up calling run_loop_.Run(), on the main thread. The call to run_loop_.Quit() must happen on the main thread (same thread as run_loop_.Run()). Therefore, it cannot happen synchronously in OnURLFetchComplete(). A post task back to the main thread is required.
On 2017/05/11 16:15:08, fdoray wrote: > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe... > File net/url_request/url_fetcher_impl_unittest.cc (right): > > https://codereview.chromium.org/2839663002/diff/160001/net/url_request/url_fe... > net/url_request/url_fetcher_impl_unittest.cc:133: > task_runner_->PostTask(FROM_HERE, run_loop_.QuitClosure()); > On 2017/05/11 15:30:39, mmenke wrote: > > On 2017/05/10 16:16:12, fdoray wrote: > > > On 2017/05/10 15:43:44, mmenke wrote: > > > > On 2017/05/09 21:43:47, fdoray wrote: > > > > > TaskScheduler tasks run on dedicated thread when they are posted within > > the > > > > > scope of a ScopedTaskEnvironment while they run on the main thread when > > > posted > > > > > within the scope of a ScopedTaskScheduler. Switching from > > > > ScopedTaskEnvironment > > > > > to ScopedTaskScheduler means that OnURLFetchComplete is not called on > the > > > main > > > > > thread and a post task to |task_runner_| is required to quit the run > loop > > on > > > > the > > > > > main thread. > > > > > > > > I'm not really following, nor do I understand why that affects this test > and > > > no > > > > others. I'll try to work through it later today, but not sure I'll get to > > it. > > > > Have a number of reviews on my plate today. > > > > > > - run_loop_.Quit() and run_loop_.Run() must be called on the same thread. > > > - This test calls run_loop_.Run() on the main thread. > > > - OnURLFetchComplete() is called as part of a TaskScheduler task. > > > - Within the scope of a ScopedTaskScheduler (before this CL), TaskScheduler > > > tasks are scheduled on the main thread. Therefore, before this CL, > > > OnURLFetchComplete() ran on the same thread as run_loop_.Run() and it was > > valid > > > to call run_loop_.Quit() inline. > > > - Within the scope of a ScopedTaskEnvironment (with this CL), TaskScheduler > > > tasks are scheduled on a dedicated thread. Therefore, it is no longer valid > to > > > call run_loop_.Quit() inline. Instead, we must post a QuitClosure() to the > > main > > > thread. > > > > URLFetchers are cross-thread objects. You start them on one thread, it > fetches > > on another thread (Which may be the starting thread), and then they notify of > > completion on the original thread. > > > > The URLFetcher is being started on the main thread, so it should also be > > notified of completion on the main thread. So if this isn't being called on > the > > main thread, we may have a problem. > > URLFetcherTest.SequencedTaskTest creates a URLFetcher in TaskScheduler. > OnURLFetchComplete() will therefore be invoked in TaskScheduler. > > URLFetcherTest.SequencedTaskTest also calls StartFetcherAndWait(), which ends up > calling run_loop_.Run(), on the main thread. > > The call to run_loop_.Quit() must happen on the main thread (same thread as > run_loop_.Run()). Therefore, it cannot happen synchronously in > OnURLFetchComplete(). A post task back to the main thread is required. Ahh, missed that some tests were running (Or pretending to run) fetchers on another task runner. LGTM, thanks for bearing with me!
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": 1494528585321100,
"parent_rev": "5641ee8c6260a7ac4fddf464ed97ea81e74116d2", "commit_rev":
"f33fede528342549ee59c5cc2b8f4f966a3edec8"}
Message was sent while issue was closed.
Description was changed from ========== Use ScopedTaskEnvironment instead of MessageLoopForIO in net unit tests. ScopedTaskEnvironment allows usage of ThreadTaskRunnerHandle and base/task_scheduler/post_task.h within its scope. It should be instantiated in everytest that uses either of these APIs (i.e. no test should instantiate a MessageLoop directly). Switching from MessageLoopForIO to ScopedTaskEnvironment in NetTestSuite allows Scoped(Async)TaskScheduler to be removed from individual net unittests. Motivation for ScopedTaskEnvironment can be found in: https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P... BUG=708584 ========== to ========== Use ScopedTaskEnvironment instead of MessageLoopForIO in net unit tests. ScopedTaskEnvironment allows usage of ThreadTaskRunnerHandle and base/task_scheduler/post_task.h within its scope. It should be instantiated in everytest that uses either of these APIs (i.e. no test should instantiate a MessageLoop directly). Switching from MessageLoopForIO to ScopedTaskEnvironment in NetTestSuite allows Scoped(Async)TaskScheduler to be removed from individual net unittests. Motivation for ScopedTaskEnvironment can be found in: https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P... BUG=708584 Review-Url: https://codereview.chromium.org/2839663002 Cr-Commit-Position: refs/heads/master@{#471073} Committed: https://chromium.googlesource.com/chromium/src/+/f33fede528342549ee59c5cc2b8f... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/f33fede528342549ee59c5cc2b8f... |
