From 97f4bdc260f4399a383d153d7a4979b97c9d61ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?rich=CE=9Brd?= Date: Mon, 13 Jan 2025 11:17:18 -0400 Subject: [PATCH] fix: code review --- examples/tutorial_4_gossipsub.nim | 7 ++----- libp2p/protocols/pubsub/floodsub.nim | 2 -- libp2p/protocols/pubsub/gossipsub.nim | 6 +++--- libp2p/protocols/pubsub/pubsub.nim | 7 ++----- libp2p/protocols/pubsub/pubsubpeer.nim | 17 +++++++---------- tests/pubsub/testgossipsub.nim | 7 +------ tests/pubsub/testgossipsub2.nim | 18 +++--------------- 7 files changed, 18 insertions(+), 46 deletions(-) diff --git a/examples/tutorial_4_gossipsub.nim b/examples/tutorial_4_gossipsub.nim index fa451cedb0..69caa709e1 100644 --- a/examples/tutorial_4_gossipsub.nim +++ b/examples/tutorial_4_gossipsub.nim @@ -95,11 +95,8 @@ proc oneNode(node: Node, rng: ref HmacDrbgContext) {.async.} = node.gossip.subscribe( "metrics", proc(topic: string, data: seq[byte]) {.async: (raises: []).} = - let m = MetricList.decode(data) - if m.isErr: - raiseAssert "failed to decode metric" - else: - echo m + let m = MetricList.decode(data).expect("metric can be decoded") + echo m , ) else: diff --git a/libp2p/protocols/pubsub/floodsub.nim b/libp2p/protocols/pubsub/floodsub.nim index 2ae5669035..f98068885c 100644 --- a/libp2p/protocols/pubsub/floodsub.nim +++ b/libp2p/protocols/pubsub/floodsub.nim @@ -188,8 +188,6 @@ method init*(f: FloodSub) = # This is top-level procedure which will work as separate task, so it # do not need to propagate CancelledError. trace "Unexpected cancellation in floodsub handler", conn - except CatchableError as exc: - trace "FloodSub handler leaks an error", description = exc.msg, conn f.handler = handler f.codec = FloodSubCodec diff --git a/libp2p/protocols/pubsub/gossipsub.nim b/libp2p/protocols/pubsub/gossipsub.nim index f730cebac6..d6e97804c8 100644 --- a/libp2p/protocols/pubsub/gossipsub.nim +++ b/libp2p/protocols/pubsub/gossipsub.nim @@ -219,8 +219,6 @@ method init*(g: GossipSub) = # This is top-level procedure which will work as separate task, so it # do not need to propogate CancelledError. trace "Unexpected cancellation in gossipsub handler", conn - except CatchableError as exc: - trace "GossipSub handler leaks an error", description = exc.msg, conn g.handler = handler g.codecs &= GossipSubCodec_12 @@ -490,7 +488,9 @@ proc validateAndRelay( ) await handleData(g, topic, msg.data) - except CatchableError as exc: + except CancelledError as exc: + info "validateAndRelay failed", description = exc.msg + except PeerRateLimitError as exc: info "validateAndRelay failed", description = exc.msg proc dataAndTopicsIdSize(msgs: seq[Message]): int = diff --git a/libp2p/protocols/pubsub/pubsub.nim b/libp2p/protocols/pubsub/pubsub.nim index d3cc962c7c..868285f926 100644 --- a/libp2p/protocols/pubsub/pubsub.nim +++ b/libp2p/protocols/pubsub/pubsub.nim @@ -602,12 +602,9 @@ method validate*( topic = topic, registered = toSeq(p.validators.keys) if topic in p.validators: trace "running validators for topic", topic = topic - try: - for validator in p.validators[topic]: + p.validators.withValue(topic, validators): + for validator in validators[]: pending.add(validator(topic, message)) - except KeyError: - raiseAssert "topic checked before" - var valResult = ValidationResult.Accept let futs = await allFinished(pending) for fut in futs: diff --git a/libp2p/protocols/pubsub/pubsubpeer.nim b/libp2p/protocols/pubsub/pubsubpeer.nim index 1c03865cda..a05bd5838e 100644 --- a/libp2p/protocols/pubsub/pubsubpeer.nim +++ b/libp2p/protocols/pubsub/pubsubpeer.nim @@ -81,9 +81,7 @@ type PubSubPeerEvent* = object kind*: PubSubPeerEventKind - GetConn* = proc(): Future[Connection] {. - gcsafe, async: (raises: [GetConnDialError]), raises: [] - .} + GetConn* = proc(): Future[Connection] {.gcsafe, async: (raises: [GetConnDialError]).} DropConn* = proc(peer: PubSubPeer) {.gcsafe, raises: [].} # have to pass peer as it's unknown during init OnEvent* = proc(peer: PubSubPeer, event: PubSubPeerEvent) {.gcsafe, raises: [].} @@ -211,7 +209,7 @@ proc handle*(p: PubSubPeer, conn: Connection) {.async: (raises: []).} = except PeerRateLimitError as exc: debug "Peer rate limit exceeded, exiting read while", conn, peer = p, description = exc.msg - except CatchableError as exc: + except LPStreamError as exc: debug "Exception occurred in PubSubPeer.handle", conn, peer = p, closed = conn.closed, description = exc.msg finally: @@ -220,9 +218,6 @@ proc handle*(p: PubSubPeer, conn: Connection) {.async: (raises: []).} = # This is top-level procedure which will work as separate task, so it # do not need to propagate CancelledError. trace "Unexpected cancellation in PubSubPeer.handle" - except CatchableError as exc: - trace "Exception occurred in PubSubPeer.handle", - conn, peer = p, closed = conn.closed, description = exc.msg finally: debug "exiting pubsub read loop", conn, peer = p, closed = conn.closed @@ -242,8 +237,6 @@ proc closeSendConn( p.onEvent(p, PubSubPeerEvent(kind: event)) except CancelledError as exc: raise exc - except CatchableError as exc: - debug "Errors during diconnection events", description = exc.msg # don't cleanup p.address else we leak some gossip stat table proc connectOnce( @@ -294,7 +287,11 @@ proc connectImpl(p: PubSubPeer) {.async: (raises: []).} = p.connectedFut.complete() return await connectOnce(p) - except CatchableError as exc: # never cancelled + except CancelledError as exc: + debug "Could not establish send connection", description = exc.msg + except LPError as exc: + debug "Could not establish send connection", description = exc.msg + except GetConnDialError as exc: debug "Could not establish send connection", description = exc.msg proc connect*(p: PubSubPeer) = diff --git a/tests/pubsub/testgossipsub.nim b/tests/pubsub/testgossipsub.nim index 64823bbea3..6b17ae9ad6 100644 --- a/tests/pubsub/testgossipsub.nim +++ b/tests/pubsub/testgossipsub.nim @@ -727,12 +727,7 @@ suite "GossipSub": handler = proc( topic: string, data: seq[byte] ) {.async: (raises: []), closure.} = - try: - if peerName notin seen: - seen[peerName] = 0 - seen[peerName].inc - except KeyError: - raiseAssert "seen checked before" + seen.mgetOrPut(peerName, 0).inc() check topic == "foobar" if not seenFut.finished() and seen.len >= runs: seenFut.complete() diff --git a/tests/pubsub/testgossipsub2.nim b/tests/pubsub/testgossipsub2.nim index 9e3d0e7bae..edf3a9ff99 100644 --- a/tests/pubsub/testgossipsub2.nim +++ b/tests/pubsub/testgossipsub2.nim @@ -65,15 +65,8 @@ suite "GossipSub": var handler: TopicHandler closureScope: var peerName = $dialer.peerInfo.peerId - handler = proc( - topic: string, data: seq[byte] - ) {.async: (raises: []), closure.} = - try: - if peerName notin seen: - seen[peerName] = 0 - seen[peerName].inc - except KeyError: - raiseAssert "seen checked before" + handler = proc(topic: string, data: seq[byte]) {.async: (raises: []).} = + seen.mgetOrPut(peerName, 0).inc() info "seen up", count = seen.len check topic == "foobar" if not seenFut.finished() and seen.len >= runs: @@ -282,12 +275,7 @@ suite "GossipSub": handler = proc( topic: string, data: seq[byte] ) {.async: (raises: []), closure.} = - try: - if peerName notin seen: - seen[peerName] = 0 - seen[peerName].inc - except KeyError: - raiseAssert "seen checked before" + seen.mgetOrPut(peerName, 0).inc() check topic == "foobar" if not seenFut.finished() and seen.len >= runs: seenFut.complete()