Raises

[dryajov]

First attempt to use raises in async procs, uses status-im/nim-chronos#166.

[dryajov]

I'm not sure how to make nim pick the correct version of chronos when using the branch name, it seems to ignore the branch in the nimble file and just pick whatever looks like the latest numeric version so CI is broken for now. But it works in the nimbus repo if the proper branches in chronos (eh-tracking) and stew (latest head) are checked out.

[arnetheduck]

It might make the transition more smooth if the refactoring to raise a single exception type was separated from the chronos exception adaptation.

pick the correct version

it is using the correct chronos branch - the error is legit, you need to annotate all method with the same raises annotations or compiler will freak out.

[dryajov]

it is using the correct chronos branch - the error is legit

It's not, it's picking the wrong chronos and this is verifiable by nimble path chronos.

you need to annotate all method with the same raises annotations or compiler will freak out.

This is well understood and it is what the branch is attempting to do. However, since eh-tracking is/was still changing, additional issues might have been introduced from the point when this branch was created.

[arnetheduck]

It's not,

no idea what nimble does locally, it tends to give a random version then, depending on what's in your nimble cache, but the error message in CI is legit for the eh-tracking branch (it's complaining about CatchableError and not Exception)

[arnetheduck]

shortLog should never raise - in fact it doesn't actually except that & raises on invalid format strings - the right thing to do here is to catch the ValueError and turn it into a Defect

[dryajov]

Yeah, quick and dirty fix to get things to compile on first pass, but I suspected that this wasn't the right approach.

[arnetheduck]

attempting to do

LPChannel.readOnce is an example of a method that is not annotated in this branch (annotating the base is not enough) - there's plenty of them in the branch though that remain unannotated - the CI error I happened to look at was because of this.

[arnetheduck]

this is probably nim-lang/Nim#17382 - looks like a nim bug, can use it in the comment to highlight where it happens

[arnetheduck]

also, it incorrectly raises whatever handler raises - since you saw Exception here it means handler itself was missing a correct raises signature - looks like it has one now though

[dryajov]

ConnEventHandler is defined as:

  ConnEventHandler* =
    proc(peerId: PeerID, event: ConnEvent): Future[void]
      {.gcsafe, raises: [Defect].}

But interestingly, it seems to ignore that when actually declaring the handler which is in https://github.com/status-im/nim-libp2p/blob/unstable/tests/testswitch.nim#L249. Another bug?

[dryajov]

It's not,

no idea what nimble does locally, it tends to give a random version then, depending on what's in your nimble cache, but the error message in CI is legit for the eh-tracking branch (it's complaining about CatchableError and not Exception)

Did you run it with the instructions I left in #547 (comment)?

[arnetheduck]

looks like there's a copy of unittest2 in the branch - this can be replaced with requires "https://github.com/status-im/nim-unittest2.git#head"

[dryajov]

attempting to do

LPChannel.readOnce is an example of a method that is not annotated in this branch (annotating the base is not enough) - there's plenty of them in the branch though that remain unannotated - the CI error I happened to look at was because of this.

There is a {.push raises: [Defect].} enabled for the entire module, shouldn't that apply to all the methods?

[dryajov]

looks like there's a copy of unittest2 in the branch - this can be replaced with requires "https://github.com/status-im/nim-unittest2.git#head"

Yes, I ran into the issue with unhandled Exception coming from the formaters which I worked around locally.

[arnetheduck]

all the methods?

no, it only applies to proc from what I can tell (might be another bug, who knows) - each and every method needs an explicit raises annotation (that matches the base exactly).

[arnetheduck]

Did you run it

yes, but that doesn't mean it works the same with unit tests and nimble etc - the eh tracking issues issues sometimes seem to depend on the order in which the compiler sees the code which might be different depending on include path order etc - it's easiest to just annotate everything consistently

[dryajov]

all the methods?

no, it only applies to proc from what I can tell

