Skip to content

Commit

Permalink
New Sequence/Thread checking API.
Browse files Browse the repository at this point in the history 
Macro-based to be zero size cost in non-dcheck builds.

Based on discussion  @ https://groups.google.com/a/chromium.org/d/topic/chromium-dev/99pKNd2kCcg/discussion

BUG=714835

Review-Url: https://codereview.chromium.org/2869893003
Cr-Commit-Position: refs/heads/master@{#470804}
  • Loading branch information
gab authored and Commit bot committed May 11, 2017
1 parent 4184757 commit d52c912
Show file tree
Hide file tree
Showing 6 changed files with 250 additions and 83 deletions.
24 changes: 24 additions & 0 deletions PRESUBMIT.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,30 @@
True,
(),
),
(
'base::NonThreadSafe',
(
'base::NonThreadSafe is deprecated.',
),
True,
(),
),
(
'base::SequenceChecker',
(
'Consider using SEQUENCE_CHECKER macros instead of the class directly.',
),
False,
(),
),
(
'base::ThreadChecker',
(
'Consider using THREAD_CHECKER macros instead of the class directly.',
),
False,
(),
),
)


Expand Down
74 changes: 53 additions & 21 deletions base/sequence_checker.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,40 +5,72 @@
#ifndef BASE_SEQUENCE_CHECKER_H_
#define BASE_SEQUENCE_CHECKER_H_

#include "base/logging.h"
#include "base/sequence_checker_impl.h"

// SequenceChecker is a helper class used to help verify that some methods of a
// class are called sequentially (for thread-safety).
//
// Use the macros below instead of the SequenceChecker directly so that the
// unused member doesn't result in an extra byte (four when padded) per
// instance in production.
//
// This class is much prefered to ThreadChecker for thread-safety checks.
// ThreadChecker should only be used for classes that are truly thread-affine
// (use thread-local-storage or a third-party API that does).
//
// Usage:
// class MyClass {
// public:
// MyClass() {
// // It's sometimes useful to detach on construction for objects that are
// // constructed in one place and forever after used from another
// // sequence.
// DETACH_FROM_SEQUENCE(my_sequence_checker_);
// }
//
// ~MyClass() {
// // SequenceChecker doesn't automatically check it's destroyed on origin
// // sequence for the same reason it's sometimes detached in the
// // constructor. It's okay to destroy off sequence if the owner
// // otherwise knows usage on the associated sequence is done. If you're
// // not detaching in the constructor, you probably want to explicitly
// // check in the destructor.
// DCHECK_CALLED_ON_VALID_THREAD(my_thread_checker_);
// }
// void MyMethod() {
// DCHECK_CALLED_ON_VALID_SEQUENCE(my_sequence_checker_);
// ... (do stuff) ...
// }
//
// private:
// SEQUENCE_CHECKER(my_sequence_checker_);
// }

#if DCHECK_IS_ON()
#define SEQUENCE_CHECKER(name) base::SequenceChecker name
#define DCHECK_CALLED_ON_VALID_SEQUENCE(name) \
DCHECK((name).CalledOnValidSequence())
#define DETACH_FROM_SEQUENCE(name) (name).DetachFromSequence()
#else // DCHECK_IS_ON()
#define SEQUENCE_CHECKER(name)
#define DCHECK_CALLED_ON_VALID_SEQUENCE(name)
#define DETACH_FROM_SEQUENCE(name)
#endif // DCHECK_IS_ON()

