From ae5f2b6a6cc7b2260e9dff99c1bf378922e0e988 Mon Sep 17 00:00:00 2001
From: James O'Beirne <james.obeirne@gmail.com>
Date: Wed, 13 Jun 2018 14:50:59 -0400
Subject: [PATCH] threads: introduce util/threadnames, refactor thread naming

This work is prerequisite to attaching thread names to log lines and deadlock
debug utilities. This code allows setting of an "internal" threadname per
thread on platforms where thread_local is available.

This commit also moves RenameThread() out of a more general module and adds a
numeric suffix to disambiguate between threads with the same name. It
explicitly names a few main threads using the new util::ThreadRename().
---
 src/Makefile.am           |  2 ++
 src/bitcoind.cpp          |  3 +++
 src/httpserver.cpp        | 11 ++++----
 src/init.cpp              |  7 ++---
 src/qt/bitcoin.cpp        |  3 +++
 src/test/setup_common.cpp |  2 +-
 src/util/system.cpp       | 20 --------------
 src/util/system.h         |  6 ++---
 src/util/threadnames.cpp  | 57 +++++++++++++++++++++++++++++++++++++++
 src/util/threadnames.h    | 21 +++++++++++++++
 src/validation.cpp        |  5 ++--
 src/validation.h          |  2 +-
 12 files changed, 103 insertions(+), 36 deletions(-)
 create mode 100644 src/util/threadnames.cpp
 create mode 100644 src/util/threadnames.h

diff --git a/src/Makefile.am b/src/Makefile.am
index 059ed18134..0fefa34cd8 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -210,6 +210,7 @@ BITCOIN_CORE_H = \
   util/memory.h \
   util/moneystr.h \
   util/rbf.h \
+  util/threadnames.h \
   util/time.h \
   util/url.h \
   util/validation.h \
@@ -491,6 +492,7 @@ libbitcoin_util_a_SOURCES = \
   util/system.cpp \
   util/moneystr.cpp \
   util/rbf.cpp \
+  util/threadnames.cpp \
   util/strencodings.cpp \
   util/time.cpp \
   util/url.cpp \
diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
index dde75c1b12..b31f86cdd9 100644
--- a/src/bitcoind.cpp
+++ b/src/bitcoind.cpp
@@ -19,6 +19,7 @@
 #include <util/system.h>
 #include <httpserver.h>
 #include <httprpc.h>
+#include <util/threadnames.h>
 #include <util/strencodings.h>
 #include <walletinitinterface.h>
 
@@ -64,6 +65,8 @@ static bool AppInit(int argc, char* argv[])
 
     bool fRet = false;
 
+    util::ThreadRename("init");
+
     //
     // Parameters
     //
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 5d9c3d2c1a..63639fa3e0 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -6,6 +6,7 @@
 
 #include <chainparamsbase.h>
 #include <compat.h>
+#include <util/threadnames.h>
 #include <util/system.h>
 #include <util/strencodings.h>
 #include <netbase.h>
@@ -17,7 +18,7 @@
 #include <memory>
 #include <stdio.h>
 #include <stdlib.h>
-#include <string.h>
+#include <string>
 
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -284,7 +285,7 @@ static void http_reject_request_cb(struct evhttp_request* req, void*)
 /** Event dispatcher thread */
 static bool ThreadHTTP(struct event_base* base)
 {
-    RenameThread("bitcoin-http");
+    util::ThreadRename("http");
     LogPrint(BCLog::HTTP, "Entering http event loop\n");
     event_base_dispatch(base);
     // Event loop will be interrupted by InterruptHTTPServer()
@@ -335,9 +336,9 @@ static bool HTTPBindAddresses(struct evhttp* http)
 }
 
 /** Simple wrapper to set thread name and run work queue */
-static void HTTPWorkQueueRun(WorkQueue<HTTPClosure>* queue)
+static void HTTPWorkQueueRun(WorkQueue<HTTPClosure>* queue, int worker_num)
 {
-    RenameThread("bitcoin-httpworker");
+    util::ThreadRename(strprintf("httpworker.%i", worker_num));
     queue->Run();
 }
 
@@ -430,7 +431,7 @@ void StartHTTPServer()
     threadHTTP = std::thread(ThreadHTTP, eventBase);
 
     for (int i = 0; i < rpcThreads; i++) {
-        g_thread_http_workers.emplace_back(HTTPWorkQueueRun, workQueue);
+        g_thread_http_workers.emplace_back(HTTPWorkQueueRun, workQueue, i);
     }
 }
 
diff --git a/src/init.cpp b/src/init.cpp
index 70459994c7..408a133e4e 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -42,6 +42,7 @@
 #include <script/sigcache.h>
 #include <scheduler.h>
 #include <shutdown.h>
