From 224d0a3fb2b899f077abe18579dc0334e8f2ef86 Mon Sep 17 00:00:00 2001 From: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com> Date: Sat, 13 Jun 2020 18:21:30 +0000 Subject: [PATCH] Backport 12381 (#3528) * Merge #12381: Remove more boost threads 004f999 boost: drop boost threads for [alert|block|wallet]notify (Cory Fields) 0827267 boost: drop boost threads from torcontrol (Cory Fields) ba91724 boost: remove useless threadGroup parameter from Discover (Cory Fields) f26866b boost: drop boost threads for upnp (Cory Fields) Pull request description: This doesn't completely get rid of boost::thread, but this batch should be easy to review, and leaves us with only threadGroup (scheduler + scriptcheck) remaining. Note to reviewers: The upnp diff changes a bunch of whitespace, it's much more clear with 'git diff -w' Tree-SHA512: 5a356798d0785f93ed143d1f0afafe890bc82f0d470bc969473da2d2aa78bcb9b096f7ba11b92564d546fb447d4bd0d347e7842994ea0170aafd53fda7e0a66e * fix using std::thread Signed-off-by: pasta * Switch to std::thread in NotifyTransactionLock * Move StopTorControl call from Shutdown to PrepareShutdown Co-authored-by: Wladimir J. van der Laan Co-authored-by: UdjinM6 --- src/init.cpp | 19 +++++---- src/net.cpp | 86 ++++++++++++++++++++++------------------- src/net.h | 10 ++--- src/qt/optionsmodel.cpp | 7 +++- src/torcontrol.cpp | 14 ++----- src/torcontrol.h | 2 +- src/validation.cpp | 3 +- src/wallet/wallet.cpp | 7 ++-- 8 files changed, 80 insertions(+), 68 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index dca5bc2cd3fa5..757458a5cb27f 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -220,6 +220,7 @@ void Interrupt() InterruptREST(); InterruptTorControl(); llmq::InterruptLLMQSystem(); + InterruptMapPort(); if (g_connman) g_connman->Interrupt(); } @@ -251,7 +252,7 @@ void PrepareShutdown() bool fRPCInWarmup = RPCIsInWarmup(&statusmessage); g_wallet_init_interface->Flush(); - MapPort(false); + StopMapPort(); // Because these depend on each-other, we make sure that neither can be // using the other before destroying them. @@ -259,6 +260,8 @@ void PrepareShutdown() if (g_connman) g_connman->Stop(); // if (g_txindex) g_txindex->Stop(); //TODO watch out when backporting bitcoin#13033 (don't accidently put the reset here, as we've already backported bitcoin#13894) + StopTorControl(); + // After everything has been shut down, but before things get flushed, stop the // CScheduler/checkqueue threadGroup threadGroup.interrupt_all(); @@ -378,8 +381,7 @@ void Shutdown() if(!fRequestRestart) { PrepareShutdown(); } - // Shutdown part 2: Stop TOR thread and delete wallet instance - StopTorControl(); + // Shutdown part 2: delete wallet instance g_wallet_init_interface->Close(); globalVerifyHandle.reset(); ECC_Stop(); @@ -695,7 +697,8 @@ static void BlockNotifyCallback(bool initialSync, const CBlockIndex *pBlockIndex std::string strCmd = gArgs.GetArg("-blocknotify", ""); if (!strCmd.empty()) { boost::replace_all(strCmd, "%s", pBlockIndex->GetBlockHash().GetHex()); - boost::thread t(runCommand, strCmd); // thread runs free + std::thread t(runCommand, strCmd); + t.detach(); // thread runs free } } @@ -2221,12 +2224,14 @@ bool AppInitMain() } LogPrintf("chainActive.Height() = %d\n", chain_active_height); if (gArgs.GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION)) - StartTorControl(threadGroup, scheduler); + StartTorControl(); - Discover(threadGroup); + Discover(); // Map ports with UPnP - MapPort(gArgs.GetBoolArg("-upnp", DEFAULT_UPNP)); + if (gArgs.GetBoolArg("-upnp", DEFAULT_UPNP)) { + StartMapPort(); + } CConnman::Options connOptions; connOptions.nLocalServices = nLocalServices; diff --git a/src/net.cpp b/src/net.cpp index 216b32122bddd..ab1c2e59bf6b5 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1948,6 +1948,8 @@ void CConnman::WakeSelect() #ifdef USE_UPNP +static CThreadInterrupt g_upnp_interrupt; +static std::thread g_upnp_thread; void ThreadMapPort() { std::string port = strprintf("%u", GetListenPort()); @@ -1998,35 +2000,29 @@ void ThreadMapPort() std::string strDesc = "Dash Core " + FormatFullVersion(); - try { - while (true) { + do { #ifndef UPNPDISCOVER_SUCCESS - /* miniupnpc 1.5 */ - r = UPNP_AddPortMapping(urls.controlURL, data.first.servicetype, - port.c_str(), port.c_str(), lanaddr, strDesc.c_str(), "TCP", 0); + /* miniupnpc 1.5 */ + r = UPNP_AddPortMapping(urls.controlURL, data.first.servicetype, + port.c_str(), port.c_str(), lanaddr, strDesc.c_str(), "TCP", 0); #else - /* miniupnpc 1.6 */ - r = UPNP_AddPortMapping(urls.controlURL, data.first.servicetype, - port.c_str(), port.c_str(), lanaddr, strDesc.c_str(), "TCP", 0, "0"); + /* miniupnpc 1.6 */ + r = UPNP_AddPortMapping(urls.controlURL, data.first.servicetype, + port.c_str(), port.c_str(), lanaddr, strDesc.c_str(), "TCP", 0, "0"); #endif - if(r!=UPNPCOMMAND_SUCCESS) - LogPrintf("AddPortMapping(%s, %s, %s) failed with code %d (%s)\n", - port, port, lanaddr, r, strupnperror(r)); - else - LogPrintf("UPnP Port Mapping successful.\n"); - - MilliSleep(20*60*1000); // Refresh every 20 minutes - } - } - catch (const boost::thread_interrupted&) - { - r = UPNP_DeletePortMapping(urls.controlURL, data.first.servicetype, port.c_str(), "TCP", 0); - LogPrintf("UPNP_DeletePortMapping() returned: %d\n", r); - freeUPNPDevlist(devlist); devlist = nullptr; - FreeUPNPUrls(&urls); - throw; + if(r!=UPNPCOMMAND_SUCCESS) + LogPrintf("AddPortMapping(%s, %s, %s) failed with code %d (%s)\n", + port, port, lanaddr, r, strupnperror(r)); + else + LogPrintf("UPnP Port Mapping successful.\n"); } + while(g_upnp_interrupt.sleep_for(std::chrono::minutes(20))); + + r = UPNP_DeletePortMapping(urls.controlURL, data.first.servicetype, port.c_str(), "TCP", 0); + LogPrintf("UPNP_DeletePortMapping() returned: %d\n", r); + freeUPNPDevlist(devlist); devlist = nullptr; + FreeUPNPUrls(&urls); } else { LogPrintf("No valid UPnP IGDs found\n"); freeUPNPDevlist(devlist); devlist = nullptr; @@ -2035,27 +2031,39 @@ void ThreadMapPort() } } -void MapPort(bool fUseUPnP) +void StartMapPort() { - static std::unique_ptr upnp_thread; + if (!g_upnp_thread.joinable()) { + assert(!g_upnp_interrupt); + g_upnp_thread = std::thread((std::bind(&TraceThread, "upnp", &ThreadMapPort))); + } +} - if (fUseUPnP) - { - if (upnp_thread) { - upnp_thread->interrupt(); - upnp_thread->join(); - } - upnp_thread.reset(new boost::thread(boost::bind(&TraceThread, "upnp", &ThreadMapPort))); +void InterruptMapPort() +{ + if(g_upnp_thread.joinable()) { + g_upnp_interrupt(); } - else if (upnp_thread) { - upnp_thread->interrupt(); - upnp_thread->join(); - upnp_thread.reset(); +} + +void StopMapPort() +{ + if(g_upnp_thread.joinable()) { + g_upnp_thread.join(); + g_upnp_interrupt.reset(); } } #else -void MapPort(bool) +void StartMapPort() +{ + // Intentionally left blank. +} +void InterruptMapPort() +{ + // Intentionally left blank. +} +void StopMapPort() { // Intentionally left blank. } @@ -2827,7 +2835,7 @@ bool CConnman::BindListenPort(const CService &addrBind, std::string& strError, b return true; } -void Discover(boost::thread_group& threadGroup) +void Discover() { if (!fDiscover) return; diff --git a/src/net.h b/src/net.h index 790a03cb4c7a0..f820eaf00c44e 100644 --- a/src/net.h +++ b/src/net.h @@ -46,10 +46,6 @@ class CScheduler; class CNode; -namespace boost { - class thread_group; -} // namespace boost - /** Time between pings automatically sent out for latency probing and keepalive (in seconds). */ static const int PING_INTERVAL = 2 * 60; /** Time after which to disconnect, after waiting for a ping response (or inactivity). */ @@ -651,8 +647,10 @@ friend class CNode; friend struct CConnmanTest; }; extern std::unique_ptr g_connman; -void Discover(boost::thread_group& threadGroup); -void MapPort(bool fUseUPnP); +void Discover(); +void StartMapPort(); +void InterruptMapPort(); +void StopMapPort(); unsigned short GetListenPort(); bool BindListenPort(const CService &bindAddr, std::string& strError, bool fWhitelisted = false); diff --git a/src/qt/optionsmodel.cpp b/src/qt/optionsmodel.cpp index 440c9d60a9965..e56da201aa2dc 100644 --- a/src/qt/optionsmodel.cpp +++ b/src/qt/optionsmodel.cpp @@ -379,7 +379,12 @@ bool OptionsModel::setData(const QModelIndex & index, const QVariant & value, in break; case MapPortUPnP: // core option - can be changed on-the-fly settings.setValue("fUseUPnP", value.toBool()); - MapPort(value.toBool()); + if (value.toBool()) { + StartMapPort(); + } else { + InterruptMapPort(); + StopMapPort(); + } break; case MinimizeOnClose: fMinimizeOnClose = value.toBool(); diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp index ccff8e976fc36..66f1cea416d11 100644 --- a/src/torcontrol.cpp +++ b/src/torcontrol.cpp @@ -731,7 +731,7 @@ void TorController::reconnect_cb(evutil_socket_t fd, short what, void *arg) /****** Thread ********/ static struct event_base *gBase; -static boost::thread torControlThread; +static std::thread torControlThread; static void TorControlThread() { @@ -740,7 +740,7 @@ static void TorControlThread() event_base_dispatch(gBase); } -void StartTorControl(boost::thread_group& threadGroup, CScheduler& scheduler) +void StartTorControl() { assert(!gBase); #ifdef WIN32 @@ -754,7 +754,7 @@ void StartTorControl(boost::thread_group& threadGroup, CScheduler& scheduler) return; } - torControlThread = boost::thread(boost::bind(&TraceThread, "torcontrol", &TorControlThread)); + torControlThread = std::thread(std::bind(&TraceThread, "torcontrol", &TorControlThread)); } void InterruptTorControl() @@ -767,14 +767,8 @@ void InterruptTorControl() void StopTorControl() { - // timed_join() avoids the wallet not closing during a repair-restart. For a 'normal' wallet exit - // it behaves for our cases exactly like the normal join() if (gBase) { -#if BOOST_VERSION >= 105000 - torControlThread.try_join_for(boost::chrono::seconds(1)); -#else - torControlThread.timed_join(boost::posix_time::seconds(1)); -#endif + torControlThread.join(); event_base_free(gBase); gBase = nullptr; } diff --git a/src/torcontrol.h b/src/torcontrol.h index a01eeefeb8382..f4bfa24c0316e 100644 --- a/src/torcontrol.h +++ b/src/torcontrol.h @@ -13,7 +13,7 @@ extern const std::string DEFAULT_TOR_CONTROL; static const bool DEFAULT_LISTEN_ONION = true; -void StartTorControl(boost::thread_group& threadGroup, CScheduler& scheduler); +void StartTorControl(); void InterruptTorControl(); void StopTorControl(); diff --git a/src/validation.cpp b/src/validation.cpp index 6497674f67052..1835bb00ffcea 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1214,7 +1214,8 @@ static void AlertNotify(const std::string& strMessage) safeStatus = singleQuote+safeStatus+singleQuote; boost::replace_all(strCmd, "%s", safeStatus); - boost::thread t(runCommand, strCmd); // thread runs free + std::thread t(runCommand, strCmd); + t.detach(); // thread runs free } static void CheckForkWarningConditions() diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ddc9ca3c3babe..4df2b6c7e80ca 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -45,7 +45,6 @@ #include #include -#include static CCriticalSection cs_wallets; static std::vector vpwallets GUARDED_BY(cs_wallets); @@ -1169,7 +1168,8 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose) if (!strCmd.empty()) { boost::replace_all(strCmd, "%s", wtxIn.GetHash().GetHex()); - boost::thread t(runCommand, strCmd); // thread runs free + std::thread t(runCommand, strCmd); + t.detach(); // thread runs free } fAnonymizableTallyCached = false; @@ -5439,7 +5439,8 @@ void CWallet::NotifyTransactionLock(const CTransaction &tx, const llmq::CInstant std::string strCmd = gArgs.GetArg("-instantsendnotify", ""); if (!strCmd.empty()) { boost::replace_all(strCmd, "%s", txHash.GetHex()); - boost::thread t(runCommand, strCmd); // thread runs free + std::thread t(runCommand, strCmd); + t.detach(); // thread runs free } } }