Seems to work for methods just as well from my experiments with this branch. However if it's not the case and you have a concrete case, we need to report it upstream and apply the fix accordingly.

[dryajov]

I'm a bit conflicted about this branch - it gets us very close to proper raises annotations, but as @arnetheduck points out, chronos only allows for Defect right now, so those are neither useful nor guaranteed to be correct.

When I started implementing this branch, a few of the specific exceptions would be flagged and adding them to the raises list fixed the issues, but most likely it was something else either in my code or simply because I was working against the in progress chronos branch.

In any case, there is a lot of good cleanup here and like I said most of the specific raises annotations actually do make sense, but alas aren't currently verifiable/enforceable.

To move this forward, we can either

• extract all the changes sans the specific exception type annotations into its own branch and leave this one around for when chronos can handle specific annotations
• merge this as is and fix the issues once chronos adds the ability to handle the specific annotations

The former option would simply mean removing raises: [...] from all methods/procs and I'm inclined to say it's probably the safest thing to do right now.

Actually, I took another look at this and I think that this PR should generally work as expected. Here is an example of an expanded async proc:

proc readExactly*(s: LPStream; pbytes: pointer; nbytes: int): Future[void] {.
    raises: [Defect, LPStreamEOFError, LPStreamIncompleteError], stackTrace: off.} =
  var chronosInternalRetFuture = newFuture[void]("readExactly")
  iterator readExactly_65120132(): FutureBase {.closure,
      raises: [Defect, CatchableError].} =
    block:
      template result(): auto {.used.} =
        {.fatal: "You should not reference the `result` variable inside a void async proc".}

      if s.atEof:
        raise newLPStreamEOFError()
...

readExactly can only raise [Defect, LPStreamEOFError, LPStreamIncompleteError], the iterator readExactly_65120132 can raise [Defect, CatchableError], which is less specific and allows any exception of types Defect and CatchableError to escape.

Any proc called by the iterator can raise an exception of type Defect and CatchableError, but readExactly itself or the procs called by it can only raise [Defect, LPStreamEOFError, LPStreamIncompleteError], so on and so forth, thus the chain of exception annotations propagates correctly across all invocations of async.

[sinkingsugar]

I'm a bit conflicted about this branch - it gets us very close to proper raises annotations, but as @arnetheduck points out, chronos only allows for Defect right now, so those are neither useful nor guaranteed to be correct.
When I started implementing this branch, a few of the specific exceptions would be flagged and adding them to the raises list fixed the issues, but most likely it was something else either in my code or simply because I was working against the in progress chronos branch.
In any case, there is a lot of good cleanup here and like I said most of the specific raises annotations actually do make sense, but alas aren't currently verifiable/enforceable.
To move this forward, we can either

• extract all the changes sans the specific exception type annotations into its own branch and leave this one around for when chronos can handle specific annotations
• merge this as is and fix the issues once chronos adds the ability to handle the specific annotations

The former option would simply mean removing raises: [...] from all methods/procs and I'm inclined to say it's probably the safest thing to do right now.

Actually, I took another look at this and I think that this PR should generally work as expected. Here is an example of an expanded async proc:

proc readExactly*(s: LPStream; pbytes: pointer; nbytes: int): Future[void] {.
    raises: [Defect, LPStreamEOFError, LPStreamIncompleteError], stackTrace: off.} =
  var chronosInternalRetFuture = newFuture[void]("readExactly")
  iterator readExactly_65120132(): FutureBase {.closure,
      raises: [Defect, CatchableError].} =
    block:
      template result(): auto {.used.} =
        {.fatal: "You should not reference the `result` variable inside a void async proc".}

      if s.atEof:
        raise newLPStreamEOFError()
...

readExactly can only raise [Defect, LPStreamEOFError, LPStreamIncompleteError], the iterator readExactly_65120132 can raise [Defect, CatchableError], which is less specific and allows any exception of types Defect and CatchableError to escape.