namespace base {

// Do nothing implementation, for use in release mode.
//
// Note: You should almost always use the SequenceChecker class to get
// the right version for your build configuration.
// Note: You should almost always use the SequenceChecker class (through the
// above macros) to get the right version for your build configuration.
class SequenceCheckerDoNothing {
public:
bool CalledOnValidSequence() const { return true; }

void DetachFromSequence() {}
};

// SequenceChecker is a helper class to verify that calls to some methods of a
// class are sequenced. Calls are sequenced when they are issued:
// - From tasks posted to SequencedTaskRunners or SingleThreadTaskRunners bound
// to the same sequence, or,
// - From a single thread outside of any task.
//
// Example:
// class MyClass {
// public:
// void Foo() {
// DCHECK(sequence_checker_.CalledOnValidSequence());
// ... (do stuff) ...
// }
//
// private:
// SequenceChecker sequence_checker_;
// }
//
// In Release mode, CalledOnValidSequence() will always return true.
#if DCHECK_IS_ON()
class SequenceChecker : public SequenceCheckerImpl {
};
Expand Down
54 changes: 53 additions & 1 deletion base/sequence_checker_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "base/sequence_checker.h"

#include <stddef.h>

#include <memory>
Expand All @@ -12,9 +14,9 @@
#include "base/callback_forward.h"
#include "base/macros.h"
#include "base/message_loop/message_loop.h"
#include "base/sequence_checker_impl.h"
#include "base/sequence_token.h"
#include "base/single_thread_task_runner.h"
#include "base/test/gtest_util.h"
#include "base/test/sequenced_worker_pool_owner.h"
#include "base/threading/simple_thread.h"
#include "testing/gtest/include/gtest/gtest.h"
Expand Down Expand Up @@ -256,4 +258,54 @@ TEST_F(SequenceCheckerTest,
second_pool_owner.pool()->FlushForTesting();
}

namespace {

// This fixture is a helper for unit testing the sequence checker macros as it
// is not possible to inline ExpectDeathOnOtherSequence() and
// ExpectNoDeathOnOtherSequenceAfterDetach() as lambdas since binding
// |Unretained(&my_sequence_checker)| wouldn't compile on non-dcheck builds
// where it won't be defined.
class SequenceCheckerMacroTest : public SequenceCheckerTest {
public:
SequenceCheckerMacroTest() = default;

void ExpectDeathOnOtherSequence() {
#if DCHECK_IS_ON()
EXPECT_DCHECK_DEATH(
{ DCHECK_CALLED_ON_VALID_SEQUENCE(my_sequence_checker_); });
#else
// Happily no-ops on non-dcheck builds.
DCHECK_CALLED_ON_VALID_SEQUENCE(my_sequence_checker_);
#endif
}

void ExpectNoDeathOnOtherSequenceAfterDetach() {
DCHECK_CALLED_ON_VALID_SEQUENCE(my_sequence_checker_);
}

protected:
SEQUENCE_CHECKER(my_sequence_checker_);

private:
DISALLOW_COPY_AND_ASSIGN(SequenceCheckerMacroTest);
};

} // namespace

TEST_F(SequenceCheckerMacroTest, Macros) {
PostToSequencedWorkerPool(
Bind(&SequenceCheckerMacroTest::ExpectDeathOnOtherSequence,
Unretained(this)),
"A");
FlushSequencedWorkerPoolForTesting();

DETACH_FROM_SEQUENCE(my_sequence_checker_);

PostToSequencedWorkerPool(
Bind(&SequenceCheckerMacroTest::ExpectNoDeathOnOtherSequenceAfterDetach,
Unretained(this)),
"A");
FlushSequencedWorkerPoolForTesting();
}

} // namespace base
25 changes: 4 additions & 21 deletions base/threading/non_thread_safe.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,27 +30,10 @@ class NonThreadSafeDoNothing {
void DetachFromThread() {}
};

// NonThreadSafe is a helper class used to help verify that methods of a
// class are called from the same thread. One can inherit from this class
// and use CalledOnValidThread() to verify.
//
// This is intended to be used with classes that appear to be thread safe, but
// aren't. For example, a service or a singleton like the preferences system.
//
// Example:
// class MyClass : public base::NonThreadSafe {
// public:
// void Foo() {
// DCHECK(CalledOnValidThread());
// ... (do stuff) ...
// }
// }
//
// Note that base::ThreadChecker offers identical functionality to
// NonThreadSafe, but does not require inheritance. In general, it is preferable
// to have a base::ThreadChecker as a member, rather than inherit from
// NonThreadSafe. For more details about when to choose one over the other, see
// the documentation for base::ThreadChecker.
// DEPRECATED! Use base::SequenceChecker (for thread-safety) or
// base::ThreadChecker (for thread-affinity) -- see their documentation for
// details. Use a checker as a protected member instead of inheriting from
// NonThreadSafe if you need subclasses to have access.
#if DCHECK_IS_ON()
typedef NonThreadSafeImpl NonThreadSafe;
#else
Expand Down
106 changes: 67 additions & 39 deletions base/threading/thread_checker.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,73 @@
#include "base/logging.h"
#include "base/threading/thread_checker_impl.h"

// ThreadChecker is a helper class used to help verify that some methods of a
// class are called from the same thread (for thread-affinity).
//
// Use the macros below instead of the ThreadChecker directly so that the unused
// member doesn't result in an extra byte (four when padded) per instance in
// production.
//
// Usage of this class should be *rare* as most classes require thread-safety
// but not thread-affinity. Prefer base::SequenceChecker to verify thread-safe
// access.
//
// Thread-affinity checks should only be required in classes that use thread-
// local-storage or a third-party API that does.
//
// Prefer to encode the minimum requirements of each class instead of the
// environment it happens to run in today. e.g. if a class requires thread-
// safety but not thread-affinity, use a SequenceChecker even if it happens to
// run on a SingleThreadTaskRunner today. That makes it easier to understand
// what would need to change to turn that SingleThreadTaskRunner into a
// SequencedTaskRunner for ease of scheduling as well as minimizes side-effects
// if that change is made.
//
// Usage:
// class MyClass {
// public:
// MyClass() {
// // It's sometimes useful to detach on construction for objects that are
// // constructed in one place and forever after used from another
// // thread.
// DETACH_FROM_THREAD(my_thread_checker_);
// }
//
// ~MyClass() {
// // ThreadChecker doesn't automatically check it's destroyed on origin
// // thread for the same reason it's sometimes detached in the
// // constructor. It's okay to destroy off thread if the owner otherwise
// // knows usage on the associated thread is done. If you're not
// // detaching in the constructor, you probably want to explicitly check
// // in the destructor.
// DCHECK_CALLED_ON_VALID_THREAD(my_thread_checker_);
// }
//
// void MyMethod() {
// DCHECK_CALLED_ON_VALID_THREAD(my_thread_checker_);
// ... (do stuff) ...
// }
//
// private:
// THREAD_CHECKER(my_thread_checker_);
// }

#if DCHECK_IS_ON()
#define THREAD_CHECKER(name) base::ThreadChecker name
#define DCHECK_CALLED_ON_VALID_THREAD(name) DCHECK((name).CalledOnValidThread())
#define DETACH_FROM_THREAD(name) (name).DetachFromThread()
#else // DCHECK_IS_ON()
#define THREAD_CHECKER(name)
#define DCHECK_CALLED_ON_VALID_THREAD(name)
#define DETACH_FROM_THREAD(name)
#endif // DCHECK_IS_ON()

namespace base {

// Do nothing implementation, for use in release mode.
//
// Note: You should almost always use the ThreadChecker class to get the
// right version for your build configuration.
// Note: You should almost always use the ThreadChecker class (through the above
// macros) to get the right version for your build configuration.
class ThreadCheckerDoNothing {
public:
bool CalledOnValidThread() const WARN_UNUSED_RESULT {
Expand All @@ -23,43 +84,10 @@ class ThreadCheckerDoNothing {
void DetachFromThread() {}
};

// ThreadChecker is a helper class used to help verify that some methods of a
// class are called from the same thread. It provides identical functionality to
// base::NonThreadSafe, but it is meant to be held as a member variable, rather
// than inherited from base::NonThreadSafe.
//
// While inheriting from base::NonThreadSafe may give a clear indication about
// the thread-safety of a class, it may also lead to violations of the style
// guide with regard to multiple inheritance. The choice between having a
// ThreadChecker member and inheriting from base::NonThreadSafe should be based
// on whether:
// - Derived classes need to know the thread they belong to, as opposed to
// having that functionality fully encapsulated in the base class.
// - Derived classes should be able to reassign the base class to another
// thread, via DetachFromThread.
//
// If neither of these are true, then having a ThreadChecker member and calling
// CalledOnValidThread is the preferable solution.
//
// Example:
// class MyClass {
// public:
// void Foo() {
// DCHECK(thread_checker_.CalledOnValidThread());
// ... (do stuff) ...
// }
//
// private:
// ThreadChecker thread_checker_;
// }
//
// Note that, when enabled, CalledOnValidThread() returns false when called from
// tasks posted to SingleThreadTaskRunners bound to different sequences, even if
// the tasks happen to run on the same thread (e.g. two independent TaskRunners
// with ExecutionMode::SINGLE_THREADED on the TaskScheduler that happen to share
// a thread).
//
// In Release mode, CalledOnValidThread will always return true.
// Note that ThreadCheckerImpl::CalledOnValidThread() returns false when called
// from tasks posted to SingleThreadTaskRunners bound to different sequences,
// even if the tasks happen to run on the same thread (e.g. two independent
// SingleThreadTaskRunners on the TaskScheduler that happen to share a thread).
#if DCHECK_IS_ON()
class ThreadChecker : public ThreadCheckerImpl {
};
Expand Down
Loading

0 comments on commit d52c912

Please sign in to comment.