Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1433373003:
Use SequenceChecker to allow Timer to run in SequencedWorkerPool (Closed)
DescriptionUse SequenceChecker to allow Timer to run in SequencedWorkerPool
Timer was explicitly ensuring single-threaded usage by tracking the
thread used for scheduling tasks. Generalize that to ensure sequenced
task runner affinity via SequenceChecker, so it can be used by
SequencedWorkerPool threads.
BUG=552633
Committed: https://crrev.com/ffa30fe7f5ec8b13c9e41d41d828e4fb8e67110b
Cr-Commit-Position: refs/heads/master@{#365568}
Patch Set 1 #Patch Set 2 : Detach in ctor #
Total comments: 13
Patch Set 3 : Comments, tests #
Total comments: 10
Patch Set 4 : Feedback and MOAR tests #
Total comments: 4
Patch Set 5 : Review feedback #
Total comments: 6
Patch Set 6 : Review nits #
Messages
Total messages: 41 (14 generated)
Description was changed from ========== Use SequenceChecker to allow Timer to run in SequencedWorkerPool Timer was explicitly ensuring single-threaded usage by tracking the thread used for scheduling tasks. Generalize that to ensure sequenced task runner affinity via SequenceChecker, so it can be used by SequencedWorkerPool threads. BUG=552633 ========== to ========== Use SequenceChecker to allow Timer to run in SequencedWorkerPool Timer was explicitly ensuring single-threaded usage by tracking the thread used for scheduling tasks. Generalize that to ensure sequenced task runner affinity via SequenceChecker, so it can be used by SequencedWorkerPool threads. BUG=552633 ==========
jsbell@chromium.org changed reviewers: + gab@chromium.org
danakj@chromium.org changed reviewers: + danakj@chromium.org
How about unit testing with a sequenced worker pool? https://code.google.com/p/chromium/codesearch#chromium/src/base/timer/timer_u... https://codereview.chromium.org/1433373003/diff/20001/base/timer/timer.cc File base/timer/timer.cc (right): https://codereview.chromium.org/1433373003/diff/20001/base/timer/timer.cc#new... base/timer/timer.cc:103: DCHECK_EQ(was_scheduled_, false); DCHECK(!was_scheduled_) for a boolean https://codereview.chromium.org/1433373003/diff/20001/base/timer/timer.h File base/timer/timer.h (right): https://codereview.chromium.org/1433373003/diff/20001/base/timer/timer.h#newc... base/timer/timer.h:43: // task runner. can you still mention same-thread here somehow too?
IIUC there is the sequence from which tasks are posted/abandoned (and this is what the new SequenceChecker verifies) and there is the sequence bound to the SequencedTaskRunner which task are executed on. It hadn't occurred to me that these could be two different sequences until I read the implementation of Timer::PostNewScheduledTask(). Can the API be clarified to highlight this? Thanks, Gab https://codereview.chromium.org/1433373003/diff/20001/base/timer/timer.h File base/timer/timer.h (right): https://codereview.chromium.org/1433373003/diff/20001/base/timer/timer.h#newc... base/timer/timer.h:43: // task runner. On 2015/11/16 22:46:42, danakj (behind on reviews) wrote: > can you still mention same-thread here somehow too? I think the new comment is actually correct. i.e. "not thread safe" doesn't mean "thread affine" it just means "needs to be sequenced (+memory barriers between thread hops)" which SequencedTaskRunner provide. https://codereview.chromium.org/1433373003/diff/20001/base/timer/timer.h#newc... base/timer/timer.h:93: // only be called before any tasks have been scheduled. The task runner must s/The task runner/|task_runner| https://codereview.chromium.org/1433373003/diff/20001/base/timer/timer.h#newc... base/timer/timer.h:94: // run tasks on the sequenced runner the timer is used on. s/sequence runner/same sequence/ ? https://codereview.chromium.org/1433373003/diff/20001/base/timer/timer.h#newc... base/timer/timer.h:139: // corresponding task_runner_ field is null, the current task runner is s/task_runner_/|task_runner_| https://codereview.chromium.org/1433373003/diff/20001/base/timer/timer.h#newc... base/timer/timer.h:139: // corresponding task_runner_ field is null, the current task runner is s/current task runner/SequencedTaskRunner for the current thread/ https://codereview.chromium.org/1433373003/diff/20001/base/timer/timer.h#newc... base/timer/timer.h:185: // Verify usage with a single SequencedTaskRunner. This comment in redundant in specifying what a SequenceChecker is IMO. Instead I think it should mention what needs to be on the same sequence and why.
Not a high priority for me, but dusting this off a bit after the long weekend. Addressed comments and added some tests, but still a work in progress. We may want to sprinkle DCHECKs in some of the other Timer methods, and we may want to add ASSERT_DEATH() cases to the tests. https://codereview.chromium.org/1433373003/diff/20001/base/timer/timer.cc File base/timer/timer.cc (right): https://codereview.chromium.org/1433373003/diff/20001/base/timer/timer.cc#new... base/timer/timer.cc:103: DCHECK_EQ(was_scheduled_, false); On 2015/11/16 22:46:42, danakj (behind on reviews) wrote: > DCHECK(!was_scheduled_) for a boolean Done. https://codereview.chromium.org/1433373003/diff/20001/base/timer/timer.h File base/timer/timer.h (right): https://codereview.chromium.org/1433373003/diff/20001/base/timer/timer.h#newc... base/timer/timer.h:93: // only be called before any tasks have been scheduled. The task runner must On 2015/11/17 15:23:57, gab wrote: > s/The task runner/|task_runner| Done. https://codereview.chromium.org/1433373003/diff/20001/base/timer/timer.h#newc... base/timer/timer.h:94: // run tasks on the sequenced runner the timer is used on. On 2015/11/17 15:23:58, gab wrote: > s/sequence runner/same sequence/ ? Done. https://codereview.chromium.org/1433373003/diff/20001/base/timer/timer.h#newc... base/timer/timer.h:139: // corresponding task_runner_ field is null, the current task runner is On 2015/11/17 15:23:57, gab wrote: > s/task_runner_/|task_runner_| Done. https://codereview.chromium.org/1433373003/diff/20001/base/timer/timer.h#newc... base/timer/timer.h:185: // Verify usage with a single SequencedTaskRunner. On 2015/11/17 15:23:57, gab wrote: > This comment in redundant in specifying what a SequenceChecker is IMO. > > Instead I think it should mention what needs to be on the same sequence and why. Done.
lgtm % naming suggestion and test suggestion Cheers, Gab https://codereview.chromium.org/1433373003/diff/40001/base/timer/timer.h File base/timer/timer.h (right): https://codereview.chromium.org/1433373003/diff/40001/base/timer/timer.h#newc... base/timer/timer.h:165: scoped_refptr<SequencedTaskRunner> task_runner_; Maybe we should call this |destination_task_runner_| to make it clear that this is different from the API this sequence is being used from? And s/sequence_checker_/origin_sequence_checker_/ ? (I like the new comments but I think have explicit names will help readability in the .cc file) https://codereview.chromium.org/1433373003/diff/40001/base/timer/timer.h#newc... base/timer/timer.h:194: // Once the timer has been scheduled, the task runner may not be changed. s/the task runner/task_runner_/ https://codereview.chromium.org/1433373003/diff/40001/base/timer/timer_unitte... File base/timer/timer_unittest.cc (right): https://codereview.chromium.org/1433373003/diff/40001/base/timer/timer_unitte... base/timer/timer_unittest.cc:542: : event_(false /*manual_reset*/, true /*initially_signaled*/) {} Add space padding in /* comment */ https://codereview.chromium.org/1433373003/diff/40001/base/timer/timer_unitte... base/timer/timer_unittest.cc:590: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1433373003/diff/40001/base/timer/timer_unitte... base/timer/timer_unittest.cc:627: Feels a test that would: 1) Create a |timer| from pool1 2) timer->SetTaskRunner(pool2) with following test case: 3) timer->Start(&Task, base::Time::FromHours(1)) 4) timer->AbandonScheduledTask() and: 3) timer->Start(&Task, base::Time::FromMilliseconds(1)) 4) Wait for Task to Signal (it verifies it ran on the proper pool)? Does that sound right? Feels this isn't covered currently.
Great suggestions - keep 'em coming! https://codereview.chromium.org/1433373003/diff/40001/base/timer/timer.h File base/timer/timer.h (right): https://codereview.chromium.org/1433373003/diff/40001/base/timer/timer.h#newc... base/timer/timer.h:165: scoped_refptr<SequencedTaskRunner> task_runner_; On 2015/12/03 22:26:08, gab wrote: > Maybe we should call this |destination_task_runner_| to make it clear that this > is different from the API this sequence is being used from? > > And s/sequence_checker_/origin_sequence_checker_/ ? > > (I like the new comments but I think have explicit names will help readability > in the .cc file) Done. https://codereview.chromium.org/1433373003/diff/40001/base/timer/timer.h#newc... base/timer/timer.h:194: // Once the timer has been scheduled, the task runner may not be changed. On 2015/12/03 22:26:08, gab wrote: > s/the task runner/task_runner_/ Done. https://codereview.chromium.org/1433373003/diff/40001/base/timer/timer_unitte... File base/timer/timer_unittest.cc (right): https://codereview.chromium.org/1433373003/diff/40001/base/timer/timer_unitte... base/timer/timer_unittest.cc:542: : event_(false /*manual_reset*/, true /*initially_signaled*/) {} On 2015/12/03 22:26:08, gab wrote: > Add space padding in /* comment */ Done. https://codereview.chromium.org/1433373003/diff/40001/base/timer/timer_unitte... base/timer/timer_unittest.cc:590: }; On 2015/12/03 22:26:08, gab wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1433373003/diff/40001/base/timer/timer_unitte... base/timer/timer_unittest.cc:627: On 2015/12/03 22:26:09, gab wrote: > Feels a test that would: > 1) Create a |timer| from pool1 > 2) timer->SetTaskRunner(pool2) > > with following test case: > 3) timer->Start(&Task, base::Time::FromHours(1)) > 4) timer->AbandonScheduledTask() > > and: > 3) timer->Start(&Task, base::Time::FromMilliseconds(1)) > 4) Wait for Task to Signal (it verifies it ran on the proper pool)? > > > Does that sound right? Feels this isn't covered currently. Those sound great - done and done. Any others?
gab - can you take one more look?
gab@ - ping for one more look?
Very nice test suite now :-), lgtm w/ two comments. Thanks! Gab PS: Sorry for the delay, had put this one on the backburner per your earlier comment of non-urgency. https://codereview.chromium.org/1433373003/diff/60001/base/timer/timer_unitte... File base/timer/timer_unittest.cc (right): https://codereview.chromium.org/1433373003/diff/60001/base/timer/timer_unitte... base/timer/timer_unittest.cc:606: scoped_ptr<base::OneShotTimer> timer_; Feels like this should either be private (i.e. also need a helper for SetTaskRunner()) or protected but with no helper for simple calls. Given you're testing Timer here I think exposing it to tests is fine, but then the one-line helpers for Create/Start/Delete feel out of place. https://codereview.chromium.org/1433373003/diff/60001/base/timer/timer_unitte... base/timer/timer_unittest.cc:652: TaskWithSignal(base::Bind(&DoNothing)))); Isn't TaskWithSignal(base::Bind(&DoNothing)) just base::Bind(&TimerSequenceTest::Signal)?
The CQ bit was checked by jsbell@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/1433373003/#ps80001 (title: "Review feedback")
Yep, good suggestions. Done, and thanks! (still not urgent, just trying to clean off my plate!) https://codereview.chromium.org/1433373003/diff/60001/base/timer/timer_unitte... File base/timer/timer_unittest.cc (right): https://codereview.chromium.org/1433373003/diff/60001/base/timer/timer_unitte... base/timer/timer_unittest.cc:606: scoped_ptr<base::OneShotTimer> timer_; On 2015/12/15 16:47:41, gab wrote: > Feels like this should either be private (i.e. also need a helper for > SetTaskRunner()) or protected but with no helper for simple calls. > > Given you're testing Timer here I think exposing it to tests is fine, but then > the one-line helpers for Create/Start/Delete feel out of place. Done. https://codereview.chromium.org/1433373003/diff/60001/base/timer/timer_unitte... base/timer/timer_unittest.cc:652: TaskWithSignal(base::Bind(&DoNothing)))); On 2015/12/15 16:47:41, gab wrote: > Isn't TaskWithSignal(base::Bind(&DoNothing)) just > base::Bind(&TimerSequenceTest::Signal)? Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1433373003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1433373003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
danakj@ - OWNERS stamp?
LGTM a few comments in the tests https://codereview.chromium.org/1433373003/diff/80001/base/timer/timer.cc File base/timer/timer.cc (right): https://codereview.chromium.org/1433373003/diff/80001/base/timer/timer.cc#new... base/timer/timer.cc:105: destination_task_runner_.swap(task_runner); nit: you could dest_tr_ = std::move(task_runner) here now https://codereview.chromium.org/1433373003/diff/80001/base/timer/timer_unitte... File base/timer/timer_unittest.cc (right): https://codereview.chromium.org/1433373003/diff/80001/base/timer/timer_unitte... base/timer/timer_unittest.cc:572: void SetTaskRunner(SequencedTaskRunner* task_runner) { nit: SetTaskRunnerForTimer? https://codereview.chromium.org/1433373003/diff/80001/base/timer/timer_unitte... base/timer/timer_unittest.cc:586: static void VerifyAffinity(scoped_refptr<SequencedTaskRunner> task_runner) { did you mean for this to hold a ref, or did you mean SequencedTaskRunner*?
Thanks! https://codereview.chromium.org/1433373003/diff/80001/base/timer/timer.cc File base/timer/timer.cc (right): https://codereview.chromium.org/1433373003/diff/80001/base/timer/timer.cc#new... base/timer/timer.cc:105: destination_task_runner_.swap(task_runner); On 2015/12/15 22:57:18, danakj (behind on reviews) wrote: > nit: you could dest_tr_ = std::move(task_runner) here now Done. Still wrapping my head around std::swipe, er, std::move. :) https://codereview.chromium.org/1433373003/diff/80001/base/timer/timer_unitte... File base/timer/timer_unittest.cc (right): https://codereview.chromium.org/1433373003/diff/80001/base/timer/timer_unitte... base/timer/timer_unittest.cc:572: void SetTaskRunner(SequencedTaskRunner* task_runner) { On 2015/12/15 22:57:18, danakj (behind on reviews) wrote: > nit: SetTaskRunnerForTimer? Done. https://codereview.chromium.org/1433373003/diff/80001/base/timer/timer_unitte... base/timer/timer_unittest.cc:586: static void VerifyAffinity(scoped_refptr<SequencedTaskRunner> task_runner) { On 2015/12/15 22:57:18, danakj (behind on reviews) wrote: > did you mean for this to hold a ref, or did you mean SequencedTaskRunner*? Raw ptr is okay, thx.
The CQ bit was checked by jsbell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/1433373003/#ps100001 (title: "Review nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1433373003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1433373003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by jsbell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1433373003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1433373003/100001
Message was sent while issue was closed.
Description was changed from ========== Use SequenceChecker to allow Timer to run in SequencedWorkerPool Timer was explicitly ensuring single-threaded usage by tracking the thread used for scheduling tasks. Generalize that to ensure sequenced task runner affinity via SequenceChecker, so it can be used by SequencedWorkerPool threads. BUG=552633 ========== to ========== Use SequenceChecker to allow Timer to run in SequencedWorkerPool Timer was explicitly ensuring single-threaded usage by tracking the thread used for scheduling tasks. Generalize that to ensure sequenced task runner affinity via SequenceChecker, so it can be used by SequencedWorkerPool threads. BUG=552633 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Use SequenceChecker to allow Timer to run in SequencedWorkerPool Timer was explicitly ensuring single-threaded usage by tracking the thread used for scheduling tasks. Generalize that to ensure sequenced task runner affinity via SequenceChecker, so it can be used by SequencedWorkerPool threads. BUG=552633 ========== to ========== Use SequenceChecker to allow Timer to run in SequencedWorkerPool Timer was explicitly ensuring single-threaded usage by tracking the thread used for scheduling tasks. Generalize that to ensure sequenced task runner affinity via SequenceChecker, so it can be used by SequencedWorkerPool threads. BUG=552633 Committed: https://crrev.com/ffa30fe7f5ec8b13c9e41d41d828e4fb8e67110b Cr-Commit-Position: refs/heads/master@{#365568} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ffa30fe7f5ec8b13c9e41d41d828e4fb8e67110b Cr-Commit-Position: refs/heads/master@{#365568}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
This made tsan unhappy: https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20TSan%20Test... Please take a look, and if it takes a while to debug, please revert in the meantime.
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1532773002/ by jsbell@chromium.org. The reason for reverting is: TSAN failures in the test. Won't have a chance to debug for a few days, so reverting for now. Sample failure: https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20TSan%20Test....
Message was sent while issue was closed.
It looks like the race is between Timer::PostNewScheduledTask() setting scheduled_run_time_ and base::Timer::RunScheduledTask() checking scheduled_run_time_ ... which makes sense as the former runs on the "origin" sequence and the latter runs on the "destination" sequence. Question is... why wasn't this noticed before? Perhaps the origin/destination mechanism was never used and has always been flawed?
Message was sent while issue was closed.
On Wed, Dec 16, 2015 at 4:35 PM, <jsbell@chromium.org> wrote: > It looks like the race is between > Timer::PostNewScheduledTask() setting scheduled_run_time_ and > base::Timer::RunScheduledTask() checking scheduled_run_time_ ... which > makes > sense as the former runs on the "origin" sequence and the latter runs on > the > "destination" sequence. > > Question is... why wasn't this noticed before? Perhaps the > origin/destination > mechanism was never used and has always been flawed? > Huh, lack of tests I think. Usually it's all on the same thread, and I don't think we got good additional coverage when adding the SetTaskRunner. It would race with desired_run_time_ too then, if you called Reset(), right? Think we need a Lock when the destination task runner isn't null? -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2015/12/17 00:50:36, danakj (behind on reviews) wrote: > On Wed, Dec 16, 2015 at 4:35 PM, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=jsbell@chromium.org> wrote: > > > It looks like the race is between > > Timer::PostNewScheduledTask() setting scheduled_run_time_ and > > base::Timer::RunScheduledTask() checking scheduled_run_time_ ... which > > makes > > sense as the former runs on the "origin" sequence and the latter runs on > > the > > "destination" sequence. > > > > Question is... why wasn't this noticed before? Perhaps the > > origin/destination > > mechanism was never used and has always been flawed? > > > > Huh, lack of tests I think. Usually it's all on the same thread, and I > don't think we got good additional coverage when adding the SetTaskRunner. > > It would race with desired_run_time_ too then, if you called Reset(), > right? Think we need a Lock when the destination task runner isn't null? Nice catch by TSAN :-), I think all members are suffering from this (at least all non-const members used in RunScheduledTask(), i.e.: is_running_, desired_run_time_ and scheduled_run_time_). This was definitely already an issue but was just caught now thanks to your much improved tests. Locking is one solution, but I don't like it as it will require locking all over the impl and will be very error-prone. A better way IMO would be to have a separate object which lives on the destination task runner and has a copy of the relevant state. Then Timer can interact with that object through thread-safe APIs without having to share state with it. In practice I think that object is BaseTimerTaskInternal but instead of having a pointer back to Timer* it holds a copy of the relevant state and Timer calls new thread-safe methods to interact with it where relevant (or perhaps even better than AutoLocking methods would be to enforce that its methods are only called on its TaskRunner, i.e. to Cancel/Reset you'd have to PostTask to it and hope to make it ahead of scheduled delay which is fine IMO as cancel will always be racy anyways).
Message was sent while issue was closed.
On Thu, Dec 17, 2015 at 10:34 AM, <gab@chromium.org> wrote: > On 2015/12/17 00:50:36, danakj (behind on reviews) wrote: > >> On Wed, Dec 16, 2015 at 4:35 PM, >> > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=jsbell@chromium.org> > wrote: > > > It looks like the race is between >> > Timer::PostNewScheduledTask() setting scheduled_run_time_ and >> > base::Timer::RunScheduledTask() checking scheduled_run_time_ ... which >> > makes >> > sense as the former runs on the "origin" sequence and the latter runs on >> > the >> > "destination" sequence. >> > >> > Question is... why wasn't this noticed before? Perhaps the >> > origin/destination >> > mechanism was never used and has always been flawed? >> > >> > > Huh, lack of tests I think. Usually it's all on the same thread, and I >> don't think we got good additional coverage when adding the SetTaskRunner. >> > > It would race with desired_run_time_ too then, if you called Reset(), >> right? Think we need a Lock when the destination task runner isn't null? >> > > Nice catch by TSAN :-), I think all members are suffering from this (at > least > all non-const members used in RunScheduledTask(), i.e.: is_running_, > desired_run_time_ and scheduled_run_time_). > > This was definitely already an issue but was just caught now thanks to > your much > improved tests. > > Locking is one solution, but I don't like it as it will require locking > all over > the impl and will be very error-prone. > > A better way IMO would be to have a separate object which lives on the > destination task runner and has a copy of the relevant state. > > Then Timer can interact with that object through thread-safe APIs without > having > to share state with it. > > In practice I think that object is BaseTimerTaskInternal but instead of > having a > pointer back to Timer* it holds a copy of the relevant state and Timer > calls new > thread-safe methods to interact with it where relevant (or perhaps even > better > than AutoLocking methods would be to enforce that its methods are only > called on > its TaskRunner, i.e. to Cancel/Reset you'd have to PostTask to it and hope > to > make it ahead of scheduled delay which is fine IMO as cancel will always > be racy > anyways). > Hm, interesting thought. It does provide different behaviour though. In theory with a lock the other thread could post a Reset() and wait for it to happen, and when it unblocked the timer would be reset. With a post, that wouldn't be possible any more. There may be other less obscure use cases that would change also. Right now without the lock those would sometimes work and sometimes not, so probably there's no code that wants to depend on something like that (also the SetTaskRunner is pretty new), but just some food for thought about lock vs post. > https://codereview.chromium.org/1433373003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Description was changed from ========== Use SequenceChecker to allow Timer to run in SequencedWorkerPool Timer was explicitly ensuring single-threaded usage by tracking the thread used for scheduling tasks. Generalize that to ensure sequenced task runner affinity via SequenceChecker, so it can be used by SequencedWorkerPool threads. BUG=552633 Committed: https://crrev.com/ffa30fe7f5ec8b13c9e41d41d828e4fb8e67110b Cr-Commit-Position: refs/heads/master@{#365568} ========== to ========== Use SequenceChecker to allow Timer to run in SequencedWorkerPool Timer was explicitly ensuring single-threaded usage by tracking the thread used for scheduling tasks. Generalize that to ensure sequenced task runner affinity via SequenceChecker, so it can be used by SequencedWorkerPool threads. BUG=552633 Committed: https://crrev.com/ffa30fe7f5ec8b13c9e41d41d828e4fb8e67110b Cr-Commit-Position: refs/heads/master@{#365568} ==========
FYI, I filed https://crbug.com/587199 to track the race, since I'm obviously not getting to it promptly. Leaving this CL in limbo until that's sorted.
Message was sent while issue was closed.
FYI, closed this out since gab is picking it up: https://codereview.chromium.org/2491613004/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
