From 90f72a6c7e7be3809d4bcb39d4f78ceb432f1eb9 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 4 Jun 2020 10:54:28 -0400 Subject: [PATCH 01/13] add new optional Thread::Options argument to thread creation, which initially will allow specification of a name. Signed-off-by: Joshua Marantz --- include/envoy/thread/thread.h | 19 ++++- .../access_log/access_log_manager_impl.cc | 3 +- source/common/common/posix/thread_impl.cc | 77 +++++++++++++++---- source/common/common/posix/thread_impl.h | 18 +---- source/common/common/win32/thread_impl.cc | 61 +++++++++------ source/common/common/win32/thread_impl.h | 22 +----- .../common/filesystem/win32/watcher_impl.cc | 3 +- .../common/grpc/google_async_client_impl.cc | 3 +- source/server/worker_impl.cc | 5 +- test/common/common/thread_test.cc | 52 +++++++++---- 10 files changed, 166 insertions(+), 97 deletions(-) diff --git a/include/envoy/thread/thread.h b/include/envoy/thread/thread.h index 70452ca5d29ab..a4c1867a642d6 100644 --- a/include/envoy/thread/thread.h +++ b/include/envoy/thread/thread.h @@ -9,6 +9,9 @@ #include "common/common/thread_annotations.h" +#include "absl/strings/string_view.h" +#include "absl/types/optional.h" + namespace Envoy { namespace Thread { @@ -36,6 +39,11 @@ class Thread { public: virtual ~Thread() = default; + /** + * Returns the name of the thread. + */ + virtual std::string name() const PURE; + /** * Join on thread exit. */ @@ -44,6 +52,13 @@ class Thread { using ThreadPtr = std::unique_ptr; +// Options specified during thread creation. +struct Options { + std::string name_; +}; + +using OptionsOptConstRef = const absl::optional&; + /** * Interface providing a mechanism for creating threads. */ @@ -54,8 +69,10 @@ class ThreadFactory { /** * Create a thread. * @param thread_routine supplies the function to invoke in the thread. + * @param name supplies a name for the thread. May be truncated per platform limits. */ - virtual ThreadPtr createThread(std::function thread_routine) PURE; + virtual ThreadPtr createThread(std::function thread_routine, + OptionsOptConstRef options = absl::nullopt) PURE; /** * Return the current system thread ID diff --git a/source/common/access_log/access_log_manager_impl.cc b/source/common/access_log/access_log_manager_impl.cc index f173904d2dffb..055e602bdcfb5 100644 --- a/source/common/access_log/access_log_manager_impl.cc +++ b/source/common/access_log/access_log_manager_impl.cc @@ -203,7 +203,8 @@ void AccessLogFileImpl::write(absl::string_view data) { } void AccessLogFileImpl::createFlushStructures() { - flush_thread_ = thread_factory_.createThread([this]() -> void { flushThreadFunc(); }); + flush_thread_ = thread_factory_.createThread([this]() -> void { flushThreadFunc(); }, + Thread::Options{"AccessLogFlush"}); flush_timer_->enableTimer(flush_interval_msec_); } diff --git a/source/common/common/posix/thread_impl.cc b/source/common/common/posix/thread_impl.cc index 324230ade176b..9298057c62d60 100644 --- a/source/common/common/posix/thread_impl.cc +++ b/source/common/common/posix/thread_impl.cc @@ -1,6 +1,9 @@ #include "common/common/assert.h" #include "common/common/thread_impl.h" +#include "absl/strings/str_cat.h" +#include "absl/synchronization/notification.h" + #if defined(__linux__) #include #endif @@ -24,26 +27,72 @@ int64_t getCurrentThreadId() { } // namespace -ThreadImplPosix::ThreadImplPosix(std::function thread_routine) - : thread_routine_(std::move(thread_routine)) { - RELEASE_ASSERT(Logger::Registry::initialized(), ""); - const int rc = pthread_create( - &thread_handle_, nullptr, - [](void* arg) -> void* { - static_cast(arg)->thread_routine_(); - return nullptr; - }, - this); - RELEASE_ASSERT(rc == 0, ""); -} +// See https://www.man7.org/linux/man-pages/man3/pthread_setname_np.3.html. +// The maximum thread name is 16 bytes including the terminating nul byte, +// so we need to truncate the string_view to 15 bytes. +#define PTHREAD_MAX_LEN_INCLUDING_NULL_BYTE 16 + +/** + * Wrapper for a pthread thread. We don't use std::thread because it eats exceptions and leads to + * unusable stack traces. + */ +class ThreadImplPosix : public Thread { +public: + ThreadImplPosix(std::function thread_routine, OptionsOptConstRef options) + : thread_routine_(std::move(thread_routine)) { + if (options) { + name_ = (std::string(options->name_.substr(0, PTHREAD_MAX_LEN_INCLUDING_NULL_BYTE - 1))); + } + RELEASE_ASSERT(Logger::Registry::initialized(), ""); + const int rc = pthread_create( + &thread_handle_, nullptr, + [](void* arg) -> void* { + auto* thread = static_cast(arg); + + // Block at thread start waiting for setup to be complete in the initiating thread. + // For example, we want to set the debug name of the thread. + thread->start_.WaitForNotification(); + + thread->thread_routine_(); + return nullptr; + }, + this); + RELEASE_ASSERT(rc == 0, ""); + if (!name_.empty()) { + const int set_name_rc = pthread_setname_np(thread_handle_, name_.c_str()); + RELEASE_ASSERT(set_name_rc == 0, absl::StrCat("Error ", set_name_rc, " setting name '", name_, + "': ", strerror(set_name_rc))); +#ifndef NDEBUG + // Verify that the name got written into the thread as expected. + char buf[PTHREAD_MAX_LEN_INCLUDING_NULL_BYTE]; + const int get_name_rc = pthread_getname_np(thread_handle_, buf, sizeof(buf)); + RELEASE_ASSERT(get_name_rc == 0, absl::StrCat("Error ", get_name_rc, " setting name '", name_, + "': ", strerror(get_name_rc))); +#endif + } + start_.Notify(); + } + + std::string name() const override { return name_; } + + // Thread::Thread + void join() override; + +private: + std::function thread_routine_; + pthread_t thread_handle_; + std::string name_; + absl::Notification start_; +}; void ThreadImplPosix::join() { const int rc = pthread_join(thread_handle_, nullptr); RELEASE_ASSERT(rc == 0, ""); } -ThreadPtr ThreadFactoryImplPosix::createThread(std::function thread_routine) { - return std::make_unique(thread_routine); +ThreadPtr ThreadFactoryImplPosix::createThread(std::function thread_routine, + OptionsOptConstRef options) { + return std::make_unique(thread_routine, options); } ThreadId ThreadFactoryImplPosix::currentThreadId() { return ThreadId(getCurrentThreadId()); } diff --git a/source/common/common/posix/thread_impl.h b/source/common/common/posix/thread_impl.h index 81c81d3be3fc5..9b373ecaceb60 100644 --- a/source/common/common/posix/thread_impl.h +++ b/source/common/common/posix/thread_impl.h @@ -9,29 +9,13 @@ namespace Envoy { namespace Thread { -/** - * Wrapper for a pthread thread. We don't use std::thread because it eats exceptions and leads to - * unusable stack traces. - */ -class ThreadImplPosix : public Thread { -public: - ThreadImplPosix(std::function thread_routine); - - // Thread::Thread - void join() override; - -private: - std::function thread_routine_; - pthread_t thread_handle_; -}; - /** * Implementation of ThreadFactory */ class ThreadFactoryImplPosix : public ThreadFactory { public: // Thread::ThreadFactory - ThreadPtr createThread(std::function thread_routine) override; + ThreadPtr createThread(std::function thread_routine, OptionsOptConstRef options) override; ThreadId currentThreadId() override; }; diff --git a/source/common/common/win32/thread_impl.cc b/source/common/common/win32/thread_impl.cc index 1d3eca9689570..36c19b858ea85 100644 --- a/source/common/common/win32/thread_impl.cc +++ b/source/common/common/win32/thread_impl.cc @@ -6,28 +6,45 @@ namespace Envoy { namespace Thread { -ThreadImplWin32::ThreadImplWin32(std::function thread_routine) - : thread_routine_(thread_routine) { - RELEASE_ASSERT(Logger::Registry::initialized(), ""); - thread_handle_ = reinterpret_cast(::_beginthreadex( - nullptr, 0, - [](void* arg) -> unsigned int { - static_cast(arg)->thread_routine_(); - return 0; - }, - this, 0, nullptr)); - RELEASE_ASSERT(thread_handle_ != 0, ""); -} - -ThreadImplWin32::~ThreadImplWin32() { ::CloseHandle(thread_handle_); } - -void ThreadImplWin32::join() { - const DWORD rc = ::WaitForSingleObject(thread_handle_, INFINITE); - RELEASE_ASSERT(rc == WAIT_OBJECT_0, ""); -} - -ThreadPtr ThreadFactoryImplWin32::createThread(std::function thread_routine) { - return std::make_unique(thread_routine); +/** + * Wrapper for a win32 thread. We don't use std::thread because it eats exceptions and leads to + * unusable stack traces. + */ +class ThreadImplWin32 : public Thread { +public: + ThreadImplWin32(std::function thread_routine, OptionsOptConstRef options) + : thread_routine_(thread_routine) { + UNREFERENCED_PARAMETER(options); // TODO(jmarantz): set the thread name for task manager, etc. + RELEASE_ASSERT(Logger::Registry::initialized(), ""); + thread_handle_ = reinterpret_cast(::_beginthreadex( + nullptr, 0, + [](void* arg) -> unsigned int { + static_cast(arg)->thread_routine_(); + return 0; + }, + this, 0, nullptr)); + RELEASE_ASSERT(thread_handle_ != 0, ""); + } + + ~ThreadImplWin32() { ::CloseHandle(thread_handle_); } + + // Thread::Thread + void join() override { + const DWORD rc = ::WaitForSingleObject(thread_handle_, INFINITE); + RELEASE_ASSERT(rc == WAIT_OBJECT_0, ""); + } + + // Needed for WatcherImpl for the QueueUserAPC callback context + HANDLE handle() const { return thread_handle_; } + +private: + std::function thread_routine_; + HANDLE thread_handle_; +}; + +ThreadPtr ThreadFactoryImplWin32::createThread(std::function thread_routine, + OptionsOptConstRef options) { + return std::make_unique(thread_routine, name); } ThreadId ThreadFactoryImplWin32::currentThreadId() { diff --git a/source/common/common/win32/thread_impl.h b/source/common/common/win32/thread_impl.h index 8b5d0fe37e15b..b2222151a7802 100644 --- a/source/common/common/win32/thread_impl.h +++ b/source/common/common/win32/thread_impl.h @@ -8,33 +8,13 @@ namespace Envoy { namespace Thread { -/** - * Wrapper for a win32 thread. We don't use std::thread because it eats exceptions and leads to - * unusable stack traces. - */ -class ThreadImplWin32 : public Thread { -public: - ThreadImplWin32(std::function thread_routine); - ~ThreadImplWin32(); - - // Thread::Thread - void join() override; - - // Needed for WatcherImpl for the QueueUserAPC callback context - HANDLE handle() const { return thread_handle_; } - -private: - std::function thread_routine_; - HANDLE thread_handle_; -}; - /** * Implementation of ThreadFactory */ class ThreadFactoryImplWin32 : public ThreadFactory { public: // Thread::ThreadFactory - ThreadPtr createThread(std::function thread_routine) override; + ThreadPtr createThread(std::function thread_routine, OptionsOptConstRef options) override; ThreadId currentThreadId() override; }; diff --git a/source/common/filesystem/win32/watcher_impl.cc b/source/common/filesystem/win32/watcher_impl.cc index 5bc400639109f..6644211b4eb38 100644 --- a/source/common/filesystem/win32/watcher_impl.cc +++ b/source/common/filesystem/win32/watcher_impl.cc @@ -31,7 +31,8 @@ WatcherImpl::WatcherImpl(Event::Dispatcher& dispatcher, Api::Api& api) thread_exit_event_ = ::CreateEvent(nullptr, false, false, nullptr); ASSERT(thread_exit_event_ != NULL); keep_watching_ = true; - watch_thread_ = thread_factory_.createThread([this]() -> void { watchLoop(); }); + Thread::Options options{absl::StrCat("watch:", dispatcher_->name())}; + watch_thread_ = thread_factory_.createThread([this]() -> void { watchLoop(); }, options); } WatcherImpl::~WatcherImpl() { diff --git a/source/common/grpc/google_async_client_impl.cc b/source/common/grpc/google_async_client_impl.cc index 18c4936e5e25b..e4bc32c72b8f9 100644 --- a/source/common/grpc/google_async_client_impl.cc +++ b/source/common/grpc/google_async_client_impl.cc @@ -21,7 +21,8 @@ static constexpr int DefaultBufferLimitBytes = 1024 * 1024; } GoogleAsyncClientThreadLocal::GoogleAsyncClientThreadLocal(Api::Api& api) - : completion_thread_(api.threadFactory().createThread([this] { completionThread(); })) {} + : completion_thread_(api.threadFactory().createThread([this] { completionThread(); }, + Thread::Options{"GoogleAsyncClient"})) {} GoogleAsyncClientThreadLocal::~GoogleAsyncClientThreadLocal() { // Force streams to shutdown and invoke TryCancel() to start the drain of diff --git a/source/server/worker_impl.cc b/source/server/worker_impl.cc index 17e02486e5f80..630600a720a77 100644 --- a/source/server/worker_impl.cc +++ b/source/server/worker_impl.cc @@ -81,8 +81,9 @@ void WorkerImpl::removeFilterChains(uint64_t listener_tag, void WorkerImpl::start(GuardDog& guard_dog) { ASSERT(!thread_); - thread_ = - api_.threadFactory().createThread([this, &guard_dog]() -> void { threadRoutine(guard_dog); }); + Thread::Options options{absl::StrCat("worker:", dispatcher_->name())}; + thread_ = api_.threadFactory().createThread( + [this, &guard_dog]() -> void { threadRoutine(guard_dog); }, options); } void WorkerImpl::initializeStats(Stats::Scope& scope) { dispatcher_->initializeStats(scope); } diff --git a/test/common/common/thread_test.cc b/test/common/common/thread_test.cc index 431d4be38f325..f8c2f25a7e9fb 100644 --- a/test/common/common/thread_test.cc +++ b/test/common/common/thread_test.cc @@ -5,6 +5,7 @@ #include "test/test_common/thread_factory_for_test.h" +#include "absl/strings/str_cat.h" #include "absl/synchronization/notification.h" #include "gtest/gtest.h" @@ -27,12 +28,15 @@ TEST_F(ThreadAsyncPtrTest, DeleteOnDestruct) { // On thread1, we will lazily instantiate the string as "thread1". However // in the creation function we will block on a sync-point. - auto thread1 = thread_factory_.createThread([&str, &sync]() { - str.get([&sync]() -> std::string* { - sync.syncPoint("creator"); - return new std::string("thread1"); - }); - }); + auto thread1 = thread_factory_.createThread( + [&str, &sync]() { + str.get([&sync]() -> std::string* { + sync.syncPoint("creator"); + return new std::string("thread1"); + }); + }, + Options{"thread1"}); + EXPECT_EQ("thread1", thread1->name()); sync.barrierOn("creator"); @@ -40,7 +44,9 @@ TEST_F(ThreadAsyncPtrTest, DeleteOnDestruct) { // string as "thread2", but that allocator will never run because // the allocator on thread1 has already locked the AtomicPtr's mutex. auto thread2 = thread_factory_.createThread( - [&str]() { str.get([]() -> std::string* { return new std::string("thread2"); }); }); + [&str]() { str.get([]() -> std::string* { return new std::string("thread2"); }); }, + Options{"thread2"}); + EXPECT_EQ("thread2", thread2->name()); // Now let thread1's initializer finish. sync.signal("creator"); @@ -68,21 +74,25 @@ TEST_F(ThreadAsyncPtrTest, DoNotDelete) { // On thread1, we will lazily instantiate the string as "thread1". However // in the creation function we will block on a sync-point. - auto thread1 = thread_factory_.createThread([&str, &sync, &thread1_str]() { - str.get([&sync, &thread1_str]() -> const std::string* { - sync.syncPoint("creator"); - return &thread1_str; - }); - }); + auto thread1 = thread_factory_.createThread( + [&str, &sync, &thread1_str]() { + str.get([&sync, &thread1_str]() -> const std::string* { + sync.syncPoint("creator"); + return &thread1_str; + }); + }, + Options{"thread1"}); sync.barrierOn("creator"); // Now spawn a separate thread that will attempt to lazy-initialize the // string as "thread2", but that allocator will never run because // the allocator on thread1 has already locked the AtomicPtr's mutex. - auto thread2 = thread_factory_.createThread([&str, &thread2_str]() { - str.get([&thread2_str]() -> const std::string* { return &thread2_str; }); - }); + auto thread2 = thread_factory_.createThread( + [&str, &thread2_str]() { + str.get([&thread2_str]() -> const std::string* { return &thread2_str; }); + }, + Options{"thread2"}); // Now let thread1's initializer finish. sync.signal("creator"); @@ -113,7 +123,9 @@ TEST_F(ThreadAsyncPtrTest, ThreadSpammer) { }; std::vector threads; for (uint32_t i = 0; i < num_threads; ++i) { - threads.emplace_back(thread_factory_.createThread(thread_fn)); + std::string name = absl::StrCat("thread", i); + threads.emplace_back(thread_factory_.createThread(thread_fn, Options{name})); + EXPECT_EQ(name, threads.back()->name()); } EXPECT_EQ(0, calls); go.Notify(); @@ -190,6 +202,12 @@ TEST_F(ThreadAsyncPtrTest, ManagedAlloc) { } } +TEST_F(ThreadAsyncPtrTest, Truncate) { + auto thread = + thread_factory_.createThread([]() {}, Options{"this name is way too long for posix"}); + EXPECT_EQ("this name is wa", thread->name()); +} + } // namespace } // namespace Thread } // namespace Envoy From ff1e9974526628c1e0cffd4781ad610a3ed9dacb Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 4 Jun 2020 11:36:48 -0400 Subject: [PATCH 02/13] fix typo in windows impl which broke compiles. Signed-off-by: Joshua Marantz --- source/common/common/win32/thread_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/common/win32/thread_impl.cc b/source/common/common/win32/thread_impl.cc index 36c19b858ea85..87dfe8d4247a7 100644 --- a/source/common/common/win32/thread_impl.cc +++ b/source/common/common/win32/thread_impl.cc @@ -44,7 +44,7 @@ class ThreadImplWin32 : public Thread { ThreadPtr ThreadFactoryImplWin32::createThread(std::function thread_routine, OptionsOptConstRef options) { - return std::make_unique(thread_routine, name); + return std::make_unique(thread_routine, options); } ThreadId ThreadFactoryImplWin32::currentThreadId() { From 555d26a490ba2634b7c6a6c00e55b80b10148e13 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 4 Jun 2020 12:18:36 -0400 Subject: [PATCH 03/13] make tests more likely to work on different platforms (hail mary for windows) and cover the case where the options are not provided. Signed-off-by: Joshua Marantz --- source/common/common/posix/thread_impl.cc | 27 ++++++++++++++++------- source/common/common/win32/thread_impl.cc | 10 ++++++++- test/common/common/thread_test.cc | 15 ++++++++++++- 3 files changed, 42 insertions(+), 10 deletions(-) diff --git a/source/common/common/posix/thread_impl.cc b/source/common/common/posix/thread_impl.cc index 9298057c62d60..de57790792350 100644 --- a/source/common/common/posix/thread_impl.cc +++ b/source/common/common/posix/thread_impl.cc @@ -58,18 +58,20 @@ class ThreadImplPosix : public Thread { }, this); RELEASE_ASSERT(rc == 0, ""); - if (!name_.empty()) { + + // If the name was not specified, get it from the OS. If the name was + // specified, write it into the thread, and assert that the OS sees it the + // same way. + if (name_.empty()) { + name_ = getNameFromOS(); + } else { const int set_name_rc = pthread_setname_np(thread_handle_, name_.c_str()); RELEASE_ASSERT(set_name_rc == 0, absl::StrCat("Error ", set_name_rc, " setting name '", name_, "': ", strerror(set_name_rc))); -#ifndef NDEBUG - // Verify that the name got written into the thread as expected. - char buf[PTHREAD_MAX_LEN_INCLUDING_NULL_BYTE]; - const int get_name_rc = pthread_getname_np(thread_handle_, buf, sizeof(buf)); - RELEASE_ASSERT(get_name_rc == 0, absl::StrCat("Error ", get_name_rc, " setting name '", name_, - "': ", strerror(get_name_rc))); -#endif + ASSERT(name_ == getNameFromOS(), + absl::StrCat("configured name=", name_, " os name=", getNameFromOS())); } + start_.Notify(); } @@ -79,6 +81,15 @@ class ThreadImplPosix : public Thread { void join() override; private: + std::string getNameFromOS() { + // Verify that the name got written into the thread as expected. + char buf[PTHREAD_MAX_LEN_INCLUDING_NULL_BYTE]; + const int get_name_rc = pthread_getname_np(thread_handle_, buf, sizeof(buf)); + RELEASE_ASSERT(get_name_rc == 0, absl::StrCat("Error ", get_name_rc, " setting name '", name_, + "': ", strerror(get_name_rc))); + return buf; + } + std::function thread_routine_; pthread_t thread_handle_; std::string name_; diff --git a/source/common/common/win32/thread_impl.cc b/source/common/common/win32/thread_impl.cc index 87dfe8d4247a7..fac57796e6840 100644 --- a/source/common/common/win32/thread_impl.cc +++ b/source/common/common/win32/thread_impl.cc @@ -14,7 +14,12 @@ class ThreadImplWin32 : public Thread { public: ThreadImplWin32(std::function thread_routine, OptionsOptConstRef options) : thread_routine_(thread_routine) { - UNREFERENCED_PARAMETER(options); // TODO(jmarantz): set the thread name for task manager, etc. + if (options) { + name_ = options->name_; + // TODO(jmarantz): set the thread name for task manager, etc, or pull the + // auto-generated name from the OS if options is not present. + } + RELEASE_ASSERT(Logger::Registry::initialized(), ""); thread_handle_ = reinterpret_cast(::_beginthreadex( nullptr, 0, @@ -37,9 +42,12 @@ class ThreadImplWin32 : public Thread { // Needed for WatcherImpl for the QueueUserAPC callback context HANDLE handle() const { return thread_handle_; } + sted::string name() const override { return name_; } + private: std::function thread_routine_; HANDLE thread_handle_; + std::string name_; }; ThreadPtr ThreadFactoryImplWin32::createThread(std::function thread_routine, diff --git a/test/common/common/thread_test.cc b/test/common/common/thread_test.cc index f8c2f25a7e9fb..aa18ac434d76f 100644 --- a/test/common/common/thread_test.cc +++ b/test/common/common/thread_test.cc @@ -7,6 +7,7 @@ #include "absl/strings/str_cat.h" #include "absl/synchronization/notification.h" +#include "gmock/gmock.h" #include "gtest/gtest.h" namespace Envoy { @@ -205,7 +206,19 @@ TEST_F(ThreadAsyncPtrTest, ManagedAlloc) { TEST_F(ThreadAsyncPtrTest, Truncate) { auto thread = thread_factory_.createThread([]() {}, Options{"this name is way too long for posix"}); - EXPECT_EQ("this name is wa", thread->name()); + + // To make this test work on multiple platforms, just assume the first 10 characters + // are retained. + EXPECT_THAT(thread->name(), testing::StartsWith("this name ")); +} + +TEST_F(ThreadAsyncPtrTest, NameNotSpecified) { + auto thread = thread_factory_.createThread([]() {}); + + // For posix builds, the thread name defaults to the name of the binary. + // Not sure about Windows. Note that for coverage, the tests are all linked + // together, and for Windows tests I'm not sure what the behavior is. + EXPECT_FALSE(thread->name().empty()); } } // namespace From 737d684fd3449d916b24f84bda9ff5386c4170ea Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 4 Jun 2020 13:08:07 -0400 Subject: [PATCH 04/13] fix typo Signed-off-by: Joshua Marantz --- source/common/common/win32/thread_impl.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/common/win32/thread_impl.cc b/source/common/common/win32/thread_impl.cc index fac57796e6840..eb02f127e69e5 100644 --- a/source/common/common/win32/thread_impl.cc +++ b/source/common/common/win32/thread_impl.cc @@ -39,11 +39,11 @@ class ThreadImplWin32 : public Thread { RELEASE_ASSERT(rc == WAIT_OBJECT_0, ""); } + std::string name() const override { return name_; } + // Needed for WatcherImpl for the QueueUserAPC callback context HANDLE handle() const { return thread_handle_; } - sted::string name() const override { return name_; } - private: std::function thread_routine_; HANDLE thread_handle_; From c5732ad7729a129875291b1789d8385b6128230f Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 4 Jun 2020 14:09:24 -0400 Subject: [PATCH 05/13] remove new blocking notification from prod, and instead be resilient to failing to set the thread name. Signed-off-by: Joshua Marantz --- source/common/common/posix/thread_impl.cc | 41 ++++++++++--------- .../common/filesystem/win32/watcher_impl.cc | 4 +- source/server/guarddog_impl.cc | 4 +- source/server/worker_impl.cc | 13 +++++- test/common/common/thread_test.cc | 21 ++++++++-- 5 files changed, 57 insertions(+), 26 deletions(-) diff --git a/source/common/common/posix/thread_impl.cc b/source/common/common/posix/thread_impl.cc index de57790792350..1e39e99e0039d 100644 --- a/source/common/common/posix/thread_impl.cc +++ b/source/common/common/posix/thread_impl.cc @@ -2,7 +2,6 @@ #include "common/common/thread_impl.h" #include "absl/strings/str_cat.h" -#include "absl/synchronization/notification.h" #if defined(__linux__) #include @@ -47,13 +46,7 @@ class ThreadImplPosix : public Thread { const int rc = pthread_create( &thread_handle_, nullptr, [](void* arg) -> void* { - auto* thread = static_cast(arg); - - // Block at thread start waiting for setup to be complete in the initiating thread. - // For example, we want to set the debug name of the thread. - thread->start_.WaitForNotification(); - - thread->thread_routine_(); + static_cast(arg)->thread_routine_(); return nullptr; }, this); @@ -63,16 +56,22 @@ class ThreadImplPosix : public Thread { // specified, write it into the thread, and assert that the OS sees it the // same way. if (name_.empty()) { - name_ = getNameFromOS(); + getNameFromOS(name_); } else { const int set_name_rc = pthread_setname_np(thread_handle_, name_.c_str()); - RELEASE_ASSERT(set_name_rc == 0, absl::StrCat("Error ", set_name_rc, " setting name '", name_, - "': ", strerror(set_name_rc))); - ASSERT(name_ == getNameFromOS(), - absl::StrCat("configured name=", name_, " os name=", getNameFromOS())); + if (set_name_rc != 0) { + ENVOY_LOG_MISC(trace, "Error {} setting name `{}': {}", set_name_rc, name_, + strerror(set_name_rc)); + } else { +#ifndef NDEBUG + std::string check_name; + if (getNameFromOS(check_name)) { + ASSERT(check_name == name_, + absl::StrCat("configured name=", name_, " os name=", check_name)); + } +#endif + } } - - start_.Notify(); } std::string name() const override { return name_; } @@ -81,19 +80,21 @@ class ThreadImplPosix : public Thread { void join() override; private: - std::string getNameFromOS() { + bool getNameFromOS(std::string& name) { // Verify that the name got written into the thread as expected. char buf[PTHREAD_MAX_LEN_INCLUDING_NULL_BYTE]; const int get_name_rc = pthread_getname_np(thread_handle_, buf, sizeof(buf)); - RELEASE_ASSERT(get_name_rc == 0, absl::StrCat("Error ", get_name_rc, " setting name '", name_, - "': ", strerror(get_name_rc))); - return buf; + if (get_name_rc != 0) { + ENVOY_LOG_MISC(trace, "Error {} getting name: {}", get_name_rc, strerror(get_name_rc)); + return false; + } + name = buf; + return true; } std::function thread_routine_; pthread_t thread_handle_; std::string name_; - absl::Notification start_; }; void ThreadImplPosix::join() { diff --git a/source/common/filesystem/win32/watcher_impl.cc b/source/common/filesystem/win32/watcher_impl.cc index 6644211b4eb38..c1677cf5bfe45 100644 --- a/source/common/filesystem/win32/watcher_impl.cc +++ b/source/common/filesystem/win32/watcher_impl.cc @@ -31,7 +31,9 @@ WatcherImpl::WatcherImpl(Event::Dispatcher& dispatcher, Api::Api& api) thread_exit_event_ = ::CreateEvent(nullptr, false, false, nullptr); ASSERT(thread_exit_event_ != NULL); keep_watching_ = true; - Thread::Options options{absl::StrCat("watch:", dispatcher_->name())}; + + // See comments in WorkerImpl::start for the naming convention. + Thread::Options options{absl::StrCat("wat:", dispatcher_->name())}; watch_thread_ = thread_factory_.createThread([this]() -> void { watchLoop(); }, options); } diff --git a/source/server/guarddog_impl.cc b/source/server/guarddog_impl.cc index d05e84f6ff6e8..ad66b59a5299b 100644 --- a/source/server/guarddog_impl.cc +++ b/source/server/guarddog_impl.cc @@ -142,8 +142,10 @@ void GuardDogImpl::stopWatching(WatchDogSharedPtr wd) { void GuardDogImpl::start(Api::Api& api) { Thread::LockGuard guard(mutex_); + // See comments in WorkerImpl::start for the naming convention. + Thread::Options options{absl::StrCat("dog:", dispatcher_->name())}; thread_ = api.threadFactory().createThread( - [this]() -> void { dispatcher_->run(Event::Dispatcher::RunType::RunUntilExit); }); + [this]() -> void { dispatcher_->run(Event::Dispatcher::RunType::RunUntilExit); }, options); loop_timer_->enableTimer(std::chrono::milliseconds(0)); } diff --git a/source/server/worker_impl.cc b/source/server/worker_impl.cc index 630600a720a77..b116721329e94 100644 --- a/source/server/worker_impl.cc +++ b/source/server/worker_impl.cc @@ -81,7 +81,18 @@ void WorkerImpl::removeFilterChains(uint64_t listener_tag, void WorkerImpl::start(GuardDog& guard_dog) { ASSERT(!thread_); - Thread::Options options{absl::StrCat("worker:", dispatcher_->name())}; + + // In posix, thread names are limited to 15 characters, so contrive to make + // sure all interesting data fits there. The naming occurs in + // ListenerManagerImpl's constructor: absl::StrCat("worker_", i). Let's say we + // have 9999 threads. We'd need, so we need 7 bytes for "worker_", 4 bytes + // for the thread index, leaving us 4 bytes left to distinguish between the + // two threads used per dispatcher. We'll call this one "dsp:" and the + // one allocated in guarddog_impl.cc "dog:". + // + // TODO(jmarantz): consider refactoring how this naming works so this naming + // architecture is centralized, resulting in clearer names. + Thread::Options options{absl::StrCat("dsp:", dispatcher_->name())}; thread_ = api_.threadFactory().createThread( [this, &guard_dog]() -> void { threadRoutine(guard_dog); }, options); } diff --git a/test/common/common/thread_test.cc b/test/common/common/thread_test.cc index aa18ac434d76f..082612b619b53 100644 --- a/test/common/common/thread_test.cc +++ b/test/common/common/thread_test.cc @@ -203,7 +203,19 @@ TEST_F(ThreadAsyncPtrTest, ManagedAlloc) { } } -TEST_F(ThreadAsyncPtrTest, Truncate) { +TEST_F(ThreadAsyncPtrTest, TruncateWait) { + absl::Notification notify; + auto thread = thread_factory_.createThread([¬ify]() { notify.WaitForNotification(); }, + Options{"this name is way too long for posix"}); + notify.Notify(); + + // To make this test work on multiple platforms, just assume the first 10 characters + // are retained. + EXPECT_THAT(thread->name(), testing::StartsWith("this name ")); + thread->join(); +} + +TEST_F(ThreadAsyncPtrTest, TruncateNoWait) { auto thread = thread_factory_.createThread([]() {}, Options{"this name is way too long for posix"}); @@ -212,13 +224,16 @@ TEST_F(ThreadAsyncPtrTest, Truncate) { EXPECT_THAT(thread->name(), testing::StartsWith("this name ")); } -TEST_F(ThreadAsyncPtrTest, NameNotSpecified) { - auto thread = thread_factory_.createThread([]() {}); +TEST_F(ThreadAsyncPtrTest, NameNotSpecifiedWait) { + absl::Notification notify; + auto thread = thread_factory_.createThread([¬ify]() { notify.WaitForNotification(); }); + notify.Notify(); // For posix builds, the thread name defaults to the name of the binary. // Not sure about Windows. Note that for coverage, the tests are all linked // together, and for Windows tests I'm not sure what the behavior is. EXPECT_FALSE(thread->name().empty()); + thread->join(); } } // namespace From 7f36e99fbbcd74c0295abd49b6a6371c2185bec0 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 4 Jun 2020 15:14:28 -0400 Subject: [PATCH 06/13] re-expose the win32 thread class. Signed-off-by: Joshua Marantz --- source/common/common/win32/thread_impl.cc | 60 ++++++++--------------- source/common/common/win32/thread_impl.h | 22 +++++++++ 2 files changed, 43 insertions(+), 39 deletions(-) diff --git a/source/common/common/win32/thread_impl.cc b/source/common/common/win32/thread_impl.cc index eb02f127e69e5..8f26d63e0eb39 100644 --- a/source/common/common/win32/thread_impl.cc +++ b/source/common/common/win32/thread_impl.cc @@ -6,49 +6,31 @@ namespace Envoy { namespace Thread { -/** - * Wrapper for a win32 thread. We don't use std::thread because it eats exceptions and leads to - * unusable stack traces. - */ -class ThreadImplWin32 : public Thread { -public: - ThreadImplWin32(std::function thread_routine, OptionsOptConstRef options) - : thread_routine_(thread_routine) { - if (options) { - name_ = options->name_; - // TODO(jmarantz): set the thread name for task manager, etc, or pull the - // auto-generated name from the OS if options is not present. - } - - RELEASE_ASSERT(Logger::Registry::initialized(), ""); - thread_handle_ = reinterpret_cast(::_beginthreadex( - nullptr, 0, - [](void* arg) -> unsigned int { - static_cast(arg)->thread_routine_(); - return 0; - }, - this, 0, nullptr)); - RELEASE_ASSERT(thread_handle_ != 0, ""); - } - - ~ThreadImplWin32() { ::CloseHandle(thread_handle_); } - - // Thread::Thread - void join() override { - const DWORD rc = ::WaitForSingleObject(thread_handle_, INFINITE); - RELEASE_ASSERT(rc == WAIT_OBJECT_0, ""); +ThreadImplWin32::ThreadImplWin32(std::function thread_routine, OptionsOptConstRef options) + : thread_routine_(thread_routine) { + if (options) { + name_ = options->name_; + // TODO(jmarantz): set the thread name for task manager, etc, or pull the + // auto-generated name from the OS if options is not present. } - std::string name() const override { return name_; } + RELEASE_ASSERT(Logger::Registry::initialized(), ""); + thread_handle_ = reinterpret_cast(::_beginthreadex( + nullptr, 0, + [](void* arg) -> unsigned int { + static_cast(arg)->thread_routine_(); + return 0; + }, + this, 0, nullptr)); + RELEASE_ASSERT(thread_handle_ != 0, ""); +} - // Needed for WatcherImpl for the QueueUserAPC callback context - HANDLE handle() const { return thread_handle_; } +ThreadImplWin32::~ThreadImplWin32() { ::CloseHandle(thread_handle_); } -private: - std::function thread_routine_; - HANDLE thread_handle_; - std::string name_; -}; +void ThreadImplWin32::join() { + const DWORD rc = ::WaitForSingleObject(thread_handle_, INFINITE); + RELEASE_ASSERT(rc == WAIT_OBJECT_0, ""); +} ThreadPtr ThreadFactoryImplWin32::createThread(std::function thread_routine, OptionsOptConstRef options) { diff --git a/source/common/common/win32/thread_impl.h b/source/common/common/win32/thread_impl.h index b2222151a7802..87be085291c86 100644 --- a/source/common/common/win32/thread_impl.h +++ b/source/common/common/win32/thread_impl.h @@ -8,6 +8,28 @@ namespace Envoy { namespace Thread { +/** + * Wrapper for a win32 thread. We don't use std::thread because it eats exceptions and leads to + * unusable stack traces. + */ +class ThreadImplWin32 : public Thread { +public: + ThreadImplWin32(std::function thread_routine, OptionsOptConstRef options); + ~ThreadImplWin32(); + + // Thread::Thread + void join() override; + std::string name() const override { return name_; } + + // Needed for WatcherImpl for the QueueUserAPC callback context + HANDLE handle() const { return thread_handle_; } + +private: + std::function thread_routine_; + HANDLE thread_handle_; + std::string name_; +}; + /** * Implementation of ThreadFactory */ From 6fa46094a4d2ac4693028c710b083e1e4b8e601a Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 4 Jun 2020 16:47:55 -0400 Subject: [PATCH 07/13] dispatcher is a local ref, not a member var ptr. Signed-off-by: Joshua Marantz --- source/common/filesystem/win32/watcher_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/filesystem/win32/watcher_impl.cc b/source/common/filesystem/win32/watcher_impl.cc index c1677cf5bfe45..80531f78d54ea 100644 --- a/source/common/filesystem/win32/watcher_impl.cc +++ b/source/common/filesystem/win32/watcher_impl.cc @@ -33,7 +33,7 @@ WatcherImpl::WatcherImpl(Event::Dispatcher& dispatcher, Api::Api& api) keep_watching_ = true; // See comments in WorkerImpl::start for the naming convention. - Thread::Options options{absl::StrCat("wat:", dispatcher_->name())}; + Thread::Options options{absl::StrCat("wat:", dispatcher.name())}; watch_thread_ = thread_factory_.createThread([this]() -> void { watchLoop(); }, options); } From 27977758b2fe678aff2d41bfd41216515d6b2bcb Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 4 Jun 2020 20:21:25 -0400 Subject: [PATCH 08/13] use ifdefs to avoid running the linux-specific code on mac. Signed-off-by: Joshua Marantz --- source/common/common/posix/thread_impl.cc | 4 ++++ test/common/common/thread_test.cc | 7 ++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/source/common/common/posix/thread_impl.cc b/source/common/common/posix/thread_impl.cc index 1e39e99e0039d..e17581318eb65 100644 --- a/source/common/common/posix/thread_impl.cc +++ b/source/common/common/posix/thread_impl.cc @@ -52,6 +52,7 @@ class ThreadImplPosix : public Thread { this); RELEASE_ASSERT(rc == 0, ""); +#ifdef __linux__ // If the name was not specified, get it from the OS. If the name was // specified, write it into the thread, and assert that the OS sees it the // same way. @@ -72,6 +73,7 @@ class ThreadImplPosix : public Thread { #endif } } +#endif } std::string name() const override { return name_; } @@ -80,6 +82,7 @@ class ThreadImplPosix : public Thread { void join() override; private: +#ifdef __linux__ bool getNameFromOS(std::string& name) { // Verify that the name got written into the thread as expected. char buf[PTHREAD_MAX_LEN_INCLUDING_NULL_BYTE]; @@ -91,6 +94,7 @@ class ThreadImplPosix : public Thread { name = buf; return true; } +#endif std::function thread_routine_; pthread_t thread_handle_; diff --git a/test/common/common/thread_test.cc b/test/common/common/thread_test.cc index 082612b619b53..3651efc28b27f 100644 --- a/test/common/common/thread_test.cc +++ b/test/common/common/thread_test.cc @@ -229,10 +229,11 @@ TEST_F(ThreadAsyncPtrTest, NameNotSpecifiedWait) { auto thread = thread_factory_.createThread([¬ify]() { notify.WaitForNotification(); }); notify.Notify(); - // For posix builds, the thread name defaults to the name of the binary. - // Not sure about Windows. Note that for coverage, the tests are all linked - // together, and for Windows tests I'm not sure what the behavior is. + // For linux builds, the thread name defaults to the name of the binary. + // Currently, this population does not occur for Mac or Windows. +#ifdef __linux__ EXPECT_FALSE(thread->name().empty()); +#endif thread->join(); } From 8e0ad3a33053ad0f231ed558c82cf96d60e64144 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 5 Jun 2020 08:52:49 -0400 Subject: [PATCH 09/13] doxygen fixes and fix test tsan error. Signed-off-by: Joshua Marantz --- include/envoy/thread/thread.h | 11 ++++++----- test/common/common/thread_test.cc | 1 + 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/include/envoy/thread/thread.h b/include/envoy/thread/thread.h index a4c1867a642d6..8633c03e1ebed 100644 --- a/include/envoy/thread/thread.h +++ b/include/envoy/thread/thread.h @@ -40,12 +40,12 @@ class Thread { virtual ~Thread() = default; /** - * Returns the name of the thread. + * @return the name of the thread. */ virtual std::string name() const PURE; /** - * Join on thread exit. + * Blocks until the thread exits. */ virtual void join() PURE; }; @@ -54,7 +54,7 @@ using ThreadPtr = std::unique_ptr; // Options specified during thread creation. struct Options { - std::string name_; + std::string name_; // A name supplied for the thread. On Linux this is limited to 15 chars. }; using OptionsOptConstRef = const absl::optional&; @@ -67,9 +67,10 @@ class ThreadFactory { virtual ~ThreadFactory() = default; /** - * Create a thread. + * Creates a thread, immediately starting the thread_routine. + * * @param thread_routine supplies the function to invoke in the thread. - * @param name supplies a name for the thread. May be truncated per platform limits. + * @param options supplies options specified on thread creation. */ virtual ThreadPtr createThread(std::function thread_routine, OptionsOptConstRef options = absl::nullopt) PURE; diff --git a/test/common/common/thread_test.cc b/test/common/common/thread_test.cc index 3651efc28b27f..cddcee25dc7b5 100644 --- a/test/common/common/thread_test.cc +++ b/test/common/common/thread_test.cc @@ -222,6 +222,7 @@ TEST_F(ThreadAsyncPtrTest, TruncateNoWait) { // To make this test work on multiple platforms, just assume the first 10 characters // are retained. EXPECT_THAT(thread->name(), testing::StartsWith("this name ")); + thread->join(); } TEST_F(ThreadAsyncPtrTest, NameNotSpecifiedWait) { From 23c59bda6680a104fdd76368f1e71ebeb0c3f7a2 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 5 Jun 2020 14:04:29 -0400 Subject: [PATCH 10/13] review comments Signed-off-by: Joshua Marantz --- .bazelversion | 2 +- source/common/common/posix/thread_impl.cc | 20 +++++++++++-------- .../common/grpc/google_async_client_impl.cc | 2 +- source/server/worker_impl.cc | 2 +- 4 files changed, 15 insertions(+), 11 deletions(-) diff --git a/.bazelversion b/.bazelversion index fd2a01863fdd3..ccbccc3dc6263 100644 --- a/.bazelversion +++ b/.bazelversion @@ -1 +1 @@ -3.1.0 +2.2.0 diff --git a/source/common/common/posix/thread_impl.cc b/source/common/common/posix/thread_impl.cc index e17581318eb65..15cedc99da0bb 100644 --- a/source/common/common/posix/thread_impl.cc +++ b/source/common/common/posix/thread_impl.cc @@ -29,7 +29,7 @@ int64_t getCurrentThreadId() { // See https://www.man7.org/linux/man-pages/man3/pthread_setname_np.3.html. // The maximum thread name is 16 bytes including the terminating nul byte, // so we need to truncate the string_view to 15 bytes. -#define PTHREAD_MAX_LEN_INCLUDING_NULL_BYTE 16 +#define PTHREAD_MAX_THREADNAME_LEN_INCLUDING_NULL_BYTE 16 /** * Wrapper for a pthread thread. We don't use std::thread because it eats exceptions and leads to @@ -40,7 +40,8 @@ class ThreadImplPosix : public Thread { ThreadImplPosix(std::function thread_routine, OptionsOptConstRef options) : thread_routine_(std::move(thread_routine)) { if (options) { - name_ = (std::string(options->name_.substr(0, PTHREAD_MAX_LEN_INCLUDING_NULL_BYTE - 1))); + name_ = (std::string( + options->name_.substr(0, PTHREAD_MAX_THREADNAME_LEN_INCLUDING_NULL_BYTE - 1))); } RELEASE_ASSERT(Logger::Registry::initialized(), ""); const int rc = pthread_create( @@ -76,10 +77,17 @@ class ThreadImplPosix : public Thread { #endif } + ~ThreadImplPosix() { ASSERT(joined_); } + std::string name() const override { return name_; } // Thread::Thread - void join() override; + void join() override { + ASSERT(!joined_); + joined_ = true; + const int rc = pthread_join(thread_handle_, nullptr); + RELEASE_ASSERT(rc == 0, ""); + } private: #ifdef __linux__ @@ -99,13 +107,9 @@ class ThreadImplPosix : public Thread { std::function thread_routine_; pthread_t thread_handle_; std::string name_; + bool joined_{false}; }; -void ThreadImplPosix::join() { - const int rc = pthread_join(thread_handle_, nullptr); - RELEASE_ASSERT(rc == 0, ""); -} - ThreadPtr ThreadFactoryImplPosix::createThread(std::function thread_routine, OptionsOptConstRef options) { return std::make_unique(thread_routine, options); diff --git a/source/common/grpc/google_async_client_impl.cc b/source/common/grpc/google_async_client_impl.cc index e4bc32c72b8f9..ff7774034ac97 100644 --- a/source/common/grpc/google_async_client_impl.cc +++ b/source/common/grpc/google_async_client_impl.cc @@ -22,7 +22,7 @@ static constexpr int DefaultBufferLimitBytes = 1024 * 1024; GoogleAsyncClientThreadLocal::GoogleAsyncClientThreadLocal(Api::Api& api) : completion_thread_(api.threadFactory().createThread([this] { completionThread(); }, - Thread::Options{"GoogleAsyncClient"})) {} + Thread::Options{"GrpcGoogClient"})) {} GoogleAsyncClientThreadLocal::~GoogleAsyncClientThreadLocal() { // Force streams to shutdown and invoke TryCancel() to start the drain of diff --git a/source/server/worker_impl.cc b/source/server/worker_impl.cc index b116721329e94..54ef058ea6e5e 100644 --- a/source/server/worker_impl.cc +++ b/source/server/worker_impl.cc @@ -92,7 +92,7 @@ void WorkerImpl::start(GuardDog& guard_dog) { // // TODO(jmarantz): consider refactoring how this naming works so this naming // architecture is centralized, resulting in clearer names. - Thread::Options options{absl::StrCat("dsp:", dispatcher_->name())}; + Thread::Options options{absl::StrCat("wrk:", dispatcher_->name())}; thread_ = api_.threadFactory().createThread( [this, &guard_dog]() -> void { threadRoutine(guard_dog); }, options); } From d251ea8b679f7801af1fb8e556048bc83c12c2b9 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 5 Jun 2020 14:18:29 -0400 Subject: [PATCH 11/13] More comments to clarify questions brought up in review. Signed-off-by: Joshua Marantz --- .bazelversion | 2 +- source/common/common/posix/thread_impl.cc | 24 +++++++++++++---------- test/common/common/thread_test.cc | 6 ++++-- 3 files changed, 19 insertions(+), 13 deletions(-) diff --git a/.bazelversion b/.bazelversion index ccbccc3dc6263..fd2a01863fdd3 100644 --- a/.bazelversion +++ b/.bazelversion @@ -1 +1 @@ -2.2.0 +3.1.0 diff --git a/source/common/common/posix/thread_impl.cc b/source/common/common/posix/thread_impl.cc index 15cedc99da0bb..408a7a67f3f90 100644 --- a/source/common/common/posix/thread_impl.cc +++ b/source/common/common/posix/thread_impl.cc @@ -62,16 +62,20 @@ class ThreadImplPosix : public Thread { } else { const int set_name_rc = pthread_setname_np(thread_handle_, name_.c_str()); if (set_name_rc != 0) { - ENVOY_LOG_MISC(trace, "Error {} setting name `{}': {}", set_name_rc, name_, - strerror(set_name_rc)); + ENVOY_LOG_MISC(trace, "Error {} setting name `{}'", set_name_rc, name_); } else { -#ifndef NDEBUG + // When compiling in debug mode, read back the thread-name from the OS, + // and verify it's what we asked for. This ensures the truncation is as + // expected, and that the OS will actually retain all the bytes of the + // name we expect. + // + // Note that the system-call to read the thread name may fail in case + // the thread exits after the call to set the name above, and before the + // call to get the name, so we can only do the assert if that call + // succeeded. std::string check_name; - if (getNameFromOS(check_name)) { - ASSERT(check_name == name_, - absl::StrCat("configured name=", name_, " os name=", check_name)); - } -#endif + ASSERT(!getNameFromOS(check_name) || check_name == name_, + absl::StrCat("configured name=", name_, " os name=", check_name)); } } #endif @@ -93,10 +97,10 @@ class ThreadImplPosix : public Thread { #ifdef __linux__ bool getNameFromOS(std::string& name) { // Verify that the name got written into the thread as expected. - char buf[PTHREAD_MAX_LEN_INCLUDING_NULL_BYTE]; + char buf[PTHREAD_MAX_THREADNAME_LEN_INCLUDING_NULL_BYTE]; const int get_name_rc = pthread_getname_np(thread_handle_, buf, sizeof(buf)); if (get_name_rc != 0) { - ENVOY_LOG_MISC(trace, "Error {} getting name: {}", get_name_rc, strerror(get_name_rc)); + ENVOY_LOG_MISC(trace, "Error {} getting name", get_name_rc); return false; } name = buf; diff --git a/test/common/common/thread_test.cc b/test/common/common/thread_test.cc index cddcee25dc7b5..6c780bad2c17c 100644 --- a/test/common/common/thread_test.cc +++ b/test/common/common/thread_test.cc @@ -230,8 +230,10 @@ TEST_F(ThreadAsyncPtrTest, NameNotSpecifiedWait) { auto thread = thread_factory_.createThread([¬ify]() { notify.WaitForNotification(); }); notify.Notify(); - // For linux builds, the thread name defaults to the name of the binary. - // Currently, this population does not occur for Mac or Windows. + // For linux builds, the thread name defaults to the name of the + // binary. However the name of the binary is different depending on whether + // this is a coverage test or not. Currently, this population does not occur + // for Mac or Windows. #ifdef __linux__ EXPECT_FALSE(thread->name().empty()); #endif From 7be7e687f0635b593803764f029c21190a53af75 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 5 Jun 2020 17:20:20 -0400 Subject: [PATCH 12/13] fix clang-tidy error Signed-off-by: Joshua Marantz --- source/common/common/posix/thread_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/common/posix/thread_impl.cc b/source/common/common/posix/thread_impl.cc index 408a7a67f3f90..32dbbb4a0e721 100644 --- a/source/common/common/posix/thread_impl.cc +++ b/source/common/common/posix/thread_impl.cc @@ -81,7 +81,7 @@ class ThreadImplPosix : public Thread { #endif } - ~ThreadImplPosix() { ASSERT(joined_); } + ~ThreadImplPosix() override { ASSERT(joined_); } std::string name() const override { return name_; } From 93e6d9bb513ddc6693b062aa075d69d9cefcc7cd Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Mon, 8 Jun 2020 08:31:40 -0400 Subject: [PATCH 13/13] minor cleanups. Signed-off-by: Joshua Marantz --- source/common/common/posix/thread_impl.cc | 6 ++++-- test/common/common/thread_test.cc | 10 ++++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/source/common/common/posix/thread_impl.cc b/source/common/common/posix/thread_impl.cc index 32dbbb4a0e721..359af8245ed97 100644 --- a/source/common/common/posix/thread_impl.cc +++ b/source/common/common/posix/thread_impl.cc @@ -40,8 +40,7 @@ class ThreadImplPosix : public Thread { ThreadImplPosix(std::function thread_routine, OptionsOptConstRef options) : thread_routine_(std::move(thread_routine)) { if (options) { - name_ = (std::string( - options->name_.substr(0, PTHREAD_MAX_THREADNAME_LEN_INCLUDING_NULL_BYTE - 1))); + name_ = options->name_.substr(0, PTHREAD_MAX_THREADNAME_LEN_INCLUDING_NULL_BYTE - 1); } RELEASE_ASSERT(Logger::Registry::initialized(), ""); const int rc = pthread_create( @@ -95,6 +94,9 @@ class ThreadImplPosix : public Thread { private: #ifdef __linux__ + // Attempts to get the name from the operating system, returning true and + // updating 'name' if successful. Note that during normal operation this + // may fail, if the thread exits prior to the system call. bool getNameFromOS(std::string& name) { // Verify that the name got written into the thread as expected. char buf[PTHREAD_MAX_THREADNAME_LEN_INCLUDING_NULL_BYTE]; diff --git a/test/common/common/thread_test.cc b/test/common/common/thread_test.cc index 6c780bad2c17c..9dac043921f72 100644 --- a/test/common/common/thread_test.cc +++ b/test/common/common/thread_test.cc @@ -219,9 +219,15 @@ TEST_F(ThreadAsyncPtrTest, TruncateNoWait) { auto thread = thread_factory_.createThread([]() {}, Options{"this name is way too long for posix"}); - // To make this test work on multiple platforms, just assume the first 10 characters - // are retained. + // In general, across platforms, just assume the first 10 characters are + // retained. EXPECT_THAT(thread->name(), testing::StartsWith("this name ")); + + // On Linux we can check for 15 exactly. +#ifdef __linux__ + EXPECT_EQ("this name is wa", thread->name()) << "truncated to 15 chars"; +#endif + thread->join(); }