{.async: (raises).} for MultistreamSelect
#1066
Conversation
Annotate `MultistreamSelect` with `{.async: (raises).}` and ensure that
`handle` returns a `MultiStreamError` instead of `CatchableError` in the
"invalid first message" case.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #1066 +/- ##
============================================
- Coverage 82.56% 82.50% -0.06%
============================================
Files 91 91
Lines 15814 15849 +35
============================================
+ Hits 13057 13077 +20
- Misses 2757 2772 +15
|
| conn: Connection, | ||
| proto: string | ||
| ): Future[bool] {.async: (raises: [ | ||
| CancelledError, LPStreamError, MultiStreamError]).} = |
There was a problem hiding this comment.
not sure this is the right PR for it, but reraising underlying stream errors represents an abstraction leak - ie they should be caught and rebranded as multistreamerror really
There was a problem hiding this comment.
Agree, but the pattern of re-raising LPStreamError applies to quite large parts of the codebase, and the goal for now is to just document the reality with the {.async: (raises).}, while fixing the obvious offenders of raise (ref CatchableError)(). Semantic changes should be separate.
There was a problem hiding this comment.
Another precedent we have in muxer.nim where the procs currently raise {.async: (raises: [CancelledError, LPStreamError, MuxerError].}, it's just how it's done today.
libp2p/multistream.nim
Outdated
| proc start*(m: MultistreamSelect) {.async.} = | ||
| await allFutures(m.handlers.mapIt(it.protocol.start())) | ||
| proc start*(m: MultistreamSelect) {.async: (raises: []).} = | ||
| await noCancel allFutures(m.handlers.mapIt(it.protocol.start())) |
There was a problem hiding this comment.
hm, this is somewhat unexpected, that protocol start functions shouldn't support cancellation - I'm thinking about the case where for example a protocol might need to perform a dns lookup or the like and we want to exit the process - timeouts in such protocols can be longer than the "cancellation tolerance".
There was a problem hiding this comment.
Once more it is simply reflecting current behaviour. But agree that having cancellation on start feels more intuitive. Have updated the logic to support that, and also to properly process cancellation to avoid inconsistent state.
There was a problem hiding this comment.
When deciding how to constrain raises, it's important to take into account an important aspect of exception-based API: when they raise CatchableError it means the calling code must be prepared to handle any error even if de facto there's no raise today - this is an important "feature" of designing exception-based api: you can "future-proof" them by signalling in the signature that you're raising more than you actually are - this is why the strategy of simply "describing current behavior" is .. incomplete: it doesn't take into account the non-code intent that went into the design.
Of course, in this particular case, there was probably no intent or thinking about cancellation at all since the code didn't actually do cancellation correctly until your fix, but when "narrowing" an API to raise less, it's a good question to ask.
There was a problem hiding this comment.
Yes, widening it to match the intended design space makes sense.
| method start*(g: GossipSub) {.async.} = | ||
| method start*( | ||
| g: GossipSub | ||
| ): Future[void] {.async: (raises: [CancelledError], raw: true).} = |
There was a problem hiding this comment.
why the raw here? can't see how it matters.
There was a problem hiding this comment.
not familiar enough to know if it matters, but it makes the flow more explicit. there is no async involved in this method, so if the transformation introduces more complex logic or increases compile time, rather opting out of it
There was a problem hiding this comment.
makes it hard to read though - complexity is negligible in simple cases like this, specially since it's not part of any hot path
There was a problem hiding this comment.
yeah, that's true, the non-raw version is a bit simpler to read, but is a bit less clear about what's going on / as in, there is no async involved at all in the implementation, it's just a regular synchronous function.
side note: one of the other PRs actually was triggered by switching some of the redirectors to raw, it changed test timing just slightly to make it fail, revealing a small logic issue.
anyway, will keep it in mind going forward and transform to raw a bit less agressively. I think one could argue that anytime newFuture can be avoided, the readability benefit generally outweighs the optimization aspect; while for redirections / wrappers where newFuture is not necessary, the raw version looks similar enough to the transformed version that there is no downside to using it?
can do another cleanup pass for readability after the raises annotations are done, depending on the outcome of the discussion. personally I'm also fine with newFuture.
There was a problem hiding this comment.
the thing with newFuture is that it opens up two avenues for "classic" bugs: returning nil on some code path and returning an accidentally unfinished future on some code path - it's risky shit, for a dumb start function. ie when reviewing, I had to spend time ascertaining this point (it would have been more clear btw had you put the newfuture call at the end of the function where it gets returned, instead of at the beginning because as is, there's too much unnecessary distance between construction and use).
The most simple things are the most pleasurable to debate ;)
There was a problem hiding this comment.
True, haven't been bitten by those personally, but can see them happening relatively easily. It's a good point, generally, especially as it seems that the async transformation is not too costly for purely synchronous functions.
Annotate
MultistreamSelectwith{.async: (raises).}and ensure thathandlereturns aMultiStreamErrorinstead ofCatchableErrorin the "invalid first message" case.