From de4e29288d30183ca78a5e0878431ed47fa58b8f Mon Sep 17 00:00:00 2001 From: Adam Ierymenko Date: Thu, 21 Aug 2014 17:49:05 -0700 Subject: Fix for crazy Windows threading bug... repeatedly adding and removing a network now doesn't leave networks in limbo. --- node/Network.cpp | 68 +++++++++++++++++++++++++++++++---------------------- node/Network.hpp | 16 ++++++++----- node/NodeConfig.cpp | 2 +- node/Thread.hpp | 15 +++++++++--- 4 files changed, 63 insertions(+), 38 deletions(-) (limited to 'node') diff --git a/node/Network.cpp b/node/Network.cpp index b38e9692..b3f065ce 100644 --- a/node/Network.cpp +++ b/node/Network.cpp @@ -66,10 +66,13 @@ Network::~Network() { Thread::join(_setupThread); - if (_tap) - _r->tapFactory->close(_tap,_destroyOnDelete); + { + Mutex::Lock _l(_lock); + if (_tap) + _r->tapFactory->close(_tap,_destroyed); + } - if (_destroyOnDelete) { + if (_destroyed) { Utils::rm(std::string(_r->homePath + ZT_PATH_SEPARATOR_S + "networks.d" + ZT_PATH_SEPARATOR_S + idString() + ".conf")); Utils::rm(std::string(_r->homePath + ZT_PATH_SEPARATOR_S + "networks.d" + ZT_PATH_SEPARATOR_S + idString() + ".mcerts")); } else { @@ -80,12 +83,6 @@ Network::~Network() SharedPtr Network::newInstance(const RuntimeEnvironment *renv,NodeConfig *nc,uint64_t id) { - /* We construct Network via a static method to ensure that it is immediately - * wrapped in a SharedPtr<>. Otherwise if there is traffic on the Ethernet - * tap device, a SharedPtr<> wrap can occur in the Ethernet frame handler - * that then causes the Network instance to be deleted before it is finished - * being constructed. C++ edge cases, how I love thee. */ - SharedPtr nw(new Network()); nw->_id = id; nw->_nc = nc; @@ -94,7 +91,7 @@ SharedPtr Network::newInstance(const RuntimeEnvironment *renv,NodeConfi nw->_tap = (EthernetTap *)0; nw->_enabled = true; nw->_lastConfigUpdate = 0; - nw->_destroyOnDelete = false; + nw->_destroyed = false; nw->_netconfFailure = NETCONF_FAILURE_NONE; if (nw->controller() == renv->identity.address()) // TODO: fix Switch to allow packets to self @@ -143,6 +140,9 @@ bool Network::setConfiguration(const Dictionary &conf,bool saveToDisk) { Mutex::Lock _l(_lock); + if (_destroyed) + return false; + try { SharedPtr newConfig(new NetworkConfig(conf)); // throws if invalid if ((newConfig->networkId() == _id)&&(newConfig->issuedTo() == _r->identity.address())) { @@ -242,6 +242,10 @@ bool Network::isAllowed(const Address &peer) const void Network::clean() { Mutex::Lock _l(_lock); + + if (_destroyed) + return; + uint64_t now = Utils::now(); if ((_config)&&(_config->isPublic())) { @@ -277,23 +281,17 @@ void Network::clean() Network::Status Network::status() const { Mutex::Lock _l(_lock); - if (_tap) { - switch(_netconfFailure) { - case NETCONF_FAILURE_ACCESS_DENIED: - return NETWORK_ACCESS_DENIED; - case NETCONF_FAILURE_NOT_FOUND: - return NETWORK_NOT_FOUND; - case NETCONF_FAILURE_NONE: - if (_lastConfigUpdate > 0) - return NETWORK_OK; - else return NETWORK_WAITING_FOR_FIRST_AUTOCONF; - case NETCONF_FAILURE_INIT_FAILED: - default: - return NETWORK_INITIALIZATION_FAILED; - } - } else if (_netconfFailure == NETCONF_FAILURE_INIT_FAILED) { - return NETWORK_INITIALIZATION_FAILED; - } else return NETWORK_INITIALIZING; + switch(_netconfFailure) { + case NETCONF_FAILURE_ACCESS_DENIED: + return NETWORK_ACCESS_DENIED; + case NETCONF_FAILURE_NOT_FOUND: + return NETWORK_NOT_FOUND; + case NETCONF_FAILURE_NONE: + return ((_lastConfigUpdate > 0) ? ((_tap) ? NETWORK_OK : NETWORK_INITIALIZING) : NETWORK_WAITING_FOR_FIRST_AUTOCONF); + //case NETCONF_FAILURE_INIT_FAILED: + default: + return NETWORK_INITIALIZATION_FAILED; + } } void Network::_CBhandleTapData(void *arg,const MAC &from,const MAC &to,unsigned int etherType,const Buffer<4096> &data) @@ -366,7 +364,7 @@ void Network::threadMain() { Mutex::Lock _l(_lock); if (_tap) // the tap creation thread can technically be re-launched, though this isn't done right now - _r->tapFactory->close(_tap,_destroyOnDelete); + _r->tapFactory->close(_tap,false); _tap = t; if (t) { if (_config) @@ -409,6 +407,20 @@ void Network::setEnabled(bool enabled) _tap->setEnabled(enabled); } +void Network::destroy() +{ + Mutex::Lock _l(_lock); + + _enabled = false; + _destroyed = true; + + Thread::join(_setupThread); + + if (_tap) + _r->tapFactory->close(_tap,true); + _tap = (EthernetTap *)0; +} + void Network::_restoreState() { Buffer buf; diff --git a/node/Network.hpp b/node/Network.hpp index 1d62bdf1..cab41411 100644 --- a/node/Network.hpp +++ b/node/Network.hpp @@ -99,11 +99,6 @@ private: */ static SharedPtr newInstance(const RuntimeEnvironment *renv,NodeConfig *nc,uint64_t id); - /** - * Causes all persistent disk presence to be erased on delete, and this network won't be reloaded on next startup - */ - inline void destroyOnDelete() throw() { _destroyOnDelete = true; } - public: /** * Broadcast multicast group: ff:ff:ff:ff:ff:ff / 0 @@ -419,6 +414,15 @@ public: */ void setEnabled(bool enabled); + /** + * Destroy this network + * + * This causes the network to disable itself, destroy its tap device, and on + * delete to delete all trace of itself on disk and remove any persistent tap + * device instances. Call this when a network is being removed from the system. + */ + void destroy(); + private: static void _CBhandleTapData(void *arg,const MAC &from,const MAC &to,unsigned int etherType,const Buffer<4096> &data); @@ -453,7 +457,7 @@ private: SharedPtr _config; volatile uint64_t _lastConfigUpdate; - volatile bool _destroyOnDelete; + volatile bool _destroyed; volatile enum { NETCONF_FAILURE_NONE, diff --git a/node/NodeConfig.cpp b/node/NodeConfig.cpp index 3b5ce2f6..732d41a3 100644 --- a/node/NodeConfig.cpp +++ b/node/NodeConfig.cpp @@ -286,7 +286,7 @@ void NodeConfig::_doCommand(IpcConnection *ipcc,const char *commandLine) if (nw == _networks.end()) { ipcc->printf("404 leave %.16llx ERROR: not a member of that network"ZT_EOL_S,(unsigned long long)nwid); } else { - nw->second->destroyOnDelete(); + nw->second->destroy(); _networks.erase(nw); } } else { diff --git a/node/Thread.hpp b/node/Thread.hpp index a796af33..e02cdde6 100644 --- a/node/Thread.hpp +++ b/node/Thread.hpp @@ -31,12 +31,13 @@ #include #include "Constants.hpp" -#include "AtomicCounter.hpp" #ifdef __WINDOWS__ +#include #include #include +#include "Mutex.hpp" namespace ZeroTier { @@ -56,6 +57,7 @@ public: throw() { _th = NULL; + _tid = 0; } template @@ -71,8 +73,15 @@ public: static inline void join(const Thread &t) { - if (t._th != NULL) - WaitForSingleObject(t._th,INFINITE); + if (t._th != NULL) { + for(;;) { + DWORD ec = STILL_ACTIVE; + GetExitCodeThread(t._th,&ec); + if (ec == STILL_ACTIVE) + WaitForSingleObject(t._th,1000); + else break; + } + } } static inline void sleep(unsigned long ms) -- cgit v1.2.3