feat(pubsub): add {.async: (raises).} annotations
#1233
Conversation
libp2p/protocols/pubsub/pubsub.nim
Outdated
| except CatchableError as e: | ||
| trace "validator for message could not be executed, ignoring", topic = topic, err = e.msg | ||
| valResult = ValidationResult.Ignore |
There was a problem hiding this comment.
To avoid returning a CatchableError I capture the exception here, and ignore the message.
richard-ramos
force-pushed
the
async/pubsub
branch
2 times, most recently
from
ad9c827 to
e7f7642
Compare
richard-ramos
force-pushed
the
async/pubsub
branch
from
e7f7642 to
e1b48e5
Compare
richard-ramos
force-pushed
the
async/pubsub
branch
from
7e65a89 to
97f4bdc
Compare
| @@ -162,7 +162,7 @@ type | |||
| .} | |||
| P2PPubSubCallback* = proc( | |||
| api: DaemonAPI, ticket: PubsubTicket, message: PubSubMessage | |||
| ): Future[bool] {.gcsafe, raises: [CatchableError].} | |||
| ): Future[bool] {.gcsafe, async: (raises: [CatchableError]).} | |||
There was a problem hiding this comment.
btw, to maintain "callback compatibility" in public api, what we do is to introduce a 2 overload, like so: https://github.com/status-im/nim-chronos/blob/7c5cbf04a6bb2258654f16cb1d51b4304986cfd1/chronos/transports/stream.nim#L121
then the overload using the old callback includes a "translation wrapper" and/or is deprecated
There was a problem hiding this comment.
Thanks for the suggestion! Will implement this in a subsequent PR
| seen[peerName].inc | ||
| handler = proc( | ||
| topic: string, data: seq[byte] | ||
| ) {.async: (raises: []), closure.} = |
There was a problem hiding this comment.
closure is a leftover from old nim versions, should not be needed any more
| @@ -271,7 +276,7 @@ proc connectOnce(p: PubSubPeer): Future[void] {.async.} = | |||
| finally: | |||
| await p.closeSendConn(PubSubPeerEventKind.StreamClosed) | |||
|
|
|||
| proc connectImpl(p: PubSubPeer) {.async.} = | |||
| proc connectImpl(p: PubSubPeer) {.async: (raises: []).} = | |||
There was a problem hiding this comment.
this looks like it should propagate cancellederror - "most" api should do this to support cancellation - is there a specific reason here not to?
There was a problem hiding this comment.
Good question. The prev version had an except CatchableError that captures the CancelledError and also interestingly enough a comment that says never cancelled
nim-libp2p/libp2p/protocols/pubsub/pubsubpeer.nim
Lines 285 to 286 in 1fa30f0
Looking at the history, you introduced this change in 70deac9#diff-6f99d21f9b0d636313f9633209e5b125903440de8a134b40821e70c9b9ae4591 as it previously raised the CancelledError. Do you remember why?
| @@ -79,7 +81,7 @@ type | |||
| PubSubPeerEvent* = object | |||
| kind*: PubSubPeerEventKind | |||
|
|
|||
| GetConn* = proc(): Future[Connection] {.gcsafe, raises: [].} | |||
| GetConn* = proc(): Future[Connection] {.gcsafe, async: (raises: [GetConnDialError]).} | |||
There was a problem hiding this comment.
gcsafe is not needed either, with async
for some callbacks, it's worth considering allowing CancelledError - again, depending on the context, I don't remember this one in particular, but "cancelling a connection attempt" seems reasonable logically at least
| @@ -542,7 +555,7 @@ proc subscribe*(p: PubSub, topic: string, handler: TopicHandler) {.public.} = | |||
|
|
|||
| method publish*( | |||
| p: PubSub, topic: string, data: seq[byte] | |||
| ): Future[int] {.base, async, public.} = | |||
| ): Future[int] {.base, async: (raises: [LPError]), public.} = | |||
There was a problem hiding this comment.
a function like publish should probably not be raising - ie it's a "fire-and-forget" kind of function that attempts to publish things to as many peers as possible but just like udp, there are no guarantees that it will reach anybody - as such, it doesn't really make sense that it raises, I think.
This PR adds
{.async: (raises).}annotations to the pubsub package.The cases in which a
raises:[CatchableError]was added were due to not being part of the package and should probably be changed in a separate PR