Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2916693002:
DetachFromSequence in Timer::Stop() (Closed)
DescriptionDetachFromSequence in Timer::Stop()
I was initially going to make Stop() always unconditionally AbandonScheduledTask()
in Stop() @
https://codereview.chromium.org/2491613004/diff2/800001:820001/base/timer/timer.cc
as it was the only way to ensure the task was abandoned on the running sequence.
But I've since realized we could actually detach the sequence in Stop() so that
the destructor may safely abandon the scheduled task.
This is preferable as it avoids a potential task storm if any user
repeatedly starts/stops the same timer in a burst...
BUG=552633
Review-Url: https://codereview.chromium.org/2916693002
Cr-Commit-Position: refs/heads/master@{#477789}
Committed: https://chromium.googlesource.com/chromium/src/+/e164e3ceab4dd13fb4e240e5d663e8ddb43b2a03
Patch Set 1 #
Total comments: 5
Patch Set 2 : comment nit #
Messages
Total messages: 24 (15 generated)
The CQ bit was checked by gab@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 gab@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.
gab@chromium.org changed reviewers: + danakj@chromium.org
Hey Dana, in the last PS of https://codereview.chromium.org/2491613004/ I opted to forgo the new Stop/Abandon logic to really keep it only around sequencification. In this CL I'm proposing a better alternative than what I had originally planned there (i.e. https://codereview.chromium.org/2491613004/diff2/800001:840001/base/timer/tim...). PTaL, thanks! https://codereview.chromium.org/2916693002/diff/1/base/timer/timer.h File base/timer/timer.h (right): https://codereview.chromium.org/2916693002/diff/1/base/timer/timer.h#newcode48 base/timer/timer.h:48: // changed *prior* to Start() via SetTaskRunner(). The documentation was already mentioning this as of https://codereview.chromium.org/2491613004/
daily ping, thanks!
gab@chromium.org changed reviewers: + fdoray@chromium.org
+fdoray, looks like Dana is away, thanks!
https://codereview.chromium.org/2916693002/diff/1/base/timer/timer.cc File base/timer/timer.cc (right): https://codereview.chromium.org/2916693002/diff/1/base/timer/timer.cc#newcode147 base/timer/timer.cc:147: // It's safe to destroy or restart Timer on another sequence after Stop(). Did you intentionally do this before running the user_task_ destructors? https://codereview.chromium.org/2916693002/diff/1/base/timer/timer.h File base/timer/timer.h (right): https://codereview.chromium.org/2916693002/diff/1/base/timer/timer.h#newcode207 base/timer/timer.h:207: // destroyed or even restarted on another sequence. s/even //
https://codereview.chromium.org/2916693002/diff/1/base/timer/timer.cc File base/timer/timer.cc (right): https://codereview.chromium.org/2916693002/diff/1/base/timer/timer.cc#newcode147 base/timer/timer.cc:147: // It's safe to destroy or restart Timer on another sequence after Stop(). On 2017/06/06 20:07:12, danakj wrote: > Did you intentionally do this before running the user_task_ destructors? Yes, see comment at the end of this method, it's not safe to access |this| after destroying |user_task_| as it can own |this|.
LGTM
Thanks https://codereview.chromium.org/2916693002/diff/1/base/timer/timer.h File base/timer/timer.h (right): https://codereview.chromium.org/2916693002/diff/1/base/timer/timer.h#newcode207 base/timer/timer.h:207: // destroyed or even restarted on another sequence. On 2017/06/06 20:07:12, danakj wrote: > s/even // Done.
The CQ bit was checked by gab@chromium.org to run a CQ dry run
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2916693002/#ps20001 (title: "comment nit")
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": 20001, "attempt_start_ts": 1496862180235150,
"parent_rev": "52a81bdab704346032d8d16c83251d22b2a5eb0c", "commit_rev":
"e164e3ceab4dd13fb4e240e5d663e8ddb43b2a03"}
Message was sent while issue was closed.
Description was changed from ========== DetachFromSequence in Timer::Stop() I was initially going to make Stop() always unconditionally AbandonScheduledTask() in Stop() @ https://codereview.chromium.org/2491613004/diff2/800001:820001/base/timer/tim... as it was the only way to ensure the task was abandoned on the running sequence. But I've since realized we could actually detach the sequence in Stop() so that the destructor may safely abandon the scheduled task. This is preferable as it avoids a potential task storm if any user repeatedly starts/stops the same timer in a burst... BUG=552633 ========== to ========== DetachFromSequence in Timer::Stop() I was initially going to make Stop() always unconditionally AbandonScheduledTask() in Stop() @ https://codereview.chromium.org/2491613004/diff2/800001:820001/base/timer/tim... as it was the only way to ensure the task was abandoned on the running sequence. But I've since realized we could actually detach the sequence in Stop() so that the destructor may safely abandon the scheduled task. This is preferable as it avoids a potential task storm if any user repeatedly starts/stops the same timer in a burst... BUG=552633 Review-Url: https://codereview.chromium.org/2916693002 Cr-Commit-Position: refs/heads/master@{#477789} Committed: https://chromium.googlesource.com/chromium/src/+/e164e3ceab4dd13fb4e240e5d663... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/e164e3ceab4dd13fb4e240e5d663... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
