Skip to content

Commit bcf6729

Browse files
jmarantzjmarantz
authored
thread: Set up MainThread and TestThread contexts as RAII for more robust isMainThread checking (#18200)
Resolves an issue where people were adding ASSERT(Thread::MainThread::isMainThread()) into their systems, when in at least some cases that test was not detecting anything (failed open). This change enforces that we always create the singleton. It also protects (somewhat) against inter-test leakage (I hate singletons) by using RAII to declare threads, and tracking nested instantiations of TestThread and MainThread, forcing them to be consistent. I don't actually anticipate TestThread ever changing, or needing this nested behavior, since we can declare that in main(). However, the main thread can be re-spawned between tests, and I suspect that some tests (fuzzing?) don't block till the previous test is completely quiesced. Previous to this PR that would lead to undefined behavior, and in this PR due to the nested tracking, it should ASSERT so that if such a leakage does occur, we'll learn about it. Signed-off-by: Joshua Marantz <[email protected]>
1 parent 72f7223 commit bcf6729

File tree

15 files changed

+152
-59
lines changed

15 files changed

+152
-59
lines changed

‎source/common/common/BUILD‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -372,8 +372,8 @@ envoy_cc_library(
372372
hdrs = ["thread.h"],
373373
external_deps = ["abseil_synchronization"],
374374
deps = envoy_cc_platform_dep("thread_impl_lib") + [
375+
":macros",
375376
":non_copyable",
376-
"//source/common/singleton:threadsafe_singleton",
377377
],
378378
)
379379

‎source/common/common/thread.cc‎

Lines changed: 101 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,36 +1,113 @@
11
#include "source/common/common/thread.h"
22

3+
#include <thread>
4+
5+
#include "source/common/common/assert.h"
6+
#include "source/common/common/macros.h"
7+
38
namespace Envoy {
49
namespace Thread {
510

6-
bool MainThread::isMainThread() {
7-
// If threading is off, only main thread is running.
8-
auto main_thread_singleton = MainThreadSingleton::getExisting();
9-
if (main_thread_singleton == nullptr) {
10-
return true;
11+
namespace {
12+
13+
// Singleton structure capturing which thread is the main dispatcher thread, and
14+
// which is the test thread. This info is used for assertions around catching
15+
// exceptions and accessing data structures which are not mutex-protected, and
16+
// are expected only from the main thread.
17+
//
18+
// TODO(jmarantz): avoid the singleton and instead have this object owned
19+
// by the ThreadFactory. That will require plumbing the API::API into all
20+
// call-sites for isMainThread(), which might be a bit of work, but will make
21+
// tests more hermetic.
22+
struct ThreadIds {
23+
// Determines whether we are currently running on the main-thread or
24+
// test-thread. We need to allow for either one because we don't establish
25+
// the full threading model in all unit tests.
26+
bool inMainOrTestThread() const {
27+
// We don't take the lock when testing the thread IDs, as they are atomic,
28+
// and are cleared when being released. All possible thread orderings
29+
// result in the correct result even without a lock.
30+
std::thread::id id = std::this_thread::get_id();
31+
return main_thread_id_ == id || test_thread_id_ == id;
1132
}
12-
// When threading is on, compare thread id with main thread id.
13-
return main_thread_singleton->inMainThread() || main_thread_singleton->inTestThread();
14-
}
15-
16-
void MainThread::clear() {
17-
delete MainThreadSingleton::getExisting();
18-
MainThreadSingleton::clear();
19-
}
20-
21-
void MainThread::initTestThread() {
22-
if (!initialized()) {
23-
MainThreadSingleton::initialize(new MainThread());
33+
34+
// Returns a singleton instance of this. The instance is never freed.
35+
static ThreadIds& get() { MUTABLE_CONSTRUCT_ON_FIRST_USE(ThreadIds); }
36+
37+
// Call this when the MainThread exits. Nested semantics are supported, so
38+
// that if multiple MainThread instances are declared, we unwind them
39+
// properly.
40+
void releaseMainThread() {
41+
absl::MutexLock lock(&mutex_);
42+
ASSERT(main_thread_use_count_ > 0);
43+
ASSERT(std::this_thread::get_id() == main_thread_id_);
44+
if (--main_thread_use_count_ == 0) {
45+
// Clearing the thread ID when its use-count goes to zero allows us
46+
// to read the atomic without taking a lock.
47+
main_thread_id_ = std::thread::id{};
48+
}
49+
}
50+
51+
// Call this when the TestThread exits. Nested semantics are supported, so
52+
// that if multiple TestThread instances are declared, we unwind them
53+
// properly.
54+
void releaseTestThread() {
55+
absl::MutexLock lock(&mutex_);
56+
ASSERT(test_thread_use_count_ > 0);
57+
ASSERT(std::this_thread::get_id() == test_thread_id_);
58+
if (--test_thread_use_count_ == 0) {
59+
// Clearing the thread ID when its use-count goes to zero allows us
60+
// to read the atomic without taking a lock.
61+
test_thread_id_ = std::thread::id{};
62+
}
63+
}
64+
65+
// Declares current thread as the main one, or verifies that the current
66+
// thread matches any previous declarations.
67+
void registerMainThread() {
68+
absl::MutexLock lock(&mutex_);
69+
if (++main_thread_use_count_ > 1) {
70+
ASSERT(std::this_thread::get_id() == main_thread_id_);
71+
} else {
72+
main_thread_id_ = std::this_thread::get_id();
73+
}
2474
}
25-
MainThreadSingleton::get().registerTestThread();
26-
}
2775

28-
void MainThread::initMainThread() {
29-
if (!initialized()) {
30-
MainThreadSingleton::initialize(new MainThread());
76+
// Declares current thread as the test thread, or verifies that the current
77+
// thread matches any previous declarations.
78+
void registerTestThread() {
79+
absl::MutexLock lock(&mutex_);
80+
if (++test_thread_use_count_ > 1) {
81+
ASSERT(std::this_thread::get_id() == test_thread_id_);
82+
} else {
83+
test_thread_id_ = std::this_thread::get_id();
84+
}
3185
}
32-
MainThreadSingleton::get().registerMainThread();
33-
}
86+
87+
private:
88+
// The atomic thread IDs can be read without a mutex, but they are written
89+
// under a mutex so that they are consistent with their use_counts. this
90+
// avoids the possibility of two threads racing to claim being the main/test
91+
// thread.
92+
std::atomic<std::thread::id> main_thread_id_;
93+
std::atomic<std::thread::id> test_thread_id_;
94+
95+
int32_t main_thread_use_count_ GUARDED_BY(mutex_) = 0;
96+
int32_t test_thread_use_count_ GUARDED_BY(mutex_) = 0;
97+
mutable absl::Mutex mutex_;
98+
};
99+
100+
} // namespace
101+
102+
bool MainThread::isMainThread() { return ThreadIds::get().inMainOrTestThread(); }
103+
104+
TestThread::TestThread() { ThreadIds::get().registerTestThread(); }
105+
106+
TestThread::~TestThread() { ThreadIds::get().releaseTestThread(); }
107+
108+
MainThread::MainThread() { ThreadIds::get().registerMainThread(); }
109+
110+
MainThread::~MainThread() { ThreadIds::get().releaseMainThread(); }
34111

35112
} // namespace Thread
36113
} // namespace Envoy

‎source/common/common/thread.h‎

Lines changed: 32 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
#include "envoy/thread/thread.h"
99

1010
#include "source/common/common/non_copyable.h"
11-
#include "source/common/singleton/threadsafe_singleton.h"
1211

1312
#include "absl/synchronization/mutex.h"
1413

@@ -169,35 +168,40 @@ class AtomicPtr : private AtomicPtrArray<T, 1, alloc_mode> {
169168
T* get(const MakeObject& make_object) { return BaseClass::get(0, make_object); }
170169
};
171170

172-
struct MainThread {
173-
using MainThreadSingleton = InjectableSingleton<MainThread>;
174-
bool inMainThread() const { return main_thread_id_ == std::this_thread::get_id(); }
175-
bool inTestThread() const {
176-
return test_thread_id_.has_value() && (test_thread_id_.value() == std::this_thread::get_id());
177-
}
178-
void registerTestThread() { test_thread_id_ = std::this_thread::get_id(); }
179-
void registerMainThread() { main_thread_id_ = std::this_thread::get_id(); }
180-
static bool initialized() { return MainThreadSingleton::getExisting() != nullptr; }
181-
/*
182-
* Register the main thread id, should be called in main thread before threading is on. Currently
183-
* called in ThreadLocal::InstanceImpl().
184-
*/
185-
static void initMainThread();
186-
/*
187-
* Register the test thread id, should be called in test thread before threading is on. Allow
188-
* some main thread only code to be executed on test thread.
189-
*/
190-
static void initTestThread();
191-
/*
192-
* Delete the main thread singleton, should be called in main thread after threading
193-
* has been shut down. Currently called in ~ThreadLocal::InstanceImpl().
171+
// RAII object to declare the TestThread. This should be declared in main() or
172+
// equivalent for any test binaries.
173+
//
174+
// Generally we expect TestThread to be instantiated only once on main() for
175+
// each test binary, though nested instantiations are allowed as long as the
176+
// thread ID does not change.
177+
class TestThread {
178+
public:
179+
TestThread();
180+
~TestThread();
181+
};
182+
183+
// RAII object to declare the MainThread. This should be declared in the thread
184+
// function or equivalent.
185+
//
186+
// Generally we expect MainThread to be instantiated only once or twice. It has
187+
// to be instantiated prior to OptionsImpl being created, so it needs to be in
188+
// instantiated from main_common(). In addition, it is instantiated by
189+
// ThreadLocal implementation to get the correct behavior for tests that do not
190+
// instantiate main.
191+
//
192+
// In general, nested instantiations are allowed as long as the thread ID does
193+
// not change.
194+
class MainThread {
195+
public:
196+
MainThread();
197+
~MainThread();
198+
199+
/**
200+
* Returns whether the current thread is the main thread or test thread.
201+
*
202+
* TODO(jmarantz): rename to isMainOrTestThread().
194203
*/
195-
static void clear();
196204
static bool isMainThread();
197-
198-
private:
199-
std::thread::id main_thread_id_;
200-
absl::optional<std::thread::id> test_thread_id_;
201205
};
202206

203207
// To improve exception safety in data plane, we plan to forbid the use of raw try in the core code

‎source/common/thread_local/thread_local_impl.cc‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ InstanceImpl::~InstanceImpl() {
1919
ASSERT(Thread::MainThread::isMainThread());
2020
ASSERT(shutdown_);
2121
thread_local_data_.data_.clear();
22-
Thread::MainThread::clear();
2322
}
2423

2524
SlotPtr InstanceImpl::allocateSlot() {

‎source/common/thread_local/thread_local_impl.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ namespace ThreadLocal {
1919
*/
2020
class InstanceImpl : Logger::Loggable<Logger::Id::main>, public NonCopyable, public Instance {
2121
public:
22-
InstanceImpl() { Thread::MainThread::initMainThread(); }
2322
~InstanceImpl() override;
2423

2524
// ThreadLocal::Instance
@@ -78,6 +77,7 @@ class InstanceImpl : Logger::Loggable<Logger::Id::main>, public NonCopyable, pub
7877

7978
static thread_local ThreadLocalData thread_local_data_;
8079

80+
Thread::MainThread main_thread_;
8181
std::vector<Slot*> slots_;
8282
// A list of index of freed slots.
8383
std::list<uint32_t> free_slot_indexes_;

‎source/exe/main_common.cc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,7 @@ int MainCommon::main(int argc, char** argv, PostServerHook hook) {
225225
// handling, such as running in a chroot jail.
226226
absl::InitializeSymbolizer(argv[0]);
227227
#endif
228+
Thread::MainThread main_thread;
228229
std::unique_ptr<Envoy::MainCommon> main_common;
229230

230231
// Initialize the server's main context under a try/catch loop and simply return EXIT_FAILURE

‎source/exe/main_common.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,8 @@ class MainCommon {
138138
static int main(int argc, char** argv, PostServerHook hook = nullptr);
139139

140140
private:
141+
Thread::MainThread main_thread_;
142+
141143
#ifdef ENVOY_HANDLE_SIGNALS
142144
Envoy::SignalAction handle_sigs_;
143145
Envoy::TerminateHandler log_on_terminate_;

‎source/extensions/grpc_credentials/file_based_metadata/config.cc‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,11 +69,12 @@ FileBasedMetadataAuthenticator::GetMetadata(grpc::string_ref, grpc::string_ref,
6969
if (!config_.header_key().empty()) {
7070
header_key = config_.header_key();
7171
}
72-
TRY_ASSERT_MAIN_THREAD {
72+
// TODO(#14320): avoid using an exception here or find some way of doing this
73+
// in the main thread.
74+
TRY_NEEDS_AUDIT {
7375
std::string header_value = Envoy::Config::DataSource::read(config_.secret_data(), true, api_);
7476
metadata->insert(std::make_pair(header_key, header_prefix + header_value));
7577
}
76-
END_TRY
7778
catch (const EnvoyException& e) {
7879
return grpc::Status(grpc::StatusCode::NOT_FOUND, e.what());
7980
}

‎test/benchmark/main.cc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ int main(int argc, char** argv) {
4343
}
4444

4545
TestEnvironment::initializeTestMain(argv[0]);
46+
Thread::TestThread test_thread;
4647

4748
// Suppressing non-error messages in benchmark tests. This hides warning
4849
// messages that appear when using a runtime feature when there isn't an initialized

‎test/fuzz/main.cc‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
#include "source/common/common/assert.h"
1616
#include "source/common/common/logger.h"
17+
#include "source/common/common/thread.h"
1718

1819
#include "test/fuzz/fuzz_runner.h"
1920
#include "test/test_common/environment.h"
@@ -55,6 +56,7 @@ INSTANTIATE_TEST_SUITE_P(CorpusExamples, FuzzerCorpusTest, testing::ValuesIn(tes
5556

5657
int main(int argc, char** argv) {
5758
Envoy::TestEnvironment::initializeTestMain(argv[0]);
59+
Envoy::Thread::TestThread test_thread;
5860

5961
// Expected usage: <test path> <corpus paths..> [other gtest flags]
6062
RELEASE_ASSERT(argc >= 2, "");

0 commit comments

Comments
 (0)