Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2679233003:
Use TaskScheduler instead of WorkerPool in polling_proxy_config_service.cc. (Closed)
DescriptionUse TaskScheduler instead of WorkerPool in polling_proxy_config_service.cc.
This CL replaces base::WorkerPool::PostTask() with
base::PostTaskWithTraits(). The following traits are used:
Priority: Inherited (default)
The priority is inherited from the calling context (i.e. TaskTraits
are initialized with the priority of the current task).
Shutdown behavior: CONTINUE_ON_SHUTDOWN
Tasks posted with this mode which have not started executing before
shutdown is initiated will never run. Tasks with this mode running at
shutdown will be ignored (the worker will not be joined).
Note: Tasks that were previously posted to base::WorkerPool should
use this shutdown behavior because this is how base::WorkerPool
handles all its tasks.
May Block:
Tasks posted with MayBlock() may block. This includes but is not
limited to tasks that wait on synchronous file I/O operations:
read or write a file from disk, interact with a pipe or a socket,
rename or delete a file, enumerate files in a directory, etc. This
trait isn't required for the mere use of locks.
BUG=659191
Review-Url: https://codereview.chromium.org/2679233003
Cr-Commit-Position: refs/heads/master@{#451335}
Committed: https://chromium.googlesource.com/chromium/src/+/a16c9bad4d2c9f5a713442c7a4a0651d85780406
Patch Set 1 #Patch Set 2 : ScpedTaskScheduler #
Total comments: 5
Patch Set 3 : private #
Total comments: 3
Messages
Total messages: 27 (15 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: 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fdoray@chromium.org changed reviewers: + xunjieli@chromium.org
PTAL
https://codereview.chromium.org/2679233003/diff/20001/net/proxy/polling_proxy... File net/proxy/polling_proxy_config_service.cc (right): https://codereview.chromium.org/2679233003/diff/20001/net/proxy/polling_proxy... net/proxy/polling_proxy_config_service.cc:94: FROM_HERE, base::TaskTraits().MayBlock().WithShutdownBehavior( Why does this have a "MayBlock()" ? https://codereview.chromium.org/2679233003/diff/20001/net/url_request/url_req... File net/url_request/url_request_context_builder_unittest.cc (right): https://codereview.chromium.org/2679233003/diff/20001/net/url_request/url_req... net/url_request/url_request_context_builder_unittest.cc:73: base::test::ScopedTaskScheduler scoped_task_scheduler_; nit: can you put this under "private" ?
PTAnL https://codereview.chromium.org/2679233003/diff/20001/net/proxy/polling_proxy... File net/proxy/polling_proxy_config_service.cc (right): https://codereview.chromium.org/2679233003/diff/20001/net/proxy/polling_proxy... net/proxy/polling_proxy_config_service.cc:94: FROM_HERE, base::TaskTraits().MayBlock().WithShutdownBehavior( On 2017/02/10 16:53:32, xunjieli wrote: > Why does this have a "MayBlock()" ? I assumed that this was blocking since it was done on a separate thread. On Windows, the task is https://cs.chromium.org/chromium/src/net/proxy/proxy_config_service_win.cc?q=... On iOS, the task is https://cs.chromium.org/chromium/src/net/proxy/proxy_config_service_ios.cc?ty... You don't believe that this tasks could block? Disclaimer: I'm not a net/ expert. https://codereview.chromium.org/2679233003/diff/20001/net/url_request/url_req... File net/url_request/url_request_context_builder_unittest.cc (right): https://codereview.chromium.org/2679233003/diff/20001/net/url_request/url_req... net/url_request/url_request_context_builder_unittest.cc:73: base::test::ScopedTaskScheduler scoped_task_scheduler_; On 2017/02/10 16:53:32, xunjieli wrote: > nit: can you put this under "private" ? Done.
xunjieli@chromium.org changed reviewers: + eroman@chromium.org
Eric, could you take this? Does GetCurrentProxyConfig() does file i/o? I'm not familiar with this part of the codebase.
https://codereview.chromium.org/2679233003/diff/20001/net/proxy/polling_proxy... File net/proxy/polling_proxy_config_service.cc (right): https://codereview.chromium.org/2679233003/diff/20001/net/proxy/polling_proxy... net/proxy/polling_proxy_config_service.cc:94: FROM_HERE, base::TaskTraits().MayBlock().WithShutdownBehavior( On 2017/02/13 15:56:04, fdoray wrote: > On 2017/02/10 16:53:32, xunjieli wrote: > > Why does this have a "MayBlock()" ? > > I assumed that this was blocking since it was done on a separate thread. > > On Windows, the task is > https://cs.chromium.org/chromium/src/net/proxy/proxy_config_service_win.cc?q=... > > On iOS, the task is > https://cs.chromium.org/chromium/src/net/proxy/proxy_config_service_ios.cc?ty... > > You don't believe that this tasks could block? > > Disclaimer: I'm not a net/ expert. Correct. The windows implementation may do file i/o (by virtue of registry lookups), and even network i/o. Which is unfortunately why we had to move it to be async on a worker thread. https://codereview.chromium.org/2679233003/diff/40001/net/url_request/url_req... File net/url_request/url_request_context_builder_unittest.cc (right): https://codereview.chromium.org/2679233003/diff/40001/net/url_request/url_req... net/url_request/url_request_context_builder_unittest.cc:77: base::test::ScopedTaskScheduler scoped_task_scheduler_; Same question that I asked on this code review (https://codereview.chromium.org/2602243002/) -- what is the formula used for including these? With this approach it is not obvious at all where these need to be added, since it depends on the transitive set of dependencies on what is run right? Can we have a single scopedtaskscheduler at the NetSuite level? In what manner to tests fail when this is not added?
PTAnL https://codereview.chromium.org/2679233003/diff/40001/net/url_request/url_req... File net/url_request/url_request_context_builder_unittest.cc (right): https://codereview.chromium.org/2679233003/diff/40001/net/url_request/url_req... net/url_request/url_request_context_builder_unittest.cc:77: base::test::ScopedTaskScheduler scoped_task_scheduler_; On 2017/02/13 23:14:52, eroman wrote: > Same question that I asked on this code review > (https://codereview.chromium.org/2602243002/) -- what is the formula used for > including these? > 1. Upload a patch set. 2. Send it to the CQ dry run. 3. Add a base::test::ScopedTaskScheduler to tests that failed because a task was posted without a registered TaskScheduler. > With this approach it is not obvious at all where these need to be added, since > it depends on the transitive set of dependencies on what is run right? base::WorkerPool is lazy-initialized the first time that it is used in a process. Tasks posted to it can continue to run after the test from which they were posted has ended. base/task_scheduler/post_task.h functions crash if they are called when there is no TaskScheduler registered in the process. A TaskScheduler can be registered in the process by instantiating a base::test::Scoped(Async)TaskScheduler. While it can't be annoying to add a base::test::Scoped(Async)TaskScheduler to every test that uses TaskScheduler, we like the fact that it prevents tasks from running after the end of the test from which they were posted. > > Can we have a single scopedtaskscheduler at the NetSuite level? Not a crazy idea. Caveats: - Tests can no longer choose to instantiate a ScopedTaskScheduler vs. a ScopedAsyncTaskScheduler. A ScopedTaskScheduler facilitates debugging and makes test more deterministic by running everything on the main thread. However, some tests (e.g. https://cs.chromium.org/chromium/src/net/disk_cache/disk_cache_test_base.h?ty...) really need multi-threaded execution. - We plan to add methods to control time on Scoped(Async)TaskScheduler (useful when you post delayed tasks). I it better if these methods are called on a member than on a global variable. - There is a TaskScheduler in tests that don't need it (unnecessary complexity). > > In what manner to tests fail when this is not added? DCHECK failure when DCHECK_IS_ON(), crash otherwise.
LGTM for the polling proxy config changes. I still have concerns around the pattern of ScopedTaskScheduler, but that is not specific to this changelist so we can continue that conversation independently. https://codereview.chromium.org/2679233003/diff/40001/net/url_request/url_req... File net/url_request/url_request_context_builder_unittest.cc (right): https://codereview.chromium.org/2679233003/diff/40001/net/url_request/url_req... net/url_request/url_request_context_builder_unittest.cc:77: base::test::ScopedTaskScheduler scoped_task_scheduler_; On 2017/02/16 21:41:00, fdoray wrote: > On 2017/02/13 23:14:52, eroman wrote: > > Same question that I asked on this code review > > (https://codereview.chromium.org/2602243002/) -- what is the formula used for > > including these? > > > > 1. Upload a patch set. > 2. Send it to the CQ dry run. > 3. Add a base::test::ScopedTaskScheduler to tests that failed because a task was > posted without a registered TaskScheduler. > > > With this approach it is not obvious at all where these need to be added, > since > > it depends on the transitive set of dependencies on what is run right? > > base::WorkerPool is lazy-initialized the first time that it is used in a > process. Tasks posted to it can continue to run after the test from which they > were posted has ended. > > base/task_scheduler/post_task.h functions crash if they are called when there is > no TaskScheduler registered in the process. A TaskScheduler can be registered in > the process by instantiating a base::test::Scoped(Async)TaskScheduler. While it > can't be annoying to add a base::test::Scoped(Async)TaskScheduler to every test > that uses TaskScheduler, we like the fact that it prevents tasks from running > after the end of the test from which they were posted. > > > > > Can we have a single scopedtaskscheduler at the NetSuite level? > > Not a crazy idea. Caveats: > - Tests can no longer choose to instantiate a ScopedTaskScheduler vs. a > ScopedAsyncTaskScheduler. A ScopedTaskScheduler facilitates debugging and makes > test more deterministic by running everything on the main thread. However, some > tests (e.g. > https://cs.chromium.org/chromium/src/net/disk_cache/disk_cache_test_base.h?ty...) > really need multi-threaded execution. > - We plan to add methods to control time on Scoped(Async)TaskScheduler (useful > when you post delayed tasks). I it better if these methods are called on a > member than on a global variable. > - There is a TaskScheduler in tests that don't need it (unnecessary complexity). > > > > > In what manner to tests fail when this is not added? > > DCHECK failure when DCHECK_IS_ON(), crash otherwise. Thanks for the explanation. This pattern still feels fragile to me, since it isn't an explicit dependency of the API, and has all the issues related to that: * May require changing tests to have a fixture * Don't know until runtime if it has such a dependency * Can lead to a proliferation of ScopedTaskScheduler in confusing places (for instance I was surprised to see it in this file -- in fact I still don't really know why we need it here, and why it isn't in other unit-test files. Perhaps by coincidence the other locations depending on this already had a ScopedTaskRunner?). * When modifying code, it is hard to know when to *delete* a ScopedTaskScheduler. Whereas you may rely on a runtime crash to determine when to add it, what is the mechanism to know when it is no longer being used and should be deleted? Have you considered making it a default dependency, but optional to override? As in, the high level test suite can initialize and set a default async scheduler so code using it will work by default. However interested tests can optionally replace it with the deterministic version. This doesn't address your concern of ensuring that tasks do not run after the test has completed, however that model is what we relied on previously anyway.
On 2017/02/16 22:14:02, eroman wrote: > LGTM for the polling proxy config changes. > > I still have concerns around the pattern of ScopedTaskScheduler, but that is not > specific to this changelist so we can continue that conversation independently. > > https://codereview.chromium.org/2679233003/diff/40001/net/url_request/url_req... > File net/url_request/url_request_context_builder_unittest.cc (right): > > https://codereview.chromium.org/2679233003/diff/40001/net/url_request/url_req... > net/url_request/url_request_context_builder_unittest.cc:77: > base::test::ScopedTaskScheduler scoped_task_scheduler_; > On 2017/02/16 21:41:00, fdoray wrote: > > On 2017/02/13 23:14:52, eroman wrote: > > > Same question that I asked on this code review > > > (https://codereview.chromium.org/2602243002/) -- what is the formula used > for > > > including these? > > > > > > > 1. Upload a patch set. > > 2. Send it to the CQ dry run. > > 3. Add a base::test::ScopedTaskScheduler to tests that failed because a task > was > > posted without a registered TaskScheduler. > > > > > With this approach it is not obvious at all where these need to be added, > > since > > > it depends on the transitive set of dependencies on what is run right? > > > > base::WorkerPool is lazy-initialized the first time that it is used in a > > process. Tasks posted to it can continue to run after the test from which they > > were posted has ended. > > > > base/task_scheduler/post_task.h functions crash if they are called when there > is > > no TaskScheduler registered in the process. A TaskScheduler can be registered > in > > the process by instantiating a base::test::Scoped(Async)TaskScheduler. While > it > > can't be annoying to add a base::test::Scoped(Async)TaskScheduler to every > test > > that uses TaskScheduler, we like the fact that it prevents tasks from running > > after the end of the test from which they were posted. > > > > > > > > Can we have a single scopedtaskscheduler at the NetSuite level? > > > > Not a crazy idea. Caveats: > > - Tests can no longer choose to instantiate a ScopedTaskScheduler vs. a > > ScopedAsyncTaskScheduler. A ScopedTaskScheduler facilitates debugging and > makes > > test more deterministic by running everything on the main thread. However, > some > > tests (e.g. > > > https://cs.chromium.org/chromium/src/net/disk_cache/disk_cache_test_base.h?ty...) > > really need multi-threaded execution. > > - We plan to add methods to control time on Scoped(Async)TaskScheduler (useful > > when you post delayed tasks). I it better if these methods are called on a > > member than on a global variable. > > - There is a TaskScheduler in tests that don't need it (unnecessary > complexity). > > > > > > > > In what manner to tests fail when this is not added? > > > > DCHECK failure when DCHECK_IS_ON(), crash otherwise. > > Thanks for the explanation. > > This pattern still feels fragile to me, since it isn't an explicit dependency of > the API, and has all the issues related to that: > > * May require changing tests to have a fixture You can add a ScopedTaskScheduler in the body of a test that doesn't have a fixture. > * Don't know until runtime if it has such a dependency > * Can lead to a proliferation of ScopedTaskScheduler in confusing places (for > instance I was surprised to see it in this file -- in fact I still don't really > know why we need it here, and why it isn't in other unit-test files. Perhaps by > coincidence the other locations depending on this already had a > ScopedTaskRunner?). > * When modifying code, it is hard to know when to *delete* a > ScopedTaskScheduler. Whereas you may rely on a runtime crash to determine when > to add it, what is the mechanism to know when it is no longer being used and > should be deleted? We could run audits to find unused ScopedTaskSchedulers. However, I don't want to prevent devs from always adding a ScopedTaskScheduler in a test when it is only required on some platforms. > > > Have you considered making it a default dependency, but optional to override? As > in, the high level test suite can initialize and set a default async scheduler > so code using it will work by default. > > However interested tests can optionally replace it with the deterministic > version. This doesn't address your concern of ensuring that tasks do not run > after the test has completed, however that model is what we relied on previously > anyway. I've started a discussion here https://groups.google.com/a/chromium.org/forum/#!topic/scheduler-dev/Oy9FXgyOqII Thanks for your feedback.
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_tsan_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": 40001, "attempt_start_ts": 1487349870000070,
"parent_rev": "35091c96d3a0e1543978414ffc1dc2d28bc3b759", "commit_rev":
"a16c9bad4d2c9f5a713442c7a4a0651d85780406"}
Message was sent while issue was closed.
Description was changed from ========== Use TaskScheduler instead of WorkerPool in polling_proxy_config_service.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits(). The following traits are used: Priority: Inherited (default) The priority is inherited from the calling context (i.e. TaskTraits are initialized with the priority of the current task). Shutdown behavior: CONTINUE_ON_SHUTDOWN Tasks posted with this mode which have not started executing before shutdown is initiated will never run. Tasks with this mode running at shutdown will be ignored (the worker will not be joined). Note: Tasks that were previously posted to base::WorkerPool should use this shutdown behavior because this is how base::WorkerPool handles all its tasks. May Block: Tasks posted with MayBlock() may block. This includes but is not limited to tasks that wait on synchronous file I/O operations: read or write a file from disk, interact with a pipe or a socket, rename or delete a file, enumerate files in a directory, etc. This trait isn't required for the mere use of locks. BUG=659191 ========== to ========== Use TaskScheduler instead of WorkerPool in polling_proxy_config_service.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits(). The following traits are used: Priority: Inherited (default) The priority is inherited from the calling context (i.e. TaskTraits are initialized with the priority of the current task). Shutdown behavior: CONTINUE_ON_SHUTDOWN Tasks posted with this mode which have not started executing before shutdown is initiated will never run. Tasks with this mode running at shutdown will be ignored (the worker will not be joined). Note: Tasks that were previously posted to base::WorkerPool should use this shutdown behavior because this is how base::WorkerPool handles all its tasks. May Block: Tasks posted with MayBlock() may block. This includes but is not limited to tasks that wait on synchronous file I/O operations: read or write a file from disk, interact with a pipe or a socket, rename or delete a file, enumerate files in a directory, etc. This trait isn't required for the mere use of locks. BUG=659191 Review-Url: https://codereview.chromium.org/2679233003 Cr-Commit-Position: refs/heads/master@{#451335} Committed: https://chromium.googlesource.com/chromium/src/+/a16c9bad4d2c9f5a713442c7a4a0... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/a16c9bad4d2c9f5a713442c7a4a0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
