nim-libp2p
nim-libp2p copied to clipboard
vacp2p
Raises
First attempt to use raises in async procs, uses https://github.com/status-im/nim-chronos/pull/166.
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.
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.
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
methodwith the sameraisesannotations 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.
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)
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.
It's not,
no idea what
nimbledoes 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 aboutCatchableErrorand notException)
Did you run it with the instructions I left in https://github.com/status-im/nim-libp2p/pull/547#issuecomment-802432804?
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"
attempting to do
LPChannel.readOnceis an example of amethodthat 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?
looks like there's a copy of
unittest2in the branch - this can be replaced withrequires "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.
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).
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
all the methods?
no, it only applies to
procfrom 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.
Seems to work for methods just as well from my experiments with this branch.
ok, maybe I hit something else, not sure about this one actually
Did you run it
yes, but that doesn't mean it works the same with unit tests and nimble etc -
Well, just to clarify, the point of this branch was to get it to a) compile and b) to pass the unittests. I'm sure that we'll uncover many more issues when using with our other projects.
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
Well, I'm not arguing with this, but it seems to deviate with the already established practice of module level annotations. If you follow the
Seems to work for methods just as well from my experiments with this branch.
ok, maybe I hit something else, not sure about this one actually
But it also seemed like I hadn't yet pushed a fix for annotating lpstream methods, so they are now. I think I did it precisely because the compiler got messed up when doing different overrides/overloads.
@arnetheduck @stefantalpalaru unittest2 seems to use setProgramResult, but I can't find it defined anywhere in the nim sources and it fails to build in some places. It seems to be a recent addition as of 1.4.x - https://nim-lang.org/docs/exitprocs.html?
It seems to be a recent addition as of 1.4.x
Good catch. It comes from merging the latest "unittest" code. We do test on "version-1-2", "version-1-4" and "devel" branches on Nim, but that code path is never taken in our tests (it would require calling fail() outside a unit test).
It seems to be a recent addition as of 1.4.x
Good catch. It comes from merging the latest "unittest" code. We do test on "version-1-2", "version-1-4" and "devel" branches on Nim, but that code path is never taken in our tests (it would require calling
fail()outside a unit test).
It actually does and it seems to happen when using check outside of a test template, this is useful with parameterized tests, which is where I'm running into it. You can see it for yourself in the testinterop.nim file in this branch.
~~since you're working on this, see if you can make the tests work in strict mode as well (https://github.com/status-im/nim-chronos/#exception-effects)~~ nevermind, just saw that it's already done :)
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.
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
Defectright 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.
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
Defectright 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() ...
readExactlycan only raise[Defect, LPStreamEOFError, LPStreamIncompleteError], the iteratorreadExactly_65120132can raise[Defect, CatchableError], which is less specific and allows any exception of typesDefectandCatchableErrorto escape.Any proc called by the iterator can raise an exception of type
DefectandCatchableError, butreadExactlyitself 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 ofasync.
As I said on discord, I think only the inner iterator matters in fact, and that is the root of the issue.
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.
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.