Skip to content

Commit

Permalink
Add thread checker for UniqueNotifier
Browse files Browse the repository at this point in the history
If we want to guarantee that "Multiple schedules should only result in one run" , we should
make sure UniqueNotifier::Schedule and UniqueNotifier::Notify happens at the same thread.

If UniqueNotifier::Schedule and UniqueNotifier::Notify happen in different threads,
UniqueNotifier::Notify runs in a different thread. Multiple schedules may result in more
than one notify. And the more schedules, the high possiblity to more runs. The empirical
value is 50000.

Bug: None
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I6adb3883362670a865bece56f24b64749fa307b7
Reviewed-on: https://chromium-review.googlesource.com/1198491
Commit-Queue: Xing Xu <xing.xu@intel.com>
Reviewed-by: Eric Karl <ericrk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594184}
  • Loading branch information
axinging authored and Commit Bot committed Sep 26, 2018
1 parent 34a921e commit d36a6cd
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 1 deletion.
3 changes: 3 additions & 0 deletions cc/base/unique_notifier.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,13 @@ UniqueNotifier::UniqueNotifier(base::SequencedTaskRunner* task_runner,
UniqueNotifier::~UniqueNotifier() = default;

void UniqueNotifier::Cancel() {
DCHECK(task_runner_->RunsTasksInCurrentSequence());
base::AutoLock hold(lock_);
notification_pending_ = false;
}

void UniqueNotifier::Schedule() {
DCHECK(task_runner_->RunsTasksInCurrentSequence());
base::AutoLock hold(lock_);
if (notification_pending_)
return;
Expand All @@ -38,6 +40,7 @@ void UniqueNotifier::Schedule() {
}

void UniqueNotifier::Notify() {
DCHECK(task_runner_->RunsTasksInCurrentSequence());
// Scope to release |lock_| before running the closure.
{
base::AutoLock hold(lock_);
Expand Down
2 changes: 2 additions & 0 deletions cc/base/unique_notifier.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ class SequencedTaskRunner;

namespace cc {

// Callers must ensure that they only schedule the notifier on the same thread
// that the provided |task_runner| runs on.
class CC_BASE_EXPORT UniqueNotifier {
public:
// Configure this notifier to issue the |closure| notification when scheduled.
Expand Down
21 changes: 20 additions & 1 deletion cc/base/unique_notifier_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,24 @@ class UniqueNotifierTest : public testing::Test {
int notification_count_;
};

// Need to guarantee that Schedule and Notify happen in the same thread.
// Multiple schedules may result in multiple runs when notify task is posted to
// a different thread. So we use thread checker to avoid this.
// Example which may result in multiple runs:
// base::Thread notifier_thread("NotifierThread");
// notifier_thread.Start();
// UniqueNotifier notifier(
// notifier_thread.task_runner().get(),
// base::BindRepeating(&UniqueNotifierTest::Notify,
// base::Unretained(this)));
// EXPECT_EQ(0, NotificationCount());
// for (int i = 0; i < 50000; ++i)
// notifier.Schedule();
// base::RunLoop().RunUntilIdle();

// notifier_thread.Stop();
// EXPECT_LE(1, NotificationCount());
// 50000 can be any number bigger than 1. The bigger the easier to more runs.
TEST_F(UniqueNotifierTest, Schedule) {
{
UniqueNotifier notifier(
Expand All @@ -43,7 +61,8 @@ TEST_F(UniqueNotifierTest, Schedule) {
base::RunLoop().RunUntilIdle();
EXPECT_EQ(1, NotificationCount());

// Multiple schedules should only result in one run.
// UniqueNotifier can only runs in the main thread, and multiple schedules
// should result in one run.
for (int i = 0; i < 5; ++i)
notifier.Schedule();

Expand Down

0 comments on commit d36a6cd

Please sign in to comment.