diff options
author | fanquake <fanquake@gmail.com> | 2020-09-19 16:51:35 +0800 |
---|---|---|
committer | fanquake <fanquake@gmail.com> | 2020-09-19 17:13:28 +0800 |
commit | 831b0ecea9156447a2b6a67d28858bc26d302c1c (patch) | |
tree | 30591da5c34512510715b813d4f51727a067c247 | |
parent | 83b23848f7300addf5bbfcfe47a6f8b75a3599f8 (diff) | |
parent | 6fe2ef2acb00b1df7f6a0c0dea1a81a1924be0e1 (diff) |
Merge #13686: ZMQ: Small cleanups in the ZMQ code
6fe2ef2acb00b1df7f6a0c0dea1a81a1924be0e1 scripted-diff: Rename SendMessage to SendZmqMessage. (Daniel Kraft)
a3ffb6ebebd753cec294c91cef7c603a30cf217e Replace zmqconfig.h by a simple zmqutil. (Daniel Kraft)
7f2ad1b9acef4ccc1b3e1a9f551416235d95cbfd Use std::unique_ptr for CZMQNotifierFactory. (Daniel Kraft)
b93b9d54569145bfcec6cee10968284fe05fe254 Simplify and fix notifier removal on error. (Daniel Kraft)
e15b1cfc310df739b92bd281112dbeb31d3bb30a Various cleanups in zmqnotificationinterface. (Daniel Kraft)
Pull request description:
This contains various small code cleanups that make the ZMQ code easier to read and maintain (at least in my opinion). The only functional change is that a potential memory leak is fixed that would have occured when a notifier is removed from the `notifiers` list after its callback function returned `false` (which is likely not relevant in practice but still a bug).
ACKs for top commit:
instagibbs:
utACK 6fe2ef2acb00b1df7f6a0c0dea1a81a1924be0e1
hebasto:
re-ACK 6fe2ef2acb00b1df7f6a0c0dea1a81a1924be0e1, only the latest commit got a scripted-diff since my [previous](https://github.com/bitcoin/bitcoin/pull/13686#pullrequestreview-487649808) review.
Tree-SHA512: 8206f8713bf3698d7cd4cb235f6657dc1c4dd920f50a8c5f371a559dd17ce5ab6d94d6281165eef860a22fc844a6bb25489ada12c83ebc780efd7ccdc0860f70
-rw-r--r-- | src/Makefile.am | 7 | ||||
-rw-r--r-- | src/zmq/zmqabstractnotifier.cpp | 2 | ||||
-rw-r--r-- | src/zmq/zmqabstractnotifier.h | 12 | ||||
-rw-r--r-- | src/zmq/zmqconfig.h | 22 | ||||
-rw-r--r-- | src/zmq/zmqnotificationinterface.cpp | 114 | ||||
-rw-r--r-- | src/zmq/zmqnotificationinterface.h | 3 | ||||
-rw-r--r-- | src/zmq/zmqpublishnotifier.cpp | 26 | ||||
-rw-r--r-- | src/zmq/zmqpublishnotifier.h | 2 | ||||
-rw-r--r-- | src/zmq/zmqutil.cpp | 14 | ||||
-rw-r--r-- | src/zmq/zmqutil.h | 10 |
10 files changed, 103 insertions, 109 deletions
diff --git a/src/Makefile.am b/src/Makefile.am index 6c52d6e4b0..b23bf062c5 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -263,10 +263,10 @@ BITCOIN_CORE_H = \ walletinitinterface.h \ warnings.h \ zmq/zmqabstractnotifier.h \ - zmq/zmqconfig.h\ zmq/zmqnotificationinterface.h \ zmq/zmqpublishnotifier.h \ - zmq/zmqrpc.h + zmq/zmqrpc.h \ + zmq/zmqutil.h obj/build.h: FORCE @@ -345,7 +345,8 @@ libbitcoin_zmq_a_SOURCES = \ zmq/zmqabstractnotifier.cpp \ zmq/zmqnotificationinterface.cpp \ zmq/zmqpublishnotifier.cpp \ - zmq/zmqrpc.cpp + zmq/zmqrpc.cpp \ + zmq/zmqutil.cpp endif diff --git a/src/zmq/zmqabstractnotifier.cpp b/src/zmq/zmqabstractnotifier.cpp index aae760adde..0d0428f3c0 100644 --- a/src/zmq/zmqabstractnotifier.cpp +++ b/src/zmq/zmqabstractnotifier.cpp @@ -4,6 +4,8 @@ #include <zmq/zmqabstractnotifier.h> +#include <cassert> + const int CZMQAbstractNotifier::DEFAULT_ZMQ_SNDHWM; CZMQAbstractNotifier::~CZMQAbstractNotifier() diff --git a/src/zmq/zmqabstractnotifier.h b/src/zmq/zmqabstractnotifier.h index 887dde7b27..34d7e5ef03 100644 --- a/src/zmq/zmqabstractnotifier.h +++ b/src/zmq/zmqabstractnotifier.h @@ -5,12 +5,16 @@ #ifndef BITCOIN_ZMQ_ZMQABSTRACTNOTIFIER_H #define BITCOIN_ZMQ_ZMQABSTRACTNOTIFIER_H -#include <zmq/zmqconfig.h> +#include <util/memory.h> + +#include <memory> +#include <string> class CBlockIndex; +class CTransaction; class CZMQAbstractNotifier; -typedef CZMQAbstractNotifier* (*CZMQNotifierFactory)(); +using CZMQNotifierFactory = std::unique_ptr<CZMQAbstractNotifier> (*)(); class CZMQAbstractNotifier { @@ -21,9 +25,9 @@ public: virtual ~CZMQAbstractNotifier(); template <typename T> - static CZMQAbstractNotifier* Create() + static std::unique_ptr<CZMQAbstractNotifier> Create() { - return new T(); + return MakeUnique<T>(); } std::string GetType() const { return type; } diff --git a/src/zmq/zmqconfig.h b/src/zmq/zmqconfig.h deleted file mode 100644 index 5f0036206d..0000000000 --- a/src/zmq/zmqconfig.h +++ /dev/null @@ -1,22 +0,0 @@ -// Copyright (c) 2014-2019 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_ZMQ_ZMQCONFIG_H -#define BITCOIN_ZMQ_ZMQCONFIG_H - -#if defined(HAVE_CONFIG_H) -#include <config/bitcoin-config.h> -#endif - -#include <stdarg.h> - -#if ENABLE_ZMQ -#include <zmq.h> -#endif - -#include <primitives/transaction.h> - -void zmqError(const char *str); - -#endif // BITCOIN_ZMQ_ZMQCONFIG_H diff --git a/src/zmq/zmqnotificationinterface.cpp b/src/zmq/zmqnotificationinterface.cpp index d55b106e04..a22772baed 100644 --- a/src/zmq/zmqnotificationinterface.cpp +++ b/src/zmq/zmqnotificationinterface.cpp @@ -4,15 +4,13 @@ #include <zmq/zmqnotificationinterface.h> #include <zmq/zmqpublishnotifier.h> +#include <zmq/zmqutil.h> + +#include <zmq.h> #include <validation.h> #include <util/system.h> -void zmqError(const char *str) -{ - LogPrint(BCLog::ZMQ, "zmq: Error: %s, errno=%s\n", str, zmq_strerror(errno)); -} - CZMQNotificationInterface::CZMQNotificationInterface() : pcontext(nullptr) { } @@ -20,61 +18,52 @@ CZMQNotificationInterface::CZMQNotificationInterface() : pcontext(nullptr) CZMQNotificationInterface::~CZMQNotificationInterface() { Shutdown(); - - for (std::list<CZMQAbstractNotifier*>::iterator i=notifiers.begin(); i!=notifiers.end(); ++i) - { - delete *i; - } } std::list<const CZMQAbstractNotifier*> CZMQNotificationInterface::GetActiveNotifiers() const { std::list<const CZMQAbstractNotifier*> result; - for (const auto* n : notifiers) { - result.push_back(n); + for (const auto& n : notifiers) { + result.push_back(n.get()); } return result; } CZMQNotificationInterface* CZMQNotificationInterface::Create() { - CZMQNotificationInterface* notificationInterface = nullptr; std::map<std::string, CZMQNotifierFactory> factories; - std::list<CZMQAbstractNotifier*> notifiers; - factories["pubhashblock"] = CZMQAbstractNotifier::Create<CZMQPublishHashBlockNotifier>; factories["pubhashtx"] = CZMQAbstractNotifier::Create<CZMQPublishHashTransactionNotifier>; factories["pubrawblock"] = CZMQAbstractNotifier::Create<CZMQPublishRawBlockNotifier>; factories["pubrawtx"] = CZMQAbstractNotifier::Create<CZMQPublishRawTransactionNotifier>; + std::list<std::unique_ptr<CZMQAbstractNotifier>> notifiers; for (const auto& entry : factories) { std::string arg("-zmq" + entry.first); if (gArgs.IsArgSet(arg)) { - CZMQNotifierFactory factory = entry.second; - std::string address = gArgs.GetArg(arg, ""); - CZMQAbstractNotifier *notifier = factory(); + const auto& factory = entry.second; + const std::string address = gArgs.GetArg(arg, ""); + std::unique_ptr<CZMQAbstractNotifier> notifier = factory(); notifier->SetType(entry.first); notifier->SetAddress(address); notifier->SetOutboundMessageHighWaterMark(static_cast<int>(gArgs.GetArg(arg + "hwm", CZMQAbstractNotifier::DEFAULT_ZMQ_SNDHWM))); - notifiers.push_back(notifier); + notifiers.push_back(std::move(notifier)); } } if (!notifiers.empty()) { - notificationInterface = new CZMQNotificationInterface(); - notificationInterface->notifiers = notifiers; + std::unique_ptr<CZMQNotificationInterface> notificationInterface(new CZMQNotificationInterface()); + notificationInterface->notifiers = std::move(notifiers); - if (!notificationInterface->Initialize()) - { - delete notificationInterface; - notificationInterface = nullptr; + if (notificationInterface->Initialize()) { + return notificationInterface.release(); } } - return notificationInterface; + return nullptr; } // Called at startup to conditionally set up ZMQ socket(s) @@ -95,26 +84,15 @@ bool CZMQNotificationInterface::Initialize() return false; } - std::list<CZMQAbstractNotifier*>::iterator i=notifiers.begin(); - for (; i!=notifiers.end(); ++i) - { - CZMQAbstractNotifier *notifier = *i; - if (notifier->Initialize(pcontext)) - { + for (auto& notifier : notifiers) { + if (notifier->Initialize(pcontext)) { LogPrint(BCLog::ZMQ, "zmq: Notifier %s ready (address = %s)\n", notifier->GetType(), notifier->GetAddress()); - } - else - { + } else { LogPrint(BCLog::ZMQ, "zmq: Notifier %s failed (address = %s)\n", notifier->GetType(), notifier->GetAddress()); - break; + return false; } } - if (i!=notifiers.end()) - { - return false; - } - return true; } @@ -124,9 +102,7 @@ void CZMQNotificationInterface::Shutdown() LogPrint(BCLog::ZMQ, "zmq: Shutdown notification interface\n"); if (pcontext) { - for (std::list<CZMQAbstractNotifier*>::iterator i=notifiers.begin(); i!=notifiers.end(); ++i) - { - CZMQAbstractNotifier *notifier = *i; + for (auto& notifier : notifiers) { LogPrint(BCLog::ZMQ, "zmq: Shutdown notifier %s at %s\n", notifier->GetType(), notifier->GetAddress()); notifier->Shutdown(); } @@ -136,45 +112,43 @@ void CZMQNotificationInterface::Shutdown() } } -void CZMQNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) -{ - if (fInitialDownload || pindexNew == pindexFork) // In IBD or blocks were disconnected without any new ones - return; +namespace { - for (std::list<CZMQAbstractNotifier*>::iterator i = notifiers.begin(); i!=notifiers.end(); ) - { - CZMQAbstractNotifier *notifier = *i; - if (notifier->NotifyBlock(pindexNew)) - { - i++; - } - else - { +template <typename Function> +void TryForEachAndRemoveFailed(std::list<std::unique_ptr<CZMQAbstractNotifier>>& notifiers, const Function& func) +{ + for (auto i = notifiers.begin(); i != notifiers.end(); ) { + CZMQAbstractNotifier* notifier = i->get(); + if (func(notifier)) { + ++i; + } else { notifier->Shutdown(); i = notifiers.erase(i); } } } +} // anonymous namespace + +void CZMQNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) +{ + if (fInitialDownload || pindexNew == pindexFork) // In IBD or blocks were disconnected without any new ones + return; + + TryForEachAndRemoveFailed(notifiers, [pindexNew](CZMQAbstractNotifier* notifier) { + return notifier->NotifyBlock(pindexNew); + }); +} + void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef& ptx) { // Used by BlockConnected and BlockDisconnected as well, because they're // all the same external callback. const CTransaction& tx = *ptx; - for (std::list<CZMQAbstractNotifier*>::iterator i = notifiers.begin(); i!=notifiers.end(); ) - { - CZMQAbstractNotifier *notifier = *i; - if (notifier->NotifyTransaction(tx)) - { - i++; - } - else - { - notifier->Shutdown(); - i = notifiers.erase(i); - } - } + TryForEachAndRemoveFailed(notifiers, [&tx](CZMQAbstractNotifier* notifier) { + return notifier->NotifyTransaction(tx); + }); } void CZMQNotificationInterface::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexConnected) diff --git a/src/zmq/zmqnotificationinterface.h b/src/zmq/zmqnotificationinterface.h index 60f3b6148a..0686960ed4 100644 --- a/src/zmq/zmqnotificationinterface.h +++ b/src/zmq/zmqnotificationinterface.h @@ -7,6 +7,7 @@ #include <validationinterface.h> #include <list> +#include <memory> class CBlockIndex; class CZMQAbstractNotifier; @@ -34,7 +35,7 @@ private: CZMQNotificationInterface(); void *pcontext; - std::list<CZMQAbstractNotifier*> notifiers; + std::list<std::unique_ptr<CZMQAbstractNotifier>> notifiers; }; extern CZMQNotificationInterface* g_zmq_notification_interface; diff --git a/src/zmq/zmqpublishnotifier.cpp b/src/zmq/zmqpublishnotifier.cpp index e2431cbbb7..d4d21b05ba 100644 --- a/src/zmq/zmqpublishnotifier.cpp +++ b/src/zmq/zmqpublishnotifier.cpp @@ -2,13 +2,23 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include <zmq/zmqpublishnotifier.h> + #include <chain.h> #include <chainparams.h> +#include <rpc/server.h> #include <streams.h> -#include <zmq/zmqpublishnotifier.h> -#include <validation.h> #include <util/system.h> -#include <rpc/server.h> +#include <validation.h> +#include <zmq/zmqutil.h> + +#include <zmq.h> + +#include <cstdarg> +#include <cstddef> +#include <map> +#include <string> +#include <utility> static std::multimap<std::string, CZMQAbstractPublishNotifier*> mapPublishNotifiers; @@ -149,7 +159,7 @@ void CZMQAbstractPublishNotifier::Shutdown() psocket = nullptr; } -bool CZMQAbstractPublishNotifier::SendMessage(const char *command, const void* data, size_t size) +bool CZMQAbstractPublishNotifier::SendZmqMessage(const char *command, const void* data, size_t size) { assert(psocket); @@ -173,7 +183,7 @@ bool CZMQPublishHashBlockNotifier::NotifyBlock(const CBlockIndex *pindex) char data[32]; for (unsigned int i = 0; i < 32; i++) data[31 - i] = hash.begin()[i]; - return SendMessage(MSG_HASHBLOCK, data, 32); + return SendZmqMessage(MSG_HASHBLOCK, data, 32); } bool CZMQPublishHashTransactionNotifier::NotifyTransaction(const CTransaction &transaction) @@ -183,7 +193,7 @@ bool CZMQPublishHashTransactionNotifier::NotifyTransaction(const CTransaction &t char data[32]; for (unsigned int i = 0; i < 32; i++) data[31 - i] = hash.begin()[i]; - return SendMessage(MSG_HASHTX, data, 32); + return SendZmqMessage(MSG_HASHTX, data, 32); } bool CZMQPublishRawBlockNotifier::NotifyBlock(const CBlockIndex *pindex) @@ -204,7 +214,7 @@ bool CZMQPublishRawBlockNotifier::NotifyBlock(const CBlockIndex *pindex) ss << block; } - return SendMessage(MSG_RAWBLOCK, &(*ss.begin()), ss.size()); + return SendZmqMessage(MSG_RAWBLOCK, &(*ss.begin()), ss.size()); } bool CZMQPublishRawTransactionNotifier::NotifyTransaction(const CTransaction &transaction) @@ -213,5 +223,5 @@ bool CZMQPublishRawTransactionNotifier::NotifyTransaction(const CTransaction &tr LogPrint(BCLog::ZMQ, "zmq: Publish rawtx %s\n", hash.GetHex()); CDataStream ss(SER_NETWORK, PROTOCOL_VERSION | RPCSerializationFlags()); ss << transaction; - return SendMessage(MSG_RAWTX, &(*ss.begin()), ss.size()); + return SendZmqMessage(MSG_RAWTX, &(*ss.begin()), ss.size()); } diff --git a/src/zmq/zmqpublishnotifier.h b/src/zmq/zmqpublishnotifier.h index 278fdb94d2..eb9ae881be 100644 --- a/src/zmq/zmqpublishnotifier.h +++ b/src/zmq/zmqpublishnotifier.h @@ -22,7 +22,7 @@ public: * data * message sequence number */ - bool SendMessage(const char *command, const void* data, size_t size); + bool SendZmqMessage(const char *command, const void* data, size_t size); bool Initialize(void *pcontext) override; void Shutdown() override; diff --git a/src/zmq/zmqutil.cpp b/src/zmq/zmqutil.cpp new file mode 100644 index 0000000000..f07a4ae9fd --- /dev/null +++ b/src/zmq/zmqutil.cpp @@ -0,0 +1,14 @@ +// Copyright (c) 2014-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. + +#include <zmq/zmqutil.h> + +#include <logging.h> + +#include <zmq.h> + +void zmqError(const char* str) +{ + LogPrint(BCLog::ZMQ, "zmq: Error: %s, errno=%s\n", str, zmq_strerror(errno)); +} diff --git a/src/zmq/zmqutil.h b/src/zmq/zmqutil.h new file mode 100644 index 0000000000..4c1df5d6db --- /dev/null +++ b/src/zmq/zmqutil.h @@ -0,0 +1,10 @@ +// Copyright (c) 2014-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_ZMQ_ZMQUTIL_H +#define BITCOIN_ZMQ_ZMQUTIL_H + +void zmqError(const char* str); + +#endif // BITCOIN_ZMQ_ZMQUTIL_H |