Any proc called by the iterator can raise an exception of type Defect and CatchableError, but readExactly itself or the procs called by it can only raise [Defect, LPStreamEOFError, LPStreamIncompleteError], so on and so forth, thus the chain of exception annotations propagates correctly across all invocations of async.

As I said on discord, I think only the inner iterator matters in fact, and that is the root of the issue.

[arnetheduck]

To understand why the annotation is wrong, you need to read the whole transformed function:

proc readExactly*(rstream: AsyncStreamReader; pbytes: pointer; nbytes: int): Future[
    void] {.stackTrace: off.} =
  var chronosInternalRetFuture = newFuture[void]("readExactly")
  iterator readExactly_25840371(): FutureBase {.closure,
      raises: [Defect, CatchableError, Exception].} =
    block:
      template result(): auto {.used.} =
        {.fatal: "You should not reference the `result` variable inside a void async proc".}
    ...
    complete(chronosInternalRetFuture)

  var nameIterVar`gensym25840373 = readExactly_25840371
  {.push, stackTrace: off.}
  var readExactly_continue_25840372: proc (udata`gensym25840374: pointer) {.gcsafe,
      raises: [Defect].}
  readExactly_continue_25840372 = proc (udata`gensym25840375: pointer) {.gcsafe,
      raises: [Defect].} =
    try:
      if not (nameIterVar`gensym25840373.finished()):
        var next`gensym25840376 = nameIterVar`gensym25840373()
        while (not next`gensym25840376.isNil()) and
            next`gensym25840376.finished():
          next`gensym25840376 = nameIterVar`gensym25840373()
          if nameIterVar`gensym25840373.finished():
            break
        if next`gensym25840376 == nil:
          if not (chronosInternalRetFuture.finished()):
            const
              msg`gensym25840377 = "Async procedure (&" & "readExactly" &
                  ") yielded `nil`, " &
                  "are you await\'ing a `nil` Future?"
            raiseAssert msg`gensym25840377
        else:
          next`gensym25840376.addCallback(readExactly_continue_25840372)
    except CancelledError:
      chronosInternalRetFuture.cancelAndSchedule()
    except CatchableError as exc`gensym25840378:
      chronosInternalRetFuture.fail(exc`gensym25840378)
    except Exception as exc`gensym25840379:
      if exc`gensym25840379 of Defect:
        raise (ref Defect)(exc`gensym25840379)
      chronosInternalRetFuture.fail((ref ValueError)(msg: exc`gensym25840379.msg,
          parent: exc`gensym25840379))
  readExactly_continue_25840372(nil)
  {.pop.}
  return chronosInternalRetFuture

In particular, you need to understand that the transformed readExactly that gets called never raises - in fact, it would be wrong for it to do so - the contract of functions that return a Future is that they don't raise themselves but rather move all exceptions inside the future - you can call these functions using await, or not, and you can call them outside async, or inside - it doesn't matter - they return a future and must never raise themselves - this would create two error paths for them, and this is wrong - hence the correct annotation on them is raises: [Defect].

If you read on, you will notice that readExactly_continue catches all Catchables and puts them in the future, for this reason. If you were to create a function that returns a Future "manually" without {.async.} it would be your responsibility to ensure this function doesn't raise and instead does the same try/except.

By adding the annotation, you're creating a lie that will impact code down the line, because readExactly, after having been transformed, doesn't raise - downstream code will pass this misinformation on and code that uses libp2p will have to add workarounds to clean up this lie.

[arnetheduck]

extract all the changes sans the specific exception type annotations into its own branch and leave this one around for when chronos can handle specific annotations

this is the path forward right now that causes the least issues for future code - it would be good to put the code in a cleaned up branch so we can remove the hacks from nbc as well - a new PR is probably appropriate because there's a lot of comments in this branch that come from pre-release versions of chronos 3.

[arnetheduck]

Obsoleted by #1050 et al.