Close minor startup race between main and scheduler threads

Don't schedule class PeerManagerImpl's background tasks from its
constructor, but instead do that from a separate method,
StartScheduledTasks(), that can be called later at the end of startup,
after other things, such as the active chain, are initialzed.
This commit is contained in:
Larry Ruane 2021-07-28 16:18:53 -06:00
parent 4b1fb50def
commit 703b1e612a
5 changed files with 21 additions and 11 deletions

View file

@ -1180,7 +1180,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
assert(!node.peerman);
node.peerman = PeerManager::make(chainparams, *node.connman, *node.addrman, node.banman.get(),
*node.scheduler, chainman, *node.mempool, ignores_incoming_txs);
chainman, *node.mempool, ignores_incoming_txs);
RegisterValidationInterface(node.peerman.get());
// sanitize comments per BIP-0014, format user agent and check total size
@ -1789,6 +1789,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
banman->DumpBanlist();
}, DUMP_BANS_INTERVAL);
if (node.peerman) node.peerman->StartScheduledTasks(*node.scheduler);
#if HAVE_SYSTEM
StartupNotify(args);
#endif

View file

@ -271,7 +271,7 @@ class PeerManagerImpl final : public PeerManager
{
public:
PeerManagerImpl(const CChainParams& chainparams, CConnman& connman, CAddrMan& addrman,
BanMan* banman, CScheduler& scheduler, ChainstateManager& chainman,
BanMan* banman, ChainstateManager& chainman,
CTxMemPool& pool, bool ignore_incoming_txs);
/** Overridden from CValidationInterface. */
@ -288,6 +288,7 @@ public:
bool SendMessages(CNode* pto) override EXCLUSIVE_LOCKS_REQUIRED(pto->cs_sendProcessing);
/** Implement PeerManager */
void StartScheduledTasks(CScheduler& scheduler) override;
void CheckForStaleTipAndEvictPeers() override;
bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const override;
bool IgnoresIncomingTxs() override { return m_ignore_incoming_txs; }
@ -1396,14 +1397,14 @@ bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex)
}
std::unique_ptr<PeerManager> PeerManager::make(const CChainParams& chainparams, CConnman& connman, CAddrMan& addrman,
BanMan* banman, CScheduler& scheduler, ChainstateManager& chainman,
BanMan* banman, ChainstateManager& chainman,
CTxMemPool& pool, bool ignore_incoming_txs)
{
return std::make_unique<PeerManagerImpl>(chainparams, connman, addrman, banman, scheduler, chainman, pool, ignore_incoming_txs);
return std::make_unique<PeerManagerImpl>(chainparams, connman, addrman, banman, chainman, pool, ignore_incoming_txs);
}
PeerManagerImpl::PeerManagerImpl(const CChainParams& chainparams, CConnman& connman, CAddrMan& addrman,
BanMan* banman, CScheduler& scheduler, ChainstateManager& chainman,
BanMan* banman, ChainstateManager& chainman,
CTxMemPool& pool, bool ignore_incoming_txs)
: m_chainparams(chainparams),
m_connman(connman),
@ -1412,6 +1413,10 @@ PeerManagerImpl::PeerManagerImpl(const CChainParams& chainparams, CConnman& conn
m_chainman(chainman),
m_mempool(pool),
m_ignore_incoming_txs(ignore_incoming_txs)
{
}
void PeerManagerImpl::StartScheduledTasks(CScheduler& scheduler)
{
// Stale tip checking and peer eviction are on two different timers, but we
// don't want them to get out of sync due to drift in the scheduler, so we

View file

@ -37,10 +37,13 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
{
public:
static std::unique_ptr<PeerManager> make(const CChainParams& chainparams, CConnman& connman, CAddrMan& addrman,
BanMan* banman, CScheduler& scheduler, ChainstateManager& chainman,
BanMan* banman, ChainstateManager& chainman,
CTxMemPool& pool, bool ignore_incoming_txs);
virtual ~PeerManager() { }
/** Begin running background tasks, should only be called once */
virtual void StartScheduledTasks(CScheduler& scheduler) = 0;
/** Get statistics from node state */
virtual bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const = 0;

View file

@ -53,7 +53,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
const CChainParams& chainparams = Params();
auto connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman);
auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, nullptr,
*m_node.scheduler, *m_node.chainman, *m_node.mempool, false);
*m_node.chainman, *m_node.mempool, false);
// Mock an outbound peer
CAddress addr1(ip(0xa0b0c001), NODE_NONE);
@ -121,7 +121,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
const CChainParams& chainparams = Params();
auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman);
auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, nullptr,
*m_node.scheduler, *m_node.chainman, *m_node.mempool, false);
*m_node.chainman, *m_node.mempool, false);
constexpr int max_outbound_full_relay = MAX_OUTBOUND_FULL_RELAY_CONNECTIONS;
CConnman::Options options;
@ -194,7 +194,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
auto banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME);
auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman);
auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, banman.get(),
*m_node.scheduler, *m_node.chainman, *m_node.mempool, false);
*m_node.chainman, *m_node.mempool, false);
CNetAddr tor_netaddr;
BOOST_REQUIRE(
@ -288,7 +288,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
auto banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME);
auto connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman);
auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, banman.get(),
*m_node.scheduler, *m_node.chainman, *m_node.mempool, false);
*m_node.chainman, *m_node.mempool, false);
banman->ClearBanned();
int64_t nStartTime = GetTime();

View file

@ -197,7 +197,7 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
m_node.banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME);
m_node.connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman); // Deterministic randomness for tests.
m_node.peerman = PeerManager::make(chainparams, *m_node.connman, *m_node.addrman,
m_node.banman.get(), *m_node.scheduler, *m_node.chainman,
m_node.banman.get(), *m_node.chainman,
*m_node.mempool, false);
{
CConnman::Options options;