Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2533473002:
Use TaskScheduler instead of WorkerPool in client_cert_store_chromeos.cc. (Closed)
DescriptionUse TaskScheduler instead of WorkerPool in client_cert_store_chromeos.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).
With File IO:
The task waits on synchronous file IO operations.
With Wait:
The task waits on things other than synchronous file IO
operations (e.g. WaitableEvent, ConditionVariable, join a thread,
join a process, blocking system call that doesn't involve
interactions with the file system).
BUG=659191
Committed: https://crrev.com/14589e9ca09b5cf03be779b0c1158cc5388def05
Cr-Commit-Position: refs/heads/master@{#436087}
Patch Set 1 #Patch Set 2 : fix build error #Patch Set 3 : add ScopedTaskScheduler #
Total comments: 2
Patch Set 4 : CR gab #16 (initialize ScopedTaskScheduler first) #Patch Set 5 : CR gab #16 (CONTINUE_ON_SHUTDOWN) #
Total comments: 4
Patch Set 6 : remove explicit priority, add withwait() #Patch Set 7 : add withfileio() #
Depends on Patchset: Messages
Total messages: 40 (28 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: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
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_chromeos_ozone_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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Use TaskScheduler instead of WorkerPool in client_cert_store_chromeos.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits() call. The following traits are used: Priority: BACKGROUND User won't notice if this task takes an arbitrarily long time to complete. Shutdown behavior: SKIP_ON_SHUTDOWN (default) Tasks posted with this mode that have not started executing at shutdown will never run. However, any task that has already begun executing when shutdown is invoked will be allowed to continue and will block shutdown until completion. No File IO (default): The task does not perform synchronous IO operations. It spends most of its time using the CPU. BUG=659191 ========== to ========== Use TaskScheduler instead of WorkerPool in client_cert_store_chromeos.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits(). The following traits are used: Priority: BACKGROUND User won't notice if this task takes an arbitrarily long time to complete. Shutdown behavior: SKIP_ON_SHUTDOWN (default) Tasks posted with this mode that have not started executing at shutdown will never run. However, any task that has already begun executing when shutdown is invoked will be allowed to continue and will block shutdown until completion. No File IO (default): The task does not perform synchronous IO operations. It spends most of its time using the CPU. BUG=659191 ==========
fdoray@chromium.org changed reviewers: + gab@chromium.org
PTAL
lgtm w/ CONTINUE_ON_SHUTDOWN and OWNERS approval of TaskPriority https://codereview.chromium.org/2533473002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/net/client_cert_store_chromeos_unittest.cc (right): https://codereview.chromium.org/2533473002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/net/client_cert_store_chromeos_unittest.cc:93: base::test::ScopedTaskScheduler scoped_task_scheduler_; Flip order to match product's destruction order.
Description was changed from ========== Use TaskScheduler instead of WorkerPool in client_cert_store_chromeos.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits(). The following traits are used: Priority: BACKGROUND User won't notice if this task takes an arbitrarily long time to complete. Shutdown behavior: SKIP_ON_SHUTDOWN (default) Tasks posted with this mode that have not started executing at shutdown will never run. However, any task that has already begun executing when shutdown is invoked will be allowed to continue and will block shutdown until completion. No File IO (default): The task does not perform synchronous IO operations. It spends most of its time using the CPU. BUG=659191 ========== to ========== Use TaskScheduler instead of WorkerPool in client_cert_store_chromeos.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits(). The following traits are used: Priority: BACKGROUND User won't notice if this task takes an arbitrarily long time to complete. Shutdown behavior: SKIP_ON_SHUTDOWN (default) Tasks posted with this mode that have not started executing at shutdown will never run. However, any task that has already begun executing when shutdown is invoked will be allowed to continue and will block shutdown until completion. No File IO (default): The task does not perform synchronous IO operations. No Wait (default): The task does not wait on things other than synchronous file IO operations (e.g. WaitableEvent, ConditionVariable, join a thread, join a process, blocking system call that doesn't involve interactions with the file system). BUG=659191 ==========
Description was changed from ========== Use TaskScheduler instead of WorkerPool in client_cert_store_chromeos.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits(). The following traits are used: Priority: BACKGROUND User won't notice if this task takes an arbitrarily long time to complete. Shutdown behavior: SKIP_ON_SHUTDOWN (default) Tasks posted with this mode that have not started executing at shutdown will never run. However, any task that has already begun executing when shutdown is invoked will be allowed to continue and will block shutdown until completion. No File IO (default): The task does not perform synchronous IO operations. No Wait (default): The task does not wait on things other than synchronous file IO operations (e.g. WaitableEvent, ConditionVariable, join a thread, join a process, blocking system call that doesn't involve interactions with the file system). BUG=659191 ========== to ========== Use TaskScheduler instead of WorkerPool in client_cert_store_chromeos.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits(). The following traits are used: Priority: BACKGROUND User won't notice if this task takes an arbitrarily long time to complete. 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). No File IO (default): The task does not perform synchronous IO operations. No Wait (default): The task does not wait on things other than synchronous file IO operations (e.g. WaitableEvent, ConditionVariable, join a thread, join a process, blocking system call that doesn't involve interactions with the file system). BUG=659191 ==========
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.
https://codereview.chromium.org/2533473002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/net/client_cert_store_chromeos_unittest.cc (right): https://codereview.chromium.org/2533473002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/net/client_cert_store_chromeos_unittest.cc:93: base::test::ScopedTaskScheduler scoped_task_scheduler_; On 2016/11/28 16:39:05, gab wrote: > Flip order to match product's destruction order. Done.
fdoray@chromium.org changed reviewers: + mattm@chromium.org
mattm@: Please review this CL. You should verify that the traits mentioned in the CL description make sense for this task. Thanks! Why are you doing this? WorkerPool is being deprecated in favor of TaskScheduler. Read the design doc https://docs.google.com/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... Is a BACKGROUND priority correct? See all priorities in base/task_scheduler/task_traits.h Is a CONTINUE_ON_SHUTDOWN shutdown behavior correct? It should, because it's how the task was handled by base::WorkerPool before this CL.
https://codereview.chromium.org/2533473002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/net/client_cert_store_chromeos.cc (right): https://codereview.chromium.org/2533473002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/net/client_cert_store_chromeos.cc:86: .WithPriority(base::TaskPriority::BACKGROUND) This should probably be USER_BLOCKING, or at least USER_VISIBLE. When you load an HTTPS site, the server can request in the handshake that the client supply a client certificate. Chrome will then show a list of matching certificates and let the user choose one before proceeding with the connection. This code is (part of) that responsible for fetching the list of certificates which would be shown to the user. One thing to note though is that the task could also block arbitrarily long depending on the user setup (smartcards, etc), so that's why it was marked as task_is_slow=true in the WorkerPool call. Are potentially slow USER_BLOCKING or USER_VISIBLE tasks a problem? https://codereview.chromium.org/2533473002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/net/client_cert_store_chromeos.cc:88: base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN), CONTINUE_ON_SHUTDOWN is fine.
Description was changed from ========== Use TaskScheduler instead of WorkerPool in client_cert_store_chromeos.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits(). The following traits are used: Priority: BACKGROUND User won't notice if this task takes an arbitrarily long time to complete. 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). No File IO (default): The task does not perform synchronous IO operations. No Wait (default): The task does not wait on things other than synchronous file IO operations (e.g. WaitableEvent, ConditionVariable, join a thread, join a process, blocking system call that doesn't involve interactions with the file system). BUG=659191 ========== to ========== Use TaskScheduler instead of WorkerPool in client_cert_store_chromeos.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits(). The following traits are used: Priority: BACKGROUND User won't notice if this task takes an arbitrarily long time to complete. 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). With File IO: The task waits on synchronous file IO operations. With Wait: The task waits on things other than synchronous file IO operations (e.g. WaitableEvent, ConditionVariable, join a thread, join a process, blocking system call that doesn't involve interactions with the file system). BUG=659191 ==========
PTAnL https://codereview.chromium.org/2533473002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/net/client_cert_store_chromeos.cc (right): https://codereview.chromium.org/2533473002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/net/client_cert_store_chromeos.cc:86: .WithPriority(base::TaskPriority::BACKGROUND) On 2016/11/29 21:48:46, mattm wrote: > This should probably be USER_BLOCKING, or at least USER_VISIBLE. > > When you load an HTTPS site, the server can request in the handshake that the > client supply a client certificate. Chrome will then show a list of matching > certificates and let the user choose one before proceeding with the connection. > This code is (part of) that responsible for fetching the list of certificates > which would be shown to the user. I removed the explicit priority. The task's priority will now be inherited from the task that posts it. Unless a post task call site is clearly USER_VISIBLE/USER_BLOCKING (e.g. inside a OnKeyPressed() method), we prefer relying on priority inheritance. > > One thing to note though is that the task could also block arbitrarily long > depending on the user setup (smartcards, etc), so that's why it was marked as > task_is_slow=true in the WorkerPool call. Are potentially slow USER_BLOCKING or > USER_VISIBLE tasks a problem? I added WithWait() for that https://cs.chromium.org/chromium/src/base/task_scheduler/task_traits.h?sq=pac... https://codereview.chromium.org/2533473002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/net/client_cert_store_chromeos.cc:88: base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN), On 2016/11/29 21:48:46, mattm wrote: > CONTINUE_ON_SHUTDOWN is fine. Acknowledged.
lgtm (remove priority:background reference from description)
Description was changed from ========== Use TaskScheduler instead of WorkerPool in client_cert_store_chromeos.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits(). The following traits are used: Priority: BACKGROUND User won't notice if this task takes an arbitrarily long time to complete. 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). With File IO: The task waits on synchronous file IO operations. With Wait: The task waits on things other than synchronous file IO operations (e.g. WaitableEvent, ConditionVariable, join a thread, join a process, blocking system call that doesn't involve interactions with the file system). BUG=659191 ========== to ========== Use TaskScheduler instead of WorkerPool in client_cert_store_chromeos.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). With File IO: The task waits on synchronous file IO operations. With Wait: The task waits on things other than synchronous file IO operations (e.g. WaitableEvent, ConditionVariable, join a thread, join a process, blocking system call that doesn't involve interactions with the file system). BUG=659191 ==========
On 2016/12/02 22:22:26, mattm wrote: > lgtm (remove priority:background reference from description) Done
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/2533473002/#ps120001 (title: "add withfileio()")
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": 120001, "attempt_start_ts": 1480720938442460,
"parent_rev": "4c80a0f137c604b510685cb2b48cffbf96aa3b0c", "commit_rev":
"144816f17e271a90e0d7695e6ad6d45913d80d46"}
Message was sent while issue was closed.
Description was changed from ========== Use TaskScheduler instead of WorkerPool in client_cert_store_chromeos.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). With File IO: The task waits on synchronous file IO operations. With Wait: The task waits on things other than synchronous file IO operations (e.g. WaitableEvent, ConditionVariable, join a thread, join a process, blocking system call that doesn't involve interactions with the file system). BUG=659191 ========== to ========== Use TaskScheduler instead of WorkerPool in client_cert_store_chromeos.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). With File IO: The task waits on synchronous file IO operations. With Wait: The task waits on things other than synchronous file IO operations (e.g. WaitableEvent, ConditionVariable, join a thread, join a process, blocking system call that doesn't involve interactions with the file system). BUG=659191 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Use TaskScheduler instead of WorkerPool in client_cert_store_chromeos.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). With File IO: The task waits on synchronous file IO operations. With Wait: The task waits on things other than synchronous file IO operations (e.g. WaitableEvent, ConditionVariable, join a thread, join a process, blocking system call that doesn't involve interactions with the file system). BUG=659191 ========== to ========== Use TaskScheduler instead of WorkerPool in client_cert_store_chromeos.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). With File IO: The task waits on synchronous file IO operations. With Wait: The task waits on things other than synchronous file IO operations (e.g. WaitableEvent, ConditionVariable, join a thread, join a process, blocking system call that doesn't involve interactions with the file system). BUG=659191 Committed: https://crrev.com/14589e9ca09b5cf03be779b0c1158cc5388def05 Cr-Commit-Position: refs/heads/master@{#436087} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/14589e9ca09b5cf03be779b0c1158cc5388def05 Cr-Commit-Position: refs/heads/master@{#436087}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2720143003/ by fdoray@chromium.org. The reason for reverting is: Using TaskScheduler with NSS is not safe until TaskScheduler supports dynamically growing its thread pool.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
