net: Drop CNodeRef for AttemptToEvictConnection

Locking for each operation here is unnecessary, and solves the wrong problem.
Additionally, it introduces a problem when cs_vNodes is held in an owning
class, to which invididual CNodeRefs won't have access.

These should be weak pointers anyway, once vNodes contain shared pointers.

Rather than using a refcounting class, use a 3-step process instead.

1. Lock vNodes long enough to snapshot the fields necessary for comparing
2. Unlock and do the comparison
3. Re-lock and mark the resulting node for disconnection if it still exists
This commit is contained in:
Cory Fields 2016-04-18 15:58:19 -04:00
parent 563f375cde
commit cca221fd21

View file

@ -794,51 +794,22 @@ void SocketSendData(CNode *pnode)
static std::list<CNode*> vNodesDisconnected;
class CNodeRef {
public:
CNodeRef(CNode *pnode) : _pnode(pnode) {
LOCK(cs_vNodes);
_pnode->AddRef();
}
~CNodeRef() {
LOCK(cs_vNodes);
_pnode->Release();
}
CNode& operator *() const {return *_pnode;};
CNode* operator ->() const {return _pnode;};
CNodeRef& operator =(const CNodeRef& other)
{
if (this != &other) {
LOCK(cs_vNodes);
_pnode->Release();
_pnode = other._pnode;
_pnode->AddRef();
}
return *this;
}
CNodeRef(const CNodeRef& other):
_pnode(other._pnode)
{
LOCK(cs_vNodes);
_pnode->AddRef();
}
private:
CNode *_pnode;
struct NodeEvictionCandidate
{
NodeId id;
int64_t nTimeConnected;
int64_t nMinPingUsecTime;
CAddress addr;
};
static bool ReverseCompareNodeMinPingTime(const CNodeRef &a, const CNodeRef &b)
static bool ReverseCompareNodeMinPingTime(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b)
{
return a->nMinPingUsecTime > b->nMinPingUsecTime;
return a.nMinPingUsecTime > b.nMinPingUsecTime;
}
static bool ReverseCompareNodeTimeConnected(const CNodeRef &a, const CNodeRef &b)
static bool ReverseCompareNodeTimeConnected(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b)
{
return a->nTimeConnected > b->nTimeConnected;
return a.nTimeConnected > b.nTimeConnected;
}
class CompareNetGroupKeyed
@ -851,14 +822,14 @@ public:
GetRandBytes(vchSecretKey.data(), vchSecretKey.size());
}
bool operator()(const CNodeRef &a, const CNodeRef &b)
bool operator()(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b)
{
std::vector<unsigned char> vchGroupA, vchGroupB;
CSHA256 hashA, hashB;
std::vector<unsigned char> vchA(32), vchB(32);
vchGroupA = a->addr.GetGroup();
vchGroupB = b->addr.GetGroup();
vchGroupA = a.addr.GetGroup();
vchGroupB = b.addr.GetGroup();
hashA.Write(begin_ptr(vchGroupA), vchGroupA.size());
hashB.Write(begin_ptr(vchGroupB), vchGroupB.size());
@ -882,7 +853,7 @@ public:
* simultaneously better at all of them than honest peers.
*/
static bool AttemptToEvictConnection(bool fPreferNewConnection) {
std::vector<CNodeRef> vEvictionCandidates;
std::vector<NodeEvictionCandidate> vEvictionCandidates;
{
LOCK(cs_vNodes);
@ -893,7 +864,8 @@ static bool AttemptToEvictConnection(bool fPreferNewConnection) {
continue;
if (node->fDisconnect)
continue;
vEvictionCandidates.push_back(CNodeRef(node));
NodeEvictionCandidate candidate = {node->id, node->nTimeConnected, node->nMinPingUsecTime, node->addr};
vEvictionCandidates.push_back(candidate);
}
}
@ -928,16 +900,16 @@ static bool AttemptToEvictConnection(bool fPreferNewConnection) {
std::vector<unsigned char> naMostConnections;
unsigned int nMostConnections = 0;
int64_t nMostConnectionsTime = 0;
std::map<std::vector<unsigned char>, std::vector<CNodeRef> > mapAddrCounts;
BOOST_FOREACH(const CNodeRef &node, vEvictionCandidates) {
mapAddrCounts[node->addr.GetGroup()].push_back(node);
int64_t grouptime = mapAddrCounts[node->addr.GetGroup()][0]->nTimeConnected;
size_t groupsize = mapAddrCounts[node->addr.GetGroup()].size();
std::map<std::vector<unsigned char>, std::vector<NodeEvictionCandidate> > mapAddrCounts;
BOOST_FOREACH(const NodeEvictionCandidate &node, vEvictionCandidates) {
mapAddrCounts[node.addr.GetGroup()].push_back(node);
int64_t grouptime = mapAddrCounts[node.addr.GetGroup()][0].nTimeConnected;
size_t groupsize = mapAddrCounts[node.addr.GetGroup()].size();
if (groupsize > nMostConnections || (groupsize == nMostConnections && grouptime > nMostConnectionsTime)) {
nMostConnections = groupsize;
nMostConnectionsTime = grouptime;
naMostConnections = node->addr.GetGroup();
naMostConnections = node.addr.GetGroup();
}
}
@ -952,9 +924,15 @@ static bool AttemptToEvictConnection(bool fPreferNewConnection) {
return false;
// Disconnect from the network group with the most connections
vEvictionCandidates[0]->fDisconnect = true;
return true;
NodeId evicted = vEvictionCandidates.front().id;
LOCK(cs_vNodes);
for(std::vector<CNode*>::const_iterator it(vNodes.begin()); it != vNodes.end(); ++it) {
if ((*it)->GetId() == evicted) {
(*it)->fDisconnect = true;
return true;
}
}
return false;
}
static void AcceptConnection(const ListenSocket& hListenSocket) {