Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2805933002:
[reland] Allow ObserverListThreadSafe to be used from sequenced tasks. (Closed)
DescriptionAllow ObserverListThreadSafe to be used from sequenced tasks.
This CL is identical to https://codereview.chromium.org/2592143003,
except that RemoveObserver() works when called from any sequence
(existing code rely on that).
Previously, observers could only be added to an ObserverListThreadSafe
from single-threaded tasks. Observers were notified on the registration
thread.
With this CL, observers can also be added to an ObserverListThreadSafe
from sequenced tasks. They are notified on the registration sequence.
ObserverListThreadSafe behaves almost the same way as before when used
from single-threaded tasks. The following things changed:
- The order in which observers registered from the same thread
are notified is no longer deterministic.
- One notification task is posted for each observer rather than one
notification task per thread.
- If an observer is added to a NOTIFY_ALL ObserverListThreadSafe from
a stack of notification dispatches, only the notification on top of
the stack is sent to the newly added observer.
BUG=675631
Review-Url: https://codereview.chromium.org/2805933002
Cr-Commit-Position: refs/heads/master@{#463345}
Committed: https://chromium.googlesource.com/chromium/src/+/6aad314383abc65e773b58a5aa697a84bf42e176
Patch Set 1 : same as https://codereview.chromium.org/2592143003 #Patch Set 2 : rebase-and-remove-dcheck #Patch Set 3 : fix-race #
Total comments: 4
Patch Set 4 : CR-gab #
Messages
Total messages: 27 (19 generated)
Description was changed from ========== Allow ObserverListThreadSafe to be used from sequenced tasks. This CL is identical to https://codereview.chromium.org/2592143003, except that a DCHECK has been disabled because it isn't compatible with existing code. Previously, observers could only be added to an ObserverListThreadSafe from single-threaded tasks. Observers were notified on the registration thread. With this CL, observers can also be added to an ObserverListThreadSafe from sequenced tasks. They are notified on the registration sequence. ObserverListThreadSafe behaves almost the same way as before when used from single-threaded tasks. The following things changed: - The order in which observers registered from the same thread are notified is no longer deterministic. - One notification task is posted for each observer rather than one notification task per thread. - If an observer is added to a NOTIFY_ALL ObserverListThreadSafe from a stack of notification dispatches, only the notification on top of the stack is sent to the newly added observer. BUG=675631 ========== to ========== Allow ObserverListThreadSafe to be used from sequenced tasks. This CL is identical to https://codereview.chromium.org/2592143003, except that RemoveObserver() works when called from any sequence (existing code rely on that). Previously, observers could only be added to an ObserverListThreadSafe from single-threaded tasks. Observers were notified on the registration thread. With this CL, observers can also be added to an ObserverListThreadSafe from sequenced tasks. They are notified on the registration sequence. ObserverListThreadSafe behaves almost the same way as before when used from single-threaded tasks. The following things changed: - The order in which observers registered from the same thread are notified is no longer deterministic. - One notification task is posted for each observer rather than one notification task per thread. - If an observer is added to a NOTIFY_ALL ObserverListThreadSafe from a stack of notification dispatches, only the notification on top of the stack is sent to the newly added observer. BUG=675631 ==========
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: This issue passed the CQ dry run.
fdoray@chromium.org changed reviewers: + gab@chromium.org
PTAL
lgtm w/ q https://codereview.chromium.org/2805933002/diff/40001/base/observer_list_unit... File base/observer_list_unittest.cc (right): https://codereview.chromium.org/2805933002/diff/40001/base/observer_list_unit... base/observer_list_unittest.cc:611: TaskScheduler::GetInstance()->FlushForTesting(); Why is it racy without this? Doesn't the ScopedTaskScheduler do this?
https://codereview.chromium.org/2805933002/diff/40001/base/observer_list_unit... File base/observer_list_unittest.cc (right): https://codereview.chromium.org/2805933002/diff/40001/base/observer_list_unit... base/observer_list_unittest.cc:611: TaskScheduler::GetInstance()->FlushForTesting(); On 2017/04/10 16:23:45, gab wrote: > Why is it racy without this? Doesn't the ScopedTaskScheduler do this? Yes. But without this, a task may still be waiting on |barrier| when |barrier| is destroyed (because |barrier| is destroyed before |scoped_task_environment|).
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...
https://codereview.chromium.org/2805933002/diff/40001/base/observer_list_unit... File base/observer_list_unittest.cc (right): https://codereview.chromium.org/2805933002/diff/40001/base/observer_list_unit... base/observer_list_unittest.cc:611: TaskScheduler::GetInstance()->FlushForTesting(); On 2017/04/10 16:34:09, fdoray wrote: > On 2017/04/10 16:23:45, gab wrote: > > Why is it racy without this? Doesn't the ScopedTaskScheduler do this? > > Yes. But without this, a task may still be waiting on |barrier| when |barrier| > is destroyed (because |barrier| is destroyed before |scoped_task_environment|). Hmmm ok, then flip the order of their definition on the stack at beginning of test w/ comment?
The CQ bit was unchecked by fdoray@chromium.org
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
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 gab@chromium.org Link to the patchset: https://codereview.chromium.org/2805933002/#ps60001 (title: "CR-gab")
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": 60001, "attempt_start_ts": 1491846233458740,
"parent_rev": "1f5bbbc2bdb5af6fa4d6940dbefb9538620d8d1c", "commit_rev":
"6aad314383abc65e773b58a5aa697a84bf42e176"}
Message was sent while issue was closed.
Description was changed from ========== Allow ObserverListThreadSafe to be used from sequenced tasks. This CL is identical to https://codereview.chromium.org/2592143003, except that RemoveObserver() works when called from any sequence (existing code rely on that). Previously, observers could only be added to an ObserverListThreadSafe from single-threaded tasks. Observers were notified on the registration thread. With this CL, observers can also be added to an ObserverListThreadSafe from sequenced tasks. They are notified on the registration sequence. ObserverListThreadSafe behaves almost the same way as before when used from single-threaded tasks. The following things changed: - The order in which observers registered from the same thread are notified is no longer deterministic. - One notification task is posted for each observer rather than one notification task per thread. - If an observer is added to a NOTIFY_ALL ObserverListThreadSafe from a stack of notification dispatches, only the notification on top of the stack is sent to the newly added observer. BUG=675631 ========== to ========== Allow ObserverListThreadSafe to be used from sequenced tasks. This CL is identical to https://codereview.chromium.org/2592143003, except that RemoveObserver() works when called from any sequence (existing code rely on that). Previously, observers could only be added to an ObserverListThreadSafe from single-threaded tasks. Observers were notified on the registration thread. With this CL, observers can also be added to an ObserverListThreadSafe from sequenced tasks. They are notified on the registration sequence. ObserverListThreadSafe behaves almost the same way as before when used from single-threaded tasks. The following things changed: - The order in which observers registered from the same thread are notified is no longer deterministic. - One notification task is posted for each observer rather than one notification task per thread. - If an observer is added to a NOTIFY_ALL ObserverListThreadSafe from a stack of notification dispatches, only the notification on top of the stack is sent to the newly added observer. BUG=675631 Review-Url: https://codereview.chromium.org/2805933002 Cr-Commit-Position: refs/heads/master@{#463345} Committed: https://chromium.googlesource.com/chromium/src/+/6aad314383abc65e773b58a5aa69... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/6aad314383abc65e773b58a5aa69...
Message was sent while issue was closed.
https://codereview.chromium.org/2805933002/diff/40001/base/observer_list_unit... File base/observer_list_unittest.cc (right): https://codereview.chromium.org/2805933002/diff/40001/base/observer_list_unit... base/observer_list_unittest.cc:611: TaskScheduler::GetInstance()->FlushForTesting(); On 2017/04/10 16:36:14, gab wrote: > On 2017/04/10 16:34:09, fdoray wrote: > > On 2017/04/10 16:23:45, gab wrote: > > > Why is it racy without this? Doesn't the ScopedTaskScheduler do this? > > > > Yes. But without this, a task may still be waiting on |barrier| when |barrier| > > is destroyed (because |barrier| is destroyed before > |scoped_task_environment|). > > Hmmm ok, then flip the order of their definition on the stack at beginning of > test w/ comment? Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
