From 84b19f04071b405d9a481fda5eca8cf0f16eec99 Mon Sep 17 00:00:00 2001 From: Daniel Hams Date: Mon, 17 Feb 2014 15:21:32 +0000 Subject: [PATCH] Change use of raw new to std::unique_ptr and fix a shutdown race condition discovered using atomics. --- alethzero/MainWin.cpp | 7 +++---- alethzero/MainWin.h | 6 +++--- libethereum/Client.cpp | 28 +++++++++++++++------------- libethereum/Client.h | 18 +++++++++++++----- libethereum/UPnP.cpp | 14 +++++++------- libethereum/UPnP.h | 4 ++-- 6 files changed, 43 insertions(+), 34 deletions(-) diff --git a/alethzero/MainWin.cpp b/alethzero/MainWin.cpp index e6140c3cb..12de0ac2a 100644 --- a/alethzero/MainWin.cpp +++ b/alethzero/MainWin.cpp @@ -25,13 +25,13 @@ Main::Main(QWidget *parent) : setWindowFlags(Qt::Window); ui->setupUi(this); g_logPost = [=](std::string const& s, char const* c) { simpleDebugOut(s, c); ui->log->addItem(QString::fromStdString(s)); }; - m_client = new Client("AlethZero"); + m_client.reset(new Client("AlethZero")); readSettings(); refresh(); - m_refresh = new QTimer(this); - connect(m_refresh, SIGNAL(timeout()), SLOT(refresh())); + m_refresh.reset(new QTimer(this)); + connect(m_refresh.get(), SIGNAL(timeout()), SLOT(refresh())); m_refresh->start(1000); #if ETH_DEBUG @@ -59,7 +59,6 @@ Main::~Main() { g_logPost = simpleDebugOut; writeSettings(); - delete ui; } void Main::on_about_triggered() diff --git a/alethzero/MainWin.h b/alethzero/MainWin.h index 0a76c6c75..85006350e 100644 --- a/alethzero/MainWin.h +++ b/alethzero/MainWin.h @@ -52,13 +52,13 @@ private: eth::u256 total() const; eth::u256 value() const; - Ui::Main *ui; + std::unique_ptr ui; - eth::Client* m_client; + std::unique_ptr m_client; QByteArray m_peers; QMutex m_guiLock; - QTimer* m_refresh; + std::unique_ptr m_refresh; QStringList m_servers; QVector m_myKeys; QStringList m_data; diff --git a/libethereum/Client.cpp b/libethereum/Client.cpp index dc0f78803..688625cca 100644 --- a/libethereum/Client.cpp +++ b/libethereum/Client.cpp @@ -33,7 +33,8 @@ Client::Client(std::string const& _clientVersion, Address _us, std::string const m_bc(_dbPath), m_stateDB(State::openDB(_dbPath)), m_s(_us, m_stateDB), - m_mined(_us, m_stateDB) + m_mined(_us, m_stateDB), + m_workState(Active) { Defaults::setDBPath(_dbPath); @@ -47,26 +48,28 @@ Client::Client(std::string const& _clientVersion, Address _us, std::string const static const char* c_threadName = "eth"; - m_work = new thread([&](){ + m_work.reset(new thread([&](){ setThreadName(c_threadName); - - while (m_workState != Deleting) work(); m_workState = Deleted; - }); + while (m_workState.load(std::memory_order_acquire) != Deleting) + work(); + m_workState.store(Deleted, std::memory_order_release); + })); } Client::~Client() { - if (m_workState == Active) - m_workState = Deleting; - while (m_workState != Deleted) + if (m_workState.load(std::memory_order_acquire) == Active) + m_workState.store(Deleting, std::memory_order_release); + while (m_workState.load(std::memory_order_acquire) != Deleted) this_thread::sleep_for(chrono::milliseconds(10)); + m_work->join(); } void Client::startNetwork(ushort _listenPort, std::string const& _seedHost, ushort _port, NodeMode _mode, unsigned _peers, string const& _publicIP, bool _upnp) { - if (m_net) + if (m_net.get()) return; - m_net = new PeerServer(m_clientVersion, m_bc, 0, _listenPort, _mode, _publicIP, _upnp); + m_net.reset(new PeerServer(m_clientVersion, m_bc, 0, _listenPort, _mode, _publicIP, _upnp)); m_net->setIdealPeerCount(_peers); if (_seedHost.size()) connect(_seedHost, _port); @@ -74,15 +77,14 @@ void Client::startNetwork(ushort _listenPort, std::string const& _seedHost, usho void Client::connect(std::string const& _seedHost, ushort _port) { - if (!m_net) + if (!m_net.get()) return; m_net->connect(_seedHost, _port); } void Client::stopNetwork() { - delete m_net; - m_net = nullptr; + m_net.reset(nullptr); } void Client::startMining() diff --git a/libethereum/Client.h b/libethereum/Client.h index 2ed0e4a50..8b7b35796 100644 --- a/libethereum/Client.h +++ b/libethereum/Client.h @@ -23,6 +23,7 @@ #include #include +#include #include "Common.h" #include "BlockChain.h" #include "TransactionQueue.h" @@ -52,6 +53,13 @@ private: Client* m_client; }; +enum ClientWorkState +{ + Active = 0, + Deleting, + Deleted +}; + class Client { public: @@ -112,7 +120,7 @@ public: /// Stop the network subsystem. void stopNetwork(); /// Get access to the peer server object. This will be null if the network isn't online. - PeerServer* peerServer() const { return m_net; } + PeerServer* peerServer() const { return m_net.get(); } // Mining stuff: @@ -136,12 +144,12 @@ private: Overlay m_stateDB; ///< Acts as the central point for the state database, so multiple States can share it. State m_s; ///< The present state of the client. State m_mined; ///< The state of the client which we're mining (i.e. it'll have all the rewards added). - PeerServer* m_net = nullptr; ///< Should run in background and send us events when blocks found and allow us to send blocks as required. - - std::thread* m_work; ///< The work thread. + std::unique_ptr m_net; ///< Should run in background and send us events when blocks found and allow us to send blocks as required. + + std::unique_ptr m_work; ///< The work thread. std::mutex m_lock; - enum { Active = 0, Deleting, Deleted } m_workState = Active; + std::atomic m_workState; bool m_doMine = false; ///< Are we supposed to be mining? MineProgress m_mineProgress; mutable bool m_miningStarted = false; diff --git a/libethereum/UPnP.cpp b/libethereum/UPnP.cpp index 9f6ed4e95..6008bc7de 100644 --- a/libethereum/UPnP.cpp +++ b/libethereum/UPnP.cpp @@ -33,8 +33,8 @@ using namespace eth; UPnP::UPnP() { - m_urls = new UPNPUrls; - m_data = new IGDdatas; + m_urls.reset(new UPNPUrls); + m_data.reset(new IGDdatas); m_ok = false; @@ -43,8 +43,8 @@ UPnP::UPnP() char* descXML; int descXMLsize = 0; int upnperror = 0; - memset(m_urls, 0, sizeof(struct UPNPUrls)); - memset(m_data, 0, sizeof(struct IGDdatas)); + memset(m_urls.get(), 0, sizeof(struct UPNPUrls)); + memset(m_data.get(), 0, sizeof(struct IGDdatas)); devlist = upnpDiscover(2000, NULL/*multicast interface*/, NULL/*minissdpd socket path*/, 0/*sameport*/, 0/*ipv6*/, &upnperror); if (devlist) { @@ -66,12 +66,12 @@ UPnP::UPnP() #endif if (descXML) { - parserootdesc (descXML, descXMLsize, m_data); + parserootdesc (descXML, descXMLsize, m_data.get()); free (descXML); descXML = 0; #if MINIUPNPC_API_VERSION >= 9 - GetUPNPUrls (m_urls, m_data, dev->descURL, 0); + GetUPNPUrls (m_urls.get(), m_data.get(), dev->descURL, 0); #else - GetUPNPUrls (m_urls, m_data, dev->descURL); + GetUPNPUrls (m_urls.get(), m_data.get(), dev->descURL); #endif m_ok = true; } diff --git a/libethereum/UPnP.h b/libethereum/UPnP.h index bb51c96d1..8713e1d2b 100644 --- a/libethereum/UPnP.h +++ b/libethereum/UPnP.h @@ -45,8 +45,8 @@ public: std::set m_reg; bool m_ok; - struct UPNPUrls* m_urls; - struct IGDdatas* m_data; + std::unique_ptr m_urls; + std::unique_ptr m_data; }; }