Commit 23107218 authored by Francois Doray's avatar Francois Doray Committed by Commit Bot

Introduce new thread restrictions API.

Benefits of the new API:
- Naming consistent with TaskTraits.
- Separate friend lists for:
  - Allow blocking
  - Allow //base sync primitives in a scope that allows blocking
  - Allow //base sync primitives in a scope that doesn't allow blocking
- SetIOAllowed(true) and SetWaitAllowed(true) are replaced with 
  ScopedAllow* classes. 

Design doc:
https://docs.google.com/document/d/1zRg0P_bQYE8Oc5x5Shrd4CIkIevPiMBzxgq6awRrt-A/edit?usp=sharing

Bug: 766678
Change-Id: I0b2217f19562a2c7f1b08ca92a0f474e90ac4548
Reviewed-on: https://chromium-review.googlesource.com/672664
Commit-Queue: Francois Doray <fdoray@chromium.org>
Reviewed-by: default avatarGabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504053}
parent 406bae4c
......@@ -2191,6 +2191,7 @@ test("base_unittests") {
"threading/thread_id_name_manager_unittest.cc",
"threading/thread_local_storage_unittest.cc",
"threading/thread_local_unittest.cc",
"threading/thread_restrictions_unittest.cc",
"threading/thread_task_runner_handle_unittest.cc",
"threading/thread_unittest.cc",
"threading/watchdog_unittest.cc",
......
......@@ -14,35 +14,120 @@ namespace base {
namespace {
LazyInstance<ThreadLocalBoolean>::Leaky
g_io_disallowed = LAZY_INSTANCE_INITIALIZER;
LazyInstance<ThreadLocalBoolean>::Leaky g_blocking_disallowed =
LAZY_INSTANCE_INITIALIZER;
LazyInstance<ThreadLocalBoolean>::Leaky
g_singleton_disallowed = LAZY_INSTANCE_INITIALIZER;
LazyInstance<ThreadLocalBoolean>::Leaky
g_wait_disallowed = LAZY_INSTANCE_INITIALIZER;
LazyInstance<ThreadLocalBoolean>::Leaky g_base_sync_primitives_disallowed =
LAZY_INSTANCE_INITIALIZER;
} // namespace
void AssertBlockingAllowed() {
DCHECK(!g_blocking_disallowed.Get().Get())
<< "Function marked as blocking was called from a scope that disallows "
"blocking! If this task is running inside the TaskScheduler, it needs "
"to have MayBlock() in its TaskTraits. Otherwise, consider making "
"this blocking work asynchronous or, as a last resort, you may use "
"ScopedAllowBlocking in a narrow scope.";
}
void DisallowBlocking() {
g_blocking_disallowed.Get().Set(true);
}
ScopedDisallowBlocking::ScopedDisallowBlocking()
: was_disallowed_(g_blocking_disallowed.Get().Get()) {
g_blocking_disallowed.Get().Set(true);
}
ScopedDisallowBlocking::~ScopedDisallowBlocking() {
DCHECK(g_blocking_disallowed.Get().Get());
g_blocking_disallowed.Get().Set(was_disallowed_);
}
ScopedAllowBlocking::ScopedAllowBlocking()
: was_disallowed_(g_blocking_disallowed.Get().Get()) {
g_blocking_disallowed.Get().Set(false);
}
ScopedAllowBlocking::~ScopedAllowBlocking() {
DCHECK(!g_blocking_disallowed.Get().Get());
g_blocking_disallowed.Get().Set(was_disallowed_);
}
void DisallowBaseSyncPrimitives() {
g_base_sync_primitives_disallowed.Get().Set(true);
}
ScopedAllowBaseSyncPrimitives::ScopedAllowBaseSyncPrimitives()
: was_disallowed_(g_base_sync_primitives_disallowed.Get().Get()) {
DCHECK(!g_blocking_disallowed.Get().Get())
<< "To allow //base sync primitives in a scope where blocking is "
"disallowed use ScopedAllowBaseSyncPrimitivesOutsideBlockingScope.";
g_base_sync_primitives_disallowed.Get().Set(false);
}
ScopedAllowBaseSyncPrimitives::~ScopedAllowBaseSyncPrimitives() {
DCHECK(!g_base_sync_primitives_disallowed.Get().Get());
g_base_sync_primitives_disallowed.Get().Set(was_disallowed_);
}
ScopedAllowBaseSyncPrimitivesOutsideBlockingScope::
ScopedAllowBaseSyncPrimitivesOutsideBlockingScope()
: was_disallowed_(g_base_sync_primitives_disallowed.Get().Get()) {
g_base_sync_primitives_disallowed.Get().Set(false);
}
ScopedAllowBaseSyncPrimitivesOutsideBlockingScope::
~ScopedAllowBaseSyncPrimitivesOutsideBlockingScope() {
DCHECK(!g_base_sync_primitives_disallowed.Get().Get());
g_base_sync_primitives_disallowed.Get().Set(was_disallowed_);
}
ScopedAllowBaseSyncPrimitivesForTesting::
ScopedAllowBaseSyncPrimitivesForTesting()
: was_disallowed_(g_base_sync_primitives_disallowed.Get().Get()) {
g_base_sync_primitives_disallowed.Get().Set(false);
}
ScopedAllowBaseSyncPrimitivesForTesting::
~ScopedAllowBaseSyncPrimitivesForTesting() {
DCHECK(!g_base_sync_primitives_disallowed.Get().Get());
g_base_sync_primitives_disallowed.Get().Set(was_disallowed_);
}
namespace internal {
void AssertBaseSyncPrimitivesAllowed() {
DCHECK(!g_base_sync_primitives_disallowed.Get().Get())
<< "Waiting on a //base sync primitive is not allowed on this thread to "
"prevent jank and deadlock. If waiting on a //base sync primitive is "
"unavoidable, do it within the scope of a "
"ScopedAllowBaseSyncPrimitives. If in a test, "
"use ScopedAllowBaseSyncPrimitivesForTesting.";
}
void ResetThreadRestrictionsForTesting() {
g_blocking_disallowed.Get().Set(false);
g_singleton_disallowed.Get().Set(false);
g_base_sync_primitives_disallowed.Get().Set(false);
}
} // namespace internal
// static
bool ThreadRestrictions::SetIOAllowed(bool allowed) {
bool previous_disallowed = g_io_disallowed.Get().Get();
g_io_disallowed.Get().Set(!allowed);
bool previous_disallowed = g_blocking_disallowed.Get().Get();
g_blocking_disallowed.Get().Set(!allowed);
return !previous_disallowed;
}
// static
void ThreadRestrictions::AssertIOAllowed() {
if (g_io_disallowed.Get().Get()) {
NOTREACHED() << "Function marked as IO-only was called from a thread that "
"disallows IO! If this thread really should be allowed to "
"make IO calls, adjust the call to "
"base::ThreadRestrictions::SetIOAllowed() in this thread's "
"startup. If this task is running inside the "
"TaskScheduler, the TaskRunner used to post it needs to "
"have MayBlock() in its TaskTraits.";
}
AssertBlockingAllowed();
}
// static
......@@ -68,22 +153,17 @@ void ThreadRestrictions::AssertSingletonAllowed() {
// static
void ThreadRestrictions::DisallowWaiting() {
g_wait_disallowed.Get().Set(true);
DisallowBaseSyncPrimitives();
}
// static
void ThreadRestrictions::AssertWaitAllowed() {
if (g_wait_disallowed.Get().Get()) {
NOTREACHED() << "Waiting is not allowed to be used on this thread to "
<< "prevent jank and deadlock. If this task is running "
"inside the TaskScheduler, the TaskRunner used to post it "
"needs to have WithBaseSyncPrimitives() in its TaskTraits.";
}
internal::AssertBaseSyncPrimitivesAllowed();
}
bool ThreadRestrictions::SetWaitAllowed(bool allowed) {
bool previous_disallowed = g_wait_disallowed.Get().Get();
g_wait_disallowed.Get().Set(!allowed);
bool previous_disallowed = g_base_sync_primitives_disallowed.Get().Get();
g_base_sync_primitives_disallowed.Get().Set(!allowed);
return !previous_disallowed;
}
......
......@@ -6,6 +6,7 @@
#define BASE_THREADING_THREAD_RESTRICTIONS_H_
#include "base/base_export.h"
#include "base/gtest_prod_util.h"
#include "base/logging.h"
#include "base/macros.h"
......@@ -108,6 +109,168 @@ class StackSamplingProfiler;
class Thread;
class ThreadTestHelper;
#if DCHECK_IS_ON()
#define INLINE_IF_DCHECK_IS_OFF BASE_EXPORT
#define EMPTY_BODY_IF_DCHECK_IS_OFF
#else
#define INLINE_IF_DCHECK_IS_OFF inline
#define EMPTY_BODY_IF_DCHECK_IS_OFF \
{}
#endif
// A "blocking call" refers to any call that causes the calling thread to wait
// off-CPU. It includes but is not limited to calls that wait on synchronous
// file I/O operations: read or write a file from disk, interact with a pipe or
// a socket, rename or delete a file, enumerate files in a directory, etc.
// Acquiring a low contention lock is not considered a blocking call.
// Asserts that blocking calls are allowed in the current scope.
INLINE_IF_DCHECK_IS_OFF void AssertBlockingAllowed()
EMPTY_BODY_IF_DCHECK_IS_OFF;
// Disallows blocking on the current thread.
INLINE_IF_DCHECK_IS_OFF void DisallowBlocking() EMPTY_BODY_IF_DCHECK_IS_OFF;
// Disallows blocking calls within its scope.
class BASE_EXPORT ScopedDisallowBlocking {
public:
ScopedDisallowBlocking() EMPTY_BODY_IF_DCHECK_IS_OFF;
~ScopedDisallowBlocking() EMPTY_BODY_IF_DCHECK_IS_OFF;
private:
#if DCHECK_IS_ON()
const bool was_disallowed_;
#endif
DISALLOW_COPY_AND_ASSIGN(ScopedDisallowBlocking);
};
// ScopedAllowBlocking(ForTesting) allow blocking calls within a scope where
// they are normally disallowed.
//
// Avoid using this. Prefer making blocking calls from tasks posted to
// base::TaskScheduler with base::MayBlock().
class BASE_EXPORT ScopedAllowBlocking {
private:
// This can only be instantiated by friends. Use ScopedAllowBlockingForTesting
// in unit tests to avoid the friend requirement.
FRIEND_TEST_ALL_PREFIXES(ThreadRestrictionsTest, ScopedAllowBlocking);
friend class ScopedAllowBlockingForTesting;
ScopedAllowBlocking() EMPTY_BODY_IF_DCHECK_IS_OFF;
~ScopedAllowBlocking() EMPTY_BODY_IF_DCHECK_IS_OFF;
#if DCHECK_IS_ON()
const bool was_disallowed_;
#endif
DISALLOW_COPY_AND_ASSIGN(ScopedAllowBlocking);
};
class ScopedAllowBlockingForTesting {
public:
ScopedAllowBlockingForTesting() {}
~ScopedAllowBlockingForTesting() {}
private:
#if DCHECK_IS_ON()
ScopedAllowBlocking scoped_allow_blocking_;
#endif
DISALLOW_COPY_AND_ASSIGN(ScopedAllowBlockingForTesting);
};
// "Waiting on a //base sync primitive" refers to calling
// base::WaitableEvent::*Wait* or base::ConditionVariable::*Wait*.
// Disallows waiting on a //base sync primitive on the current thread.
INLINE_IF_DCHECK_IS_OFF void DisallowBaseSyncPrimitives()
EMPTY_BODY_IF_DCHECK_IS_OFF;
// ScopedAllowBaseSyncPrimitives(ForTesting)(OutsideBlockingScope) allow waiting
// on a //base sync primitive within a scope where this is normally disallowed.
//
// Avoid using this. Instead of waiting on a WaitableEvent or a
// ConditionVariable, put the work that should happen after the wait in a
// callback and post that callback from where the WaitableEvent or
// ConditionVariable would have been signaled. If something needs to be
// scheduled after many tasks have executed, use base::BarrierClosure.
// This can only be used in a scope where blocking is allowed.
class BASE_EXPORT ScopedAllowBaseSyncPrimitives {
private:
// This can only be instantiated by friends. Use
// ScopedAllowBaseSyncPrimitivesForTesting in unit tests to avoid the friend
// requirement.
FRIEND_TEST_ALL_PREFIXES(ThreadRestrictionsTest,
ScopedAllowBaseSyncPrimitives);
FRIEND_TEST_ALL_PREFIXES(ThreadRestrictionsTest,
ScopedAllowBaseSyncPrimitivesResetsState);
FRIEND_TEST_ALL_PREFIXES(ThreadRestrictionsTest,
ScopedAllowBaseSyncPrimitivesWithBlockingDisallowed);
ScopedAllowBaseSyncPrimitives() EMPTY_BODY_IF_DCHECK_IS_OFF;
~ScopedAllowBaseSyncPrimitives() EMPTY_BODY_IF_DCHECK_IS_OFF;
#if DCHECK_IS_ON()
const bool was_disallowed_;
#endif
DISALLOW_COPY_AND_ASSIGN(ScopedAllowBaseSyncPrimitives);
};
// This can be used in a scope where blocking is disallowed.
class BASE_EXPORT ScopedAllowBaseSyncPrimitivesOutsideBlockingScope {
private:
// This can only be instantiated by friends. Use
// ScopedAllowBaseSyncPrimitivesForTesting in unit tests to avoid the friend
// requirement.
FRIEND_TEST_ALL_PREFIXES(ThreadRestrictionsTest,
ScopedAllowBaseSyncPrimitivesOutsideBlockingScope);
FRIEND_TEST_ALL_PREFIXES(
ThreadRestrictionsTest,
ScopedAllowBaseSyncPrimitivesOutsideBlockingScopeResetsState);
ScopedAllowBaseSyncPrimitivesOutsideBlockingScope()
EMPTY_BODY_IF_DCHECK_IS_OFF;
~ScopedAllowBaseSyncPrimitivesOutsideBlockingScope()
EMPTY_BODY_IF_DCHECK_IS_OFF;
#if DCHECK_IS_ON()
const bool was_disallowed_;
#endif
DISALLOW_COPY_AND_ASSIGN(ScopedAllowBaseSyncPrimitivesOutsideBlockingScope);
};
// This can be used in tests without being a friend of
// ScopedAllowBaseSyncPrimitives(OutsideBlockingScope).
class BASE_EXPORT ScopedAllowBaseSyncPrimitivesForTesting {
public:
ScopedAllowBaseSyncPrimitivesForTesting() EMPTY_BODY_IF_DCHECK_IS_OFF;
~ScopedAllowBaseSyncPrimitivesForTesting() EMPTY_BODY_IF_DCHECK_IS_OFF;
private:
#if DCHECK_IS_ON()
const bool was_disallowed_;
#endif
DISALLOW_COPY_AND_ASSIGN(ScopedAllowBaseSyncPrimitivesForTesting);
};
namespace internal {
// Asserts that waiting on a //base sync primitive is allowed in the current
// scope.
INLINE_IF_DCHECK_IS_OFF void AssertBaseSyncPrimitivesAllowed()
EMPTY_BODY_IF_DCHECK_IS_OFF;
// Resets all thread restrictions on the current thread.
INLINE_IF_DCHECK_IS_OFF void ResetThreadRestrictionsForTesting()
EMPTY_BODY_IF_DCHECK_IS_OFF;
} // namespace internal
// Certain behavior is disallowed on certain threads. ThreadRestrictions helps
// enforce these rules. Examples of such rules:
//
......@@ -138,6 +301,8 @@ class BASE_EXPORT ThreadRestrictions {
public:
// Constructing a ScopedAllowIO temporarily allows IO for the current
// thread. Doing this is almost certainly always incorrect.
//
// DEPRECATED. Use ScopedAllowBlocking(ForTesting).
class BASE_EXPORT ScopedAllowIO {
public:
ScopedAllowIO() { previous_value_ = SetIOAllowed(true); }
......@@ -153,11 +318,15 @@ class BASE_EXPORT ThreadRestrictions {
// Set whether the current thread to make IO calls.
// Threads start out in the *allowed* state.
// Returns the previous value.
//
// DEPRECATED. Use ScopedAllowBlocking(ForTesting) or ScopedDisallowBlocking.
static bool SetIOAllowed(bool allowed);
// Check whether the current thread is allowed to make IO calls,
// and DCHECK if not. See the block comment above the class for
// a discussion of where to add these checks.
//
// DEPRECATED. Use AssertBlockingAllowed.
static void AssertIOAllowed();
// Set whether the current thread can use singletons. Returns the previous
......@@ -170,9 +339,13 @@ class BASE_EXPORT ThreadRestrictions {
// Disable waiting on the current thread. Threads start out in the *allowed*
// state. Returns the previous value.
//
// DEPRECATED. Use DisallowBaseSyncPrimitives.
static void DisallowWaiting();
// Check whether the current thread is allowed to wait, and DCHECK if not.
//
// DEPRECATED. Use AssertBaseSyncPrimitivesAllowed.
static void AssertWaitAllowed();
#else
// Inline the empty definitions of these functions so that they can be
......@@ -247,6 +420,7 @@ class BASE_EXPORT ThreadRestrictions {
// END USAGE THAT NEEDS TO BE FIXED.
#if DCHECK_IS_ON()
// DEPRECATED. Use ScopedAllowBaseSyncPrimitives.
static bool SetWaitAllowed(bool allowed);
#else
static bool SetWaitAllowed(bool allowed) { return true; }
......@@ -256,6 +430,8 @@ class BASE_EXPORT ThreadRestrictions {
// thread. Doing this is almost always incorrect, which is why we limit who
// can use this through friend. If you find yourself needing to use this, find
// another way. Talk to jam or brettw.
//
// DEPRECATED. Use ScopedAllowBaseSyncPrimitives.
class BASE_EXPORT ScopedAllowWait {
public:
ScopedAllowWait() { previous_value_ = SetWaitAllowed(true); }
......
// Copyright 2017 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "base/threading/thread_restrictions.h"
#include <utility>
#include "base/bind.h"
#include "base/callback.h"
#include "base/macros.h"
#include "base/test/gtest_util.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace base {
namespace {
class ThreadRestrictionsTest : public testing::Test {
public:
ThreadRestrictionsTest() = default;
~ThreadRestrictionsTest() override {
internal::ResetThreadRestrictionsForTesting();
}
private:
DISALLOW_COPY_AND_ASSIGN(ThreadRestrictionsTest);
};
} // namespace
TEST_F(ThreadRestrictionsTest, BlockingAllowedByDefault) {
AssertBlockingAllowed();
}
TEST_F(ThreadRestrictionsTest, ScopedDisallowBlocking) {
{
ScopedDisallowBlocking scoped_disallow_blocking;
EXPECT_DCHECK_DEATH({ AssertBlockingAllowed(); });
}
AssertBlockingAllowed();
}
TEST_F(ThreadRestrictionsTest, ScopedAllowBlocking) {
ScopedDisallowBlocking scoped_disallow_blocking;
{
ScopedAllowBlocking scoped_allow_blocking;
AssertBlockingAllowed();
}
EXPECT_DCHECK_DEATH({ AssertBlockingAllowed(); });
}
TEST_F(ThreadRestrictionsTest, ScopedAllowBlockingForTesting) {
ScopedDisallowBlocking scoped_disallow_blocking;
{
ScopedAllowBlockingForTesting scoped_allow_blocking_for_testing;
AssertBlockingAllowed();
}
EXPECT_DCHECK_DEATH({ AssertBlockingAllowed(); });
}
TEST_F(ThreadRestrictionsTest, BaseSyncPrimitivesAllowedByDefault) {}
TEST_F(ThreadRestrictionsTest, DisallowBaseSyncPrimitives) {
DisallowBaseSyncPrimitives();
EXPECT_DCHECK_DEATH({ internal::AssertBaseSyncPrimitivesAllowed(); });
}
TEST_F(ThreadRestrictionsTest, ScopedAllowBaseSyncPrimitives) {
DisallowBaseSyncPrimitives();
ScopedAllowBaseSyncPrimitives scoped_allow_base_sync_primitives;
internal::AssertBaseSyncPrimitivesAllowed();
}
TEST_F(ThreadRestrictionsTest, ScopedAllowBaseSyncPrimitivesResetsState) {
DisallowBaseSyncPrimitives();
{ ScopedAllowBaseSyncPrimitives scoped_allow_base_sync_primitives; }
EXPECT_DCHECK_DEATH({ internal::AssertBaseSyncPrimitivesAllowed(); });
}
TEST_F(ThreadRestrictionsTest,
ScopedAllowBaseSyncPrimitivesWithBlockingDisallowed) {
ScopedDisallowBlocking scoped_disallow_blocking;
DisallowBaseSyncPrimitives();
// This should DCHECK because blocking is not allowed in this scope
// and OutsideBlockingScope is not passed to the constructor.
EXPECT_DCHECK_DEATH(
{ ScopedAllowBaseSyncPrimitives scoped_allow_base_sync_primitives; });
}
TEST_F(ThreadRestrictionsTest,
ScopedAllowBaseSyncPrimitivesOutsideBlockingScope) {
ScopedDisallowBlocking scoped_disallow_blocking;
DisallowBaseSyncPrimitives();
ScopedAllowBaseSyncPrimitivesOutsideBlockingScope
scoped_allow_base_sync_primitives;
internal::AssertBaseSyncPrimitivesAllowed();
}
TEST_F(ThreadRestrictionsTest,
ScopedAllowBaseSyncPrimitivesOutsideBlockingScopeResetsState) {
DisallowBaseSyncPrimitives();
{
ScopedAllowBaseSyncPrimitivesOutsideBlockingScope
scoped_allow_base_sync_primitives;
}
EXPECT_DCHECK_DEATH({ internal::AssertBaseSyncPrimitivesAllowed(); });
}
TEST_F(ThreadRestrictionsTest, ScopedAllowBaseSyncPrimitivesForTesting) {
DisallowBaseSyncPrimitives();
ScopedAllowBaseSyncPrimitivesForTesting
scoped_allow_base_sync_primitives_for_testing;
internal::AssertBaseSyncPrimitivesAllowed();
}
TEST_F(ThreadRestrictionsTest,
ScopedAllowBaseSyncPrimitivesForTestingResetsState) {
DisallowBaseSyncPrimitives();
{
ScopedAllowBaseSyncPrimitivesForTesting
scoped_allow_base_sync_primitives_for_testing;
}
EXPECT_DCHECK_DEATH({ internal::AssertBaseSyncPrimitivesAllowed(); });
}
TEST_F(ThreadRestrictionsTest,
ScopedAllowBaseSyncPrimitivesForTestingWithBlockingDisallowed) {
ScopedDisallowBlocking scoped_disallow_blocking;
DisallowBaseSyncPrimitives();
// This should not DCHECK.
ScopedAllowBaseSyncPrimitivesForTesting
scoped_allow_base_sync_primitives_for_testing;
}
} // namespace base
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment