From 9e7c778cc8ebdfd5d3f773a7d3cb30ad154e7189 Mon Sep 17 00:00:00 2001 From: Adam Ierymenko Date: Fri, 27 Jan 2017 16:16:06 -0800 Subject: Fix deadlock. --- node/Node.cpp | 13 +++++++++---- node/Topology.cpp | 31 ++++++++++++++++++------------- node/Topology.hpp | 39 +++++++++++++++++---------------------- 3 files changed, 44 insertions(+), 39 deletions(-) (limited to 'node') diff --git a/node/Node.cpp b/node/Node.cpp index c4a40395..3d5b5c3d 100644 --- a/node/Node.cpp +++ b/node/Node.cpp @@ -159,7 +159,8 @@ public: _PingPeersThatNeedPing(const RuntimeEnvironment *renv,uint64_t now) : lastReceiveFromUpstream(0), RR(renv), - _now(now) + _now(now), + _bestCurrentUpstream(RR->topology->getUpstreamPeer()) { RR->topology->getUpstreamStableEndpoints(_upstreams); } @@ -194,8 +195,11 @@ public: } } else contacted = true; - if (!contacted) - p->sendHELLO(InetAddress(),InetAddress(),_now); + if ((!contacted)&&(_bestCurrentUpstream)) { + const SharedPtr up(_bestCurrentUpstream->getBestPath(_now,true)); + if (up) + p->sendHELLO(up->localAddress(),up->address(),_now); + } lastReceiveFromUpstream = std::max(p->lastReceive(),lastReceiveFromUpstream); } else if (p->isActive(_now)) { @@ -205,7 +209,8 @@ public: private: const RuntimeEnvironment *RR; - uint64_t _now; + const uint64_t _now; + const SharedPtr _bestCurrentUpstream; Hashtable< Address,std::vector > _upstreams; }; diff --git a/node/Topology.cpp b/node/Topology.cpp index ece93ee6..5632c337 100644 --- a/node/Topology.cpp +++ b/node/Topology.cpp @@ -79,7 +79,7 @@ SharedPtr Topology::addPeer(const SharedPtr &peer) SharedPtr np; { - Mutex::Lock _l(_lock); + Mutex::Lock _l(_peers_m); SharedPtr &hp = _peers[peer->address()]; if (!hp) hp = peer; @@ -99,7 +99,7 @@ SharedPtr Topology::getPeer(const Address &zta) } { - Mutex::Lock _l(_lock); + Mutex::Lock _l(_peers_m); const SharedPtr *const ap = _peers.get(zta); if (ap) return *ap; @@ -110,7 +110,7 @@ SharedPtr Topology::getPeer(const Address &zta) if (id) { SharedPtr np(new Peer(RR,RR->identity,id)); { - Mutex::Lock _l(_lock); + Mutex::Lock _l(_peers_m); SharedPtr &ap = _peers[zta]; if (!ap) ap.swap(np); @@ -127,7 +127,7 @@ Identity Topology::getIdentity(const Address &zta) if (zta == RR->identity.address()) { return RR->identity; } else { - Mutex::Lock _l(_lock); + Mutex::Lock _l(_peers_m); const SharedPtr *const ap = _peers.get(zta); if (ap) return (*ap)->identity(); @@ -147,7 +147,8 @@ void Topology::saveIdentity(const Identity &id) SharedPtr Topology::getUpstreamPeer(const Address *avoid,unsigned int avoidCount,bool strictAvoid) { const uint64_t now = RR->node->now(); - Mutex::Lock _l(_lock); + Mutex::Lock _l1(_peers_m); + Mutex::Lock _l2(_upstreams_m); if (_amRoot) { /* If I am a root, pick another root that isn't mine and that @@ -208,13 +209,13 @@ SharedPtr Topology::getUpstreamPeer(const Address *avoid,unsigned int avoi bool Topology::isUpstream(const Identity &id) const { - Mutex::Lock _l(_lock); + Mutex::Lock _l(_upstreams_m); return (std::find(_upstreamAddresses.begin(),_upstreamAddresses.end(),id.address()) != _upstreamAddresses.end()); } ZT_PeerRole Topology::role(const Address &ztaddr) const { - Mutex::Lock _l(_lock); + Mutex::Lock _l(_upstreams_m); if (std::find(_upstreamAddresses.begin(),_upstreamAddresses.end(),ztaddr) != _upstreamAddresses.end()) { for(std::vector::const_iterator i(_planet.roots().begin());i!=_planet.roots().end();++i) { if (i->identity.address() == ztaddr) @@ -227,7 +228,7 @@ ZT_PeerRole Topology::role(const Address &ztaddr) const bool Topology::isProhibitedEndpoint(const Address &ztaddr,const InetAddress &ipaddr) const { - Mutex::Lock _l(_lock); + Mutex::Lock _l(_upstreams_m); // For roots the only permitted addresses are those defined. This adds just a little // bit of extra security against spoofing, replaying, etc. @@ -257,7 +258,8 @@ bool Topology::addWorld(const World &newWorld) if ((newWorld.type() != World::TYPE_PLANET)&&(newWorld.type() != World::TYPE_MOON)) return false; - Mutex::Lock _l(_lock); + Mutex::Lock _l1(_upstreams_m); + Mutex::Lock _l2(_peers_m); World *existing = (World *)0; switch(newWorld.type()) { @@ -313,7 +315,7 @@ void Topology::addMoon(const uint64_t id) { { const Address a(id >> 24); - Mutex::Lock _l(_lock); + Mutex::Lock _l(_upstreams_m); if (std::find(_contactingMoons.begin(),_contactingMoons.end(),a) == _contactingMoons.end()) _contactingMoons.push_back(a); } @@ -339,7 +341,8 @@ void Topology::addMoon(const uint64_t id) void Topology::removeMoon(const uint64_t id) { - Mutex::Lock _l(_lock); + Mutex::Lock _l1(_upstreams_m); + Mutex::Lock _l2(_peers_m); std::vector nm; for(std::vector::const_iterator m(_moons.begin());m!=_moons.end();++m) { @@ -365,8 +368,9 @@ void Topology::removeMoon(const uint64_t id) void Topology::clean(uint64_t now) { - Mutex::Lock _l(_lock); { + Mutex::Lock _l1(_peers_m); + Mutex::Lock _l2(_upstreams_m); Hashtable< Address,SharedPtr >::Iterator i(_peers); Address *a = (Address *)0; SharedPtr *p = (SharedPtr *)0; @@ -376,6 +380,7 @@ void Topology::clean(uint64_t now) } } { + Mutex::Lock _l(_paths_m); Hashtable< Path::HashKey,SharedPtr >::Iterator i(_paths); Path::HashKey *k = (Path::HashKey *)0; SharedPtr *p = (SharedPtr *)0; @@ -401,7 +406,7 @@ Identity Topology::_getIdentity(const Address &zta) void Topology::_memoizeUpstreams() { - // assumes _lock is locked + // assumes _upstreams_m and _peers_m are locked _upstreamAddresses.clear(); _amRoot = false; for(std::vector::const_iterator i(_planet.roots().begin());i!=_planet.roots().end();++i) { diff --git a/node/Topology.hpp b/node/Topology.hpp index e8efe0db..78dc0fe8 100644 --- a/node/Topology.hpp +++ b/node/Topology.hpp @@ -82,7 +82,7 @@ public: */ inline SharedPtr getPeerNoCache(const Address &zta) { - Mutex::Lock _l(_lock); + Mutex::Lock _l(_peers_m); const SharedPtr *const ap = _peers.get(zta); if (ap) return *ap; @@ -98,7 +98,7 @@ public: */ inline SharedPtr getPath(const InetAddress &l,const InetAddress &r) { - Mutex::Lock _l(_lock); + Mutex::Lock _l(_paths_m); SharedPtr &p = _paths[Path::HashKey(l,r)]; if (!p) p.setToUnsafe(new Path(l,r)); @@ -178,7 +178,7 @@ public: */ inline void getUpstreamStableEndpoints(Hashtable< Address,std::vector > &eps) const { - Mutex::Lock _l(_lock); + Mutex::Lock _l(_upstreams_m); for(std::vector::const_iterator i(_planet.roots().begin());i!=_planet.roots().end();++i) { std::vector &ips = eps[i->identity.address()]; for(std::vector::const_iterator j(i->stableEndpoints.begin());j!=i->stableEndpoints.end();++j) { @@ -204,7 +204,7 @@ public: */ inline std::vector
upstreamAddresses() const { - Mutex::Lock _l(_lock); + Mutex::Lock _l(_upstreams_m); std::vector
u(_upstreamAddresses); for(std::vector
::const_iterator m(_contactingMoons.begin());m!=_contactingMoons.end();++m) { if (std::find(u.begin(),u.end(),*m) == u.end()) @@ -218,7 +218,7 @@ public: */ inline std::vector moons() const { - Mutex::Lock _l(_lock); + Mutex::Lock _l(_upstreams_m); return _moons; } @@ -227,7 +227,7 @@ public: */ inline World planet() const { - Mutex::Lock _l(_lock); + Mutex::Lock _l(_upstreams_m); return _planet; } @@ -286,7 +286,7 @@ public: inline unsigned long countActive(uint64_t now) const { unsigned long cnt = 0; - Mutex::Lock _l(_lock); + Mutex::Lock _l(_peers_m); Hashtable< Address,SharedPtr >::Iterator i(const_cast(this)->_peers); Address *a = (Address *)0; SharedPtr *p = (SharedPtr *)0; @@ -299,20 +299,13 @@ public: /** * Apply a function or function object to all peers * - * Note: explicitly template this by reference if you want the object - * passed by reference instead of copied. - * - * Warning: be careful not to use features in these that call any other - * methods of Topology that may lock _lock, otherwise a recursive lock - * and deadlock or lock corruption may occur. - * * @param f Function to apply * @tparam F Function or function object type */ template inline void eachPeer(F f) { - Mutex::Lock _l(_lock); + Mutex::Lock _l(_peers_m); Hashtable< Address,SharedPtr >::Iterator i(_peers); Address *a = (Address *)0; SharedPtr *p = (SharedPtr *)0; @@ -332,7 +325,7 @@ public: */ inline std::vector< std::pair< Address,SharedPtr > > allPeers() const { - Mutex::Lock _l(_lock); + Mutex::Lock _l(_peers_m); return _peers.entries(); } @@ -382,7 +375,7 @@ public: { if (count > ZT_MAX_TRUSTED_PATHS) count = ZT_MAX_TRUSTED_PATHS; - Mutex::Lock _l(_lock); + Mutex::Lock _l(_trustedPaths_m); for(unsigned int i=0;i _moons; + Mutex _trustedPaths_m; Hashtable< Address,SharedPtr > _peers; + Mutex _peers_m; + Hashtable< Path::HashKey,SharedPtr > _paths; + Mutex _paths_m; + World _planet; + std::vector _moons; std::vector
_contactingMoons; std::vector
_upstreamAddresses; bool _amRoot; - - Mutex _lock; + Mutex _upstreams_m; // locks worlds, upstream info, moon info, etc. }; } // namespace ZeroTier -- cgit v1.2.3