Skip to content
This repository has been archived by the owner on Apr 21, 2022. It is now read-only.

Fix peer tagging #25

Closed
wants to merge 2 commits into from
Closed

Fix peer tagging #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
50c1048
Fix peer tag for a race condition
cannium Nov 29, 2018
7bbcb23
Fix test
cannium Nov 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions connmgr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,12 @@ func (cm *BasicConnMgr) TagPeer(p peer.ID, tag string, val int) {

pi, ok := cm.peers[p]
if !ok {
log.Info("tried to tag conn from untracked peer: ", p)
return
pi = &peerInfo{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, unfortunately, this will leak memory if we:

  1. Connect.
  2. Disconnect.
  3. Tag the peer.

A real fix will, unfortunately, require access to a Host and/or Network object (to check if we're currently connected. Unfortunately... that requires a much larger refactor (which is why I haven't bothered fixing this...).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But as long as Connect and Disconnect are paired, the calls from a rational caller to TagPeer and UntagPeer are paired. So maybe the situation is improved a little by this patch.

I'm not self-bloating, but you should really consider refactor the whole system like I said here: libp2p/go-libp2p#467 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But as long as Connect and Disconnect are paired, the calls from a rational caller to TagPeer and UntagPeer are paired. So maybe the situation is improved a little by this patch.

Not necessarily. Peers are sometimes tagged within Connect/Disconnect hooks but many services tag peers on an as-needed basis (and may never explicitly untag them). The expectation is that the connection manager will clean up any tags after a final disconnect.

firstSeen: time.Now(),
tags: make(map[string]int),
conns: make(map[inet.Conn]time.Time),
}
cm.peers[p] = pi
}

// Update the total value of the peer.
Expand Down
4 changes: 2 additions & 2 deletions connmgr_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,8 @@ func TestTagPeerNonExistant(t *testing.T) {
id := tu.RandPeerIDFatal(t)
cm.TagPeer(id, "test", 1)

if len(cm.peers) != 0 {
t.Fatal("expected zero peers")
if len(cm.peers) != 1 {
t.Fatal("expected 1 peer")
}
}

Expand Down