From 505ced5cc825b910d9f52bb29e6f2d3acaee5a5d Mon Sep 17 00:00:00 2001
From: Matthias Radestock <matthias.radestock@gmail.com>
Date: Mon, 11 May 2015 09:36:19 +0100
Subject: [PATCH] move localRefCount accounting responsibility from Peer to
 Peers

This makes for a more symmetric API - Peers.FetchWithDefault does the
increment and Peers.Dereference does the decrement, whereas previously
the latter was done by Peer.DecrementLocalRefCount.

This does increase lock granularity, i.e. modifying any ref count now
requires a write lock on Peers.
---
 router/connection.go  |  2 +-
 router/gossip_test.go |  4 ++--
 router/mocks_test.go  |  2 +-
 router/peer.go        | 20 +-------------------
 router/peers.go       | 36 ++++++++++++++----------------------
 5 files changed, 19 insertions(+), 45 deletions(-)

diff --git a/router/connection.go b/router/connection.go
index 891572a5..7e380730 100644
--- a/router/connection.go
+++ b/router/connection.go
@@ -412,7 +412,7 @@ func (conn *LocalConnection) shutdown(err error) {
 	}
 
 	if conn.remote != nil {
-		conn.remote.DecrementLocalRefCount()
+		conn.Router.Peers.Dereference(conn.remote)
 		conn.Router.Ourself.DeleteConnection(conn)
 	}
 
diff --git a/router/gossip_test.go b/router/gossip_test.go
index fc727d1e..09240735 100644
--- a/router/gossip_test.go
+++ b/router/gossip_test.go
@@ -55,8 +55,8 @@ func (router *Router) DeleteTestChannelConnection(r *Router) {
 	fromPeer, _ := r.Peers.Fetch(fromName)
 	toPeer, _ := router.Peers.Fetch(toName)
 
-	fromPeer.DecrementLocalRefCount()
-	toPeer.DecrementLocalRefCount()
+	r.Peers.Dereference(fromPeer)
+	router.Peers.Dereference(toPeer)
 
 	conn, _ := router.Ourself.ConnectionTo(toName)
 	router.Ourself.handleDeleteConnection(conn)
diff --git a/router/mocks_test.go b/router/mocks_test.go
index 7168034a..674eaa3a 100644
--- a/router/mocks_test.go
+++ b/router/mocks_test.go
@@ -33,7 +33,7 @@ func (peers *Peers) AddTestRemoteConnection(p1, p2 *Peer) {
 func (peers *Peers) DeleteTestConnection(p *Peer) {
 	toName := p.Name
 	toPeer, _ := peers.Fetch(toName)
-	toPeer.DecrementLocalRefCount()
+	peers.Dereference(toPeer)
 	conn, _ := peers.ourself.ConnectionTo(toName)
 	peers.ourself.deleteConnection(conn)
 }
diff --git a/router/peer.go b/router/peer.go
index a497a9a6..50b55e9a 100644
--- a/router/peer.go
+++ b/router/peer.go
@@ -13,7 +13,7 @@ type Peer struct {
 	NickName      string
 	UID           uint64
 	version       uint64
-	localRefCount uint64
+	localRefCount uint64 // maintained by Peers
 	connections   map[PeerName]Connection
 }
 
@@ -48,24 +48,6 @@ func (peer *Peer) Version() uint64 {
 	return peer.version
 }
 
-func (peer *Peer) IncrementLocalRefCount() {
-	peer.Lock()
-	defer peer.Unlock()
-	peer.localRefCount++
-}
-
-func (peer *Peer) DecrementLocalRefCount() {
-	peer.Lock()
-	defer peer.Unlock()
-	peer.localRefCount--
-}
-
-func (peer *Peer) IsLocallyReferenced() bool {
-	peer.RLock()
-	defer peer.RUnlock()
-	return peer.localRefCount != 0
-}
-
 func (peer *Peer) ConnectionTo(name PeerName) (Connection, bool) {
 	peer.RLock()
 	defer peer.RUnlock()
diff --git a/router/peers.go b/router/peers.go
index fff0cd84..db7577c5 100644
--- a/router/peers.go
+++ b/router/peers.go
@@ -47,20 +47,17 @@ func NewPeers(ourself *LocalPeer, onGC func(*Peer)) *Peers {
 }
 
 func (peers *Peers) FetchWithDefault(peer *Peer) *Peer {
-	peers.RLock()
-	res, found := peers.fetchAlias(peer)
-	peers.RUnlock()
-	if found {
-		return res
-	}
 	peers.Lock()
 	defer peers.Unlock()
-	res, found = peers.fetchAlias(peer)
-	if found {
-		return res
+	if existingPeer, found := peers.table[peer.Name]; found {
+		if existingPeer.UID != peer.UID {
+			return nil
+		}
+		existingPeer.localRefCount++
+		return existingPeer
 	}
 	peers.table[peer.Name] = peer
-	peer.IncrementLocalRefCount()
+	peer.localRefCount++
 	return peer
 }
 
@@ -71,6 +68,12 @@ func (peers *Peers) Fetch(name PeerName) (*Peer, bool) {
 	return peer, found // GRRR, why can't I inline this!?
 }
 
+func (peers *Peers) Dereference(peer *Peer) {
+	peers.Lock()
+	defer peers.Unlock()
+	peer.localRefCount--
+}
+
 func (peers *Peers) ForEach(fun func(*Peer)) {
 	peers.RLock()
 	defer peers.RUnlock()
@@ -162,22 +165,11 @@ func (peers *Peers) String() string {
 	return buf.String()
 }
 
-func (peers *Peers) fetchAlias(peer *Peer) (*Peer, bool) {
-	if existingPeer, found := peers.table[peer.Name]; found {
-		if existingPeer.UID != peer.UID {
-			return nil, true
-		}
-		existingPeer.IncrementLocalRefCount()
-		return existingPeer, true
-	}
-	return nil, false
-}
-
 func (peers *Peers) garbageCollect() []*Peer {
 	removed := []*Peer{}
 	_, reached := peers.ourself.Routes(nil, false)
 	for name, peer := range peers.table {
-		if _, found := reached[peer.Name]; !found && !peer.IsLocallyReferenced() {
+		if _, found := reached[peer.Name]; !found && peer.localRefCount == 0 {
 			delete(peers.table, name)
 			peers.onGC(peer)
 			removed = append(removed, peer)