From 618e01eba39ffac0c4425ac193d8a4410201d856 Mon Sep 17 00:00:00 2001
From: Jacek Sieka <jacek@status.im>
Date: Mon, 11 May 2020 21:05:24 +0200
Subject: [PATCH] don't crash on double peerinfo close (#167)

---
 libp2p/connection.nim | 36 +++++++++++++-----------------------
 libp2p/peerinfo.nim   | 32 ++++++++++++++++++--------------
 2 files changed, 31 insertions(+), 37 deletions(-)

diff --git a/libp2p/connection.nim b/libp2p/connection.nim
index f6230921d..0590111a1 100644
--- a/libp2p/connection.nim
+++ b/libp2p/connection.nim
@@ -13,9 +13,7 @@ import peerinfo,
        errors,
        multiaddress,
        stream/lpstream,
-       peerinfo,
-       varint,
-       vbuffer
+       peerinfo
 
 export lpstream
 
@@ -64,18 +62,20 @@ proc setupConnectionTracker(): ConnectionTracker =
 
 declareGauge libp2p_open_connection, "open Connection instances"
 
+proc `$`*(conn: Connection): string =
+  if not isNil(conn.peerInfo):
+    result = $(conn.peerInfo)
+
 proc bindStreamClose(conn: Connection) {.async.} =
   # bind stream's close event to connection's close
   # to ensure correct close propagation
   if not isNil(conn.stream.closeEvent):
     await conn.stream.closeEvent.wait()
-    trace "wrapped stream closed, about to close conn", closed = conn.isClosed,
-                                                        peer = if not isNil(conn.peerInfo):
-                                                          conn.peerInfo.id else: ""
+    trace "wrapped stream closed, about to close conn",
+      closed = conn.isClosed, conn = $conn
     if not conn.isClosed:
-      trace "wrapped stream closed, closing conn", closed = conn.isClosed,
-                                                    peer = if not isNil(conn.peerInfo):
-                                                      conn.peerInfo.id else: ""
+      trace "wrapped stream closed, closing conn",
+        closed = conn.isClosed, conn = $conn
       await conn.close()
 
 proc init[T: Connection](self: var T, stream: LPStream): T =
@@ -119,36 +119,26 @@ method closed*(s: Connection): bool =
   result = s.stream.closed
 
 method close*(s: Connection) {.async, gcsafe.} =
-  trace "about to close connection", closed = s.closed,
-                                     peer = if not isNil(s.peerInfo):
-                                       s.peerInfo.id else: ""
+  trace "about to close connection", closed = s.closed, conn = $s
 
   if not s.isClosed:
     s.isClosed = true
     inc getConnectionTracker().closed
 
     if not isNil(s.stream) and not s.stream.closed:
-      trace "closing child stream", closed = s.closed,
-                                    peer = if not isNil(s.peerInfo):
-                                      s.peerInfo.id else: ""
+      trace "closing child stream", closed = s.closed, conn = $s
       await s.stream.close()
 
     s.closeEvent.fire()
 
-    trace "waiting readloops", count=s.readLoops.len
+    trace "waiting readloops", count=s.readLoops.len, conn = $s
     let loopFuts = await allFinished(s.readLoops)
     checkFutures(loopFuts)
     s.readLoops = @[]
 
-    trace "connection closed", closed = s.closed,
-                               peer = if not isNil(s.peerInfo):
-                                 s.peerInfo.id else: ""
+    trace "connection closed", closed = s.closed, conn = $s
     libp2p_open_connection.dec()
 
 method getObservedAddrs*(c: Connection): Future[MultiAddress] {.base, async, gcsafe.} =
   ## get resolved multiaddresses for the connection
   result = c.observedAddrs
-
-proc `$`*(conn: Connection): string =
-  if not isNil(conn.peerInfo):
-    result = $(conn.peerInfo)
diff --git a/libp2p/peerinfo.nim b/libp2p/peerinfo.nim
index 996f21f20..bad3f4466 100644
--- a/libp2p/peerinfo.nim
+++ b/libp2p/peerinfo.nim
@@ -8,7 +8,7 @@
 ## those terms.
 
 import options
-import chronos
+import chronos, chronicles
 import peer, multiaddress, crypto/crypto
 
 ## A peer can be constructed in one of tree ways:
@@ -35,6 +35,18 @@ type
     of HasPublic:
       key: Option[PublicKey]
 
+proc id*(p: PeerInfo): string =
+  p.peerId.pretty()
+
+proc `$`*(p: PeerInfo): string =
+  result.add(p.id)
+
+  result.add(" ")
+  result.add($p.addrs)
+
+  result.add(" ")
+  result.add($p.protocols)
+
 template postInit(peerinfo: PeerInfo,
                   addrs: openarray[MultiAddress],
                   protocols: openarray[string]) =
@@ -71,7 +83,11 @@ proc init*(p: typedesc[PeerInfo], key: PublicKey,
   result.postInit(addrs, protocols)
 
 proc close*(p: PeerInfo) {.inline.} =
-  p.lifefut.complete()
+  if not p.lifefut.finished:
+    p.lifefut.complete()
+  else:
+    # TODO this should ideally not happen
+    notice "Closing closed peer", peer = p.id
 
 proc join*(p: PeerInfo): Future[void] {.inline.} =
   var retFuture = newFuture[void]()
@@ -103,15 +119,3 @@ proc publicKey*(p: PeerInfo): Option[PublicKey] {.inline.} =
       result = p.key
   else:
     result = some(p.privateKey.getKey())
-
-proc id*(p: PeerInfo): string {.inline.} =
-  p.peerId.pretty()
-
-proc `$`*(p: PeerInfo): string =
-  result.add(p.id)
-
-  result.add(" ")
-  result.add($p.addrs)
-
-  result.add(" ")
-  result.add($p.protocols)