Skip to content

Commit

Permalink
util: avoid using thread_local variable that has a destructor
Browse files Browse the repository at this point in the history
Store the thread name in a `thread_local` variable of type `char[]`
instead of `std::string`. This avoids calling the destructor when
the thread exits. This is a workaround for
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=278701

For type-safety, return `std::string` from
`util::ThreadGetInternalName()` instead of `char[]`.

As a side effect of this change, we no longer store a reference
to a `thread_local` variable in `CLockLocation`. This was
dangerous because if the thread quits while the reference still
exists (in the global variable `lock_data`, see inside `GetLockData()`)
then the reference will become dangling.
  • Loading branch information
vasild committed May 14, 2024
1 parent b940619 commit c5f9afd
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 21 deletions.
5 changes: 0 additions & 5 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -1049,11 +1049,6 @@ if test "$use_thread_local" = "yes" || test "$use_thread_local" = "auto"; then
dnl https://gist.github.com/jamesob/fe9a872051a88b2025b1aa37bfa98605
AC_MSG_RESULT([no])
;;
*freebsd*)
dnl FreeBSD's implementation of thread_local is also buggy (per
dnl https://groups.google.com/d/msg/bsdmailinglist/22ncTZAbDp4/Dii_pII5AwAJ)
AC_MSG_RESULT([no])
;;
*)
AC_DEFINE([HAVE_THREAD_LOCAL], [1], [Define if thread_local is supported.])
AC_MSG_RESULT([yes])
Expand Down
6 changes: 3 additions & 3 deletions src/sync.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,11 @@ struct CLockLocation {
const char* pszFile,
int nLine,
bool fTryIn,
const std::string& thread_name)
std::string&& thread_name)
: fTry(fTryIn),
mutexName(pszName),
sourceFile(pszFile),
m_thread_name(thread_name),
m_thread_name(std::move(thread_name)),
sourceLine(nLine) {}

std::string ToString() const
Expand All @@ -60,7 +60,7 @@ struct CLockLocation {
bool fTry;
std::string mutexName;
std::string sourceFile;
const std::string& m_thread_name;
const std::string m_thread_name;
int sourceLine;
};

Expand Down
25 changes: 15 additions & 10 deletions src/util/threadnames.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include <config/bitcoin-config.h> // IWYU pragma: keep

#include <cstring>
#include <string>
#include <thread>
#include <utility>
Expand Down Expand Up @@ -40,27 +41,31 @@ static void SetThreadName(const char* name)
// global.
#if defined(HAVE_THREAD_LOCAL)

static thread_local std::string g_thread_name;
const std::string& util::ThreadGetInternalName() { return g_thread_name; }
static thread_local char g_thread_name[128]{'\0'};
std::string util::ThreadGetInternalName() { return g_thread_name; }
//! Set the in-memory internal name for this thread. Does not affect the process
//! name.
static void SetInternalName(std::string name) { g_thread_name = std::move(name); }
static void SetInternalName(const std::string& name)
{
const size_t copy_bytes{std::min(sizeof(g_thread_name) - 1, name.length())};
std::memcpy(g_thread_name, name.data(), copy_bytes);
g_thread_name[copy_bytes] = '\0';
}

// Without thread_local available, don't handle internal name at all.
#else

static const std::string empty_string;
const std::string& util::ThreadGetInternalName() { return empty_string; }
static void SetInternalName(std::string name) { }
std::string util::ThreadGetInternalName() { return ""; }
static void SetInternalName(const std::string& name) { }
#endif

void util::ThreadRename(std::string&& name)
void util::ThreadRename(const std::string& name)
{
SetThreadName(("b-" + name).c_str());
SetInternalName(std::move(name));
SetInternalName(name);
}

void util::ThreadSetInternalName(std::string&& name)
void util::ThreadSetInternalName(const std::string& name)
{
SetInternalName(std::move(name));
SetInternalName(name);
}
6 changes: 3 additions & 3 deletions src/util/threadnames.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,14 @@ namespace util {
//! as its system thread name.
//! @note Do not call this for the main thread, as this will interfere with
//! UNIX utilities such as top and killall. Use ThreadSetInternalName instead.
void ThreadRename(std::string&&);
void ThreadRename(const std::string&);

//! Set the internal (in-memory) name of the current thread only.
void ThreadSetInternalName(std::string&&);
void ThreadSetInternalName(const std::string&);

//! Get the thread's internal (in-memory) name; used e.g. for identification in
//! logging.
const std::string& ThreadGetInternalName();
std::string ThreadGetInternalName();

} // namespace util

Expand Down

0 comments on commit c5f9afd

Please sign in to comment.