+#include <util/threadnames.h>
 #include <timedata.h>
 #include <txdb.h>
 #include <txmempool.h>
@@ -208,7 +209,7 @@ void Shutdown(InitInterfaces& interfaces)
     /// for example if the data directory was found to be locked.
     /// Be sure that anything that writes files or flushes caches only does this if the respective
     /// module was initialized.
-    RenameThread("bitcoin-shutoff");
+    util::ThreadRename("shutoff");
     mempool.AddTransactionsUpdated(1);
 
     StopHTTPRPC();
@@ -669,7 +670,7 @@ static void CleanupBlockRevFiles()
 static void ThreadImport(std::vector<fs::path> vImportFiles)
 {
     const CChainParams& chainparams = Params();
-    RenameThread("bitcoin-loadblk");
+    util::ThreadRename("loadblk");
     ScheduleBatchPriority();
 
     {
@@ -1305,7 +1306,7 @@ bool AppInitMain(InitInterfaces& interfaces)
     LogPrintf("Using %u threads for script verification\n", nScriptCheckThreads);
     if (nScriptCheckThreads) {
         for (int i=0; i<nScriptCheckThreads-1; i++)
-            threadGroup.create_thread(&ThreadScriptCheck);
+            threadGroup.create_thread([i]() { return ThreadScriptCheck(i); });
     }
 
     // Start the lightweight task scheduler thread
diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
index 1b063771ef..81255aaae9 100644
--- a/src/qt/bitcoin.cpp
+++ b/src/qt/bitcoin.cpp
@@ -30,6 +30,7 @@
 #include <interfaces/handler.h>
 #include <interfaces/node.h>
 #include <noui.h>
+#include <util/threadnames.h>
 #include <rpc/server.h>
 #include <ui_interface.h>
 #include <uint256.h>
@@ -149,6 +150,7 @@ void BitcoinCore::initialize()
     try
     {
         qDebug() << __func__ << ": Running initialization in thread";
+        util::ThreadRename("qt-init");
         bool rv = m_node.appInitMain();
         Q_EMIT initializeResult(rv);
     } catch (const std::exception& e) {
@@ -423,6 +425,7 @@ int GuiMain(int argc, char* argv[])
     std::tie(argc, argv) = winArgs.get();
 #endif
     SetupEnvironment();
+    util::ThreadRename("main");
 
     std::unique_ptr<interfaces::Node> node = interfaces::MakeNode();
 
diff --git a/src/test/setup_common.cpp b/src/test/setup_common.cpp
index 29633cc7bf..6b671b2400 100644
--- a/src/test/setup_common.cpp
+++ b/src/test/setup_common.cpp
@@ -94,7 +94,7 @@ TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(cha
 
     nScriptCheckThreads = 3;
     for (int i = 0; i < nScriptCheckThreads - 1; i++)
-        threadGroup.create_thread(&ThreadScriptCheck);
+        threadGroup.create_thread([i]() { return ThreadScriptCheck(i); });
 
     g_banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
     g_connman = MakeUnique<CConnman>(0x1337, 0x1337); // Deterministic randomness for tests.
diff --git a/src/util/system.cpp b/src/util/system.cpp
index 9594dd81bf..efd35bed55 100644
--- a/src/util/system.cpp
+++ b/src/util/system.cpp
@@ -60,10 +60,6 @@
 #include <shlobj.h>
 #endif
 
-#ifdef HAVE_SYS_PRCTL_H
-#include <sys/prctl.h>
-#endif
-
 #ifdef HAVE_MALLOPT_ARENA_MAX
 #include <malloc.h>
 #endif
@@ -1137,22 +1133,6 @@ void runCommand(const std::string& strCommand)
         LogPrintf("runCommand error: system(%s) returned %d\n", strCommand, nErr);
 }
 
-void RenameThread(const char* name)
-{
-#if defined(PR_SET_NAME)
-    // Only the first 15 characters are used (16 - NUL terminator)
-    ::prctl(PR_SET_NAME, name, 0, 0, 0);
-#elif (defined(__FreeBSD__) || defined(__OpenBSD__) || defined(__DragonFly__))
-    pthread_set_name_np(pthread_self(), name);
-
-#elif defined(MAC_OSX)
-    pthread_setname_np(name);
-#else
-    // Prevent warnings for unused parameters...
-    (void)name;
-#endif
-}
-
 void SetupEnvironment()
 {
 #ifdef HAVE_MALLOPT_ARENA_MAX
diff --git a/src/util/system.h b/src/util/system.h
index 54eb88e261..1a83cb67b1 100644
--- a/src/util/system.h
+++ b/src/util/system.h
@@ -20,6 +20,7 @@
 #include <fs.h>
 #include <logging.h>
 #include <sync.h>
+#include <util/threadnames.h>
 #include <tinyformat.h>
 #include <util/memory.h>
 #include <util/time.h>
@@ -325,15 +326,12 @@ std::string HelpMessageOpt(const std::string& option, const std::string& message
  */
 int GetNumCores();
 
-void RenameThread(const char* name);
-
 /**
  * .. and a wrapper that just calls func once
  */
 template <typename Callable> void TraceThread(const char* name,  Callable func)
 {
-    std::string s = strprintf("bitcoin-%s", name);
-    RenameThread(s.c_str());
+    util::ThreadRename(name);
     try
     {
         LogPrintf("%s thread start\n", name);
diff --git a/src/util/threadnames.cpp b/src/util/threadnames.cpp
new file mode 100644
index 0000000000..7b0d744aec
--- /dev/null
+++ b/src/util/threadnames.cpp
@@ -0,0 +1,57 @@
+// Copyright (c) 2018 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#if defined(HAVE_CONFIG_H)
+#include <config/bitcoin-config.h>
+#endif
+
+#include <atomic>
+#include <thread>
+
+#include <util/threadnames.h>
+
+#ifdef HAVE_SYS_PRCTL_H
+#include <sys/prctl.h> // For prctl, PR_SET_NAME, PR_GET_NAME
+#endif
+
+//! Set the thread's name at the process level. Does not affect the
+//! internal name.
+static void SetThreadName(const char* name)
+{
+#if defined(PR_SET_NAME)
+    // Only the first 15 characters are used (16 - NUL terminator)
+    ::prctl(PR_SET_NAME, name, 0, 0, 0);
+#elif (defined(__FreeBSD__) || defined(__OpenBSD__) || defined(__DragonFly__))
+    pthread_set_name_np(pthread_self(), name);
+#elif defined(MAC_OSX)
+    pthread_setname_np(name);
+#else
+    // Prevent warnings for unused parameters...
+    (void)name;
+#endif
+}
+
+// If we have thread_local, just keep thread ID and name in a thread_local
+// global.
+#if defined(HAVE_THREAD_LOCAL)
+
+static thread_local std::string g_thread_name;
+const 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); }
+
+// 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) { }
+#endif
+
+void util::ThreadRename(std::string&& name)
+{
+    SetThreadName(("bitcoin-" + name).c_str());
+    SetInternalName(std::move(name));
+}
diff --git a/src/util/threadnames.h b/src/util/threadnames.h
new file mode 100644
index 0000000000..aaf07b9bf8
--- /dev/null
+++ b/src/util/threadnames.h
@@ -0,0 +1,21 @@
+// Copyright (c) 2018 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_UTIL_THREADNAMES_H
+#define BITCOIN_UTIL_THREADNAMES_H
+
+#include <string>
+
+namespace util {
+//! Rename a thread both in terms of an internal (in-memory) name as well
+//! as its system thread name.
+void ThreadRename(std::string&&);
+
+//! Get the thread's internal (in-memory) name; used e.g. for identification in
+//! logging.
+const std::string& ThreadGetInternalName();
+
+} // namespace util
+
+#endif // BITCOIN_UTIL_THREADNAMES_H
diff --git a/src/validation.cpp b/src/validation.cpp
index be6257ea28..208cf2c3f5 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -48,6 +48,7 @@
 
 #include <future>
 #include <sstream>
+#include <string>
 
 #include <boost/algorithm/string/replace.hpp>
 #include <boost/thread.hpp>
@@ -1654,8 +1655,8 @@ static bool WriteUndoDataForBlock(const CBlockUndo& blockundo, CValidationState&
 
 static CCheckQueue<CScriptCheck> scriptcheckqueue(128);
 
-void ThreadScriptCheck() {
-    RenameThread("bitcoin-scriptch");
+void ThreadScriptCheck(int worker_num) {
+    util::ThreadRename(strprintf("scriptch.%i", worker_num));
     scriptcheckqueue.Thread();
 }
 
diff --git a/src/validation.h b/src/validation.h
index 041d8860b0..82151b1349 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -255,7 +255,7 @@ bool LoadChainTip(const CChainParams& chainparams) EXCLUSIVE_LOCKS_REQUIRED(cs_m
 /** Unload database information */
 void UnloadBlockIndex();
 /** Run an instance of the script checking thread */
-void ThreadScriptCheck();
+void ThreadScriptCheck(int worker_num);
 /** Check whether we are doing an initial block download (synchronizing from disk or network) */
 bool IsInitialBlockDownload();
 /** Retrieve a transaction (from memory pool, or from disk, if possible) */