exception tracking #166

arnetheduck · 2021-03-14T16:49:09Z

This PR adds minimal exception tracking to chronos, moving the goalpost
one step further.

In particular, it becomes invalid to raise exceptions from callSoon
callbacks: this is critical for writing correct error handling because
there's no reasonable way that a user of chronos can possibly reason
about exceptions coming out of there: the event loop will be in an
indeterminite state when the loop is executing an random callback.

As expected, there are several issues in the error handling of chronos:
in particular, it will end up in an inconsistent internal state whenever
the selector loop operations fail, because the internal state update
functions are not written in an exception-safe way. This PR turns this
into a Defect, which probably is not the optimal way of handling things

expect more work to be done here.

Some API have no way of reporting back errors to callers - for example,
when something fails in the accept loop, there's not much it can do, and
no way to report it back to the user of the API - this has been fixed
with the new accept flow - the old one should be deprecated.

Finally, there is information loss in the API: in composite operations
like poll and waitFor there's no way to differentiate internal
errors from user-level errors originating from callbacks.

store CatchableError in future
annotate proc's with correct raises information
selectors2 to avoid non-CatchableError IOSelectorsException
$ should never raise
remove unnecessary gcsafe annotations
fix exceptions leaking out of timer waits
fix some imports
functions must signal raising the union of all exceptions across all
platforms to enable cross-platform code
switch to unittest2
add selectors2 which supercedes the std library version and fixes
several exception handling issues in there

An optional strict exception mode is also introduced, to be used when code correctly declares what exceptions it's raising - it is enabled with -d:chronosStrictException.

chronos/asyncmacro2.nim

chronos/transports/datagram.nim

chronos/transports/stream.nim

chronos/asyncfutures2.nim

chronos/asyncmacro2.nim

This commit introduces `unittest2`, driven by the need to fix several long-standing issues with `unittest`, in particular exception handling, test isolation, timing and others. Upgrading to `unittest2` generally requires updating the unittest import and potentially fixing the code to support the new `one-proc-per-test` setup. * nim-unittest2 has been moved from Stefans repository to status-im - status-im/nim-unittest2#2 fixes some exception handling issues and brings it in line with latest nim devel - a notable difference to unittest is that each test is run inside a proc which gives it a fresh stack and removes a lot of globals, meaning tests should run under conditions more similar to "normal" code and interfere with each other less - this is a crucial first step to improving the testing experience in general, that stefan implemented for the multithreading support * the multithreading in unittest2 is optional, and disabled by default * nim-testutils contains utilities for fuzzing, as well as other "large:ish" features that have dependencies - status-im/nim-testutils#27 pulls in unittest2 so unittest2 can be kept "low-dependency" * nim-chronos needs the above unittest changes to test its own new, stricter exception handling - status-im/nim-chronos#166 - the strictess introduced is expected to increase with time

This PR adds minimal exception tracking to chronos, moving the goalpost one step further. In particular, it becomes invalid to raise exceptions from `callSoon` callbacks: this is critical for writing correct error handling because there's no reasonable way that a user of chronos can possibly _reason_ about exceptions coming out of there: the event loop will be in an indeterminite state when the loop is executing an _random_ callback. As expected, there are several issues in the error handling of chronos: in particular, it will end up in an inconsistent internal state whenever the selector loop operations fail, because the internal state update functions are not written in an exception-safe way. This PR turns this into a Defect, which probably is not the optimal way of handling things - expect more work to be done here. Some API have no way of reporting back errors to callers - for example, when something fails in the accept loop, there's not much it can do, and no way to report it back to the user of the API - this has been fixed with the new accept flow - the old one should be deprecated. Finally, there is information loss in the API: in composite operations like `poll` and `waitFor` there's no way to differentiate internal errors from user-level errors originating from callbacks. * store `CatchableError` in future * annotate proc's with correct raises information * `selectors2` to avoid non-CatchableError IOSelectorsException * `$` should never raise * remove unnecessary gcsafe annotations * fix exceptions leaking out of timer waits * fix some imports * functions must signal raising the union of all exceptions across all platforms to enable cross-platform code * switch to unittest2 * add `selectors2` which supercedes the std library version and fixes several exception handling issues in there

chronos/apps/http/httpserver.nim

chronos/asyncfutures2.nim

chronos/asyncloop.nim

cheatfate · 2021-03-22T08:35:14Z

chronos/asyncloop.nim

-proc runForever*() =
+{.push raises: [Defect].}
+
+proc runForever*() {.raises: [Defect, CatchableError].} =


I think this procedure should not leak CatchableError.

the exception happens when poll raises which happens when the select/iocp call fails, which "shouldn't" happen under normal conditions - to make this loop "never" raise would require that poll doesn't raise, or it'll simply get into a tight failure loop.

cheatfate · 2021-03-22T08:35:20Z

chronos/asyncloop.nim

  while true:
    poll()

-proc waitFor*[T](fut: Future[T]): T =
+proc waitFor*[T](fut: Future[T]): T {.raises: [Defect, CatchableError].} =


I think this procedure should not leak CatchableError.

it raises the CatchableError that is potentially inside the future - it would have to return the future (or a Result) in order to only raise when poll raises

cheatfate · 2021-03-22T08:39:43Z

chronos/transports/stream.nim

@@ -864,10 +879,12 @@ when defined(windows):
                                           DWORD(server.bufferSize),
                                           DWORD(0), nil)
          if pipeHandle == INVALID_HANDLE_VALUE:
-            raiseTransportOsError(osLastError())
+            raiseAssert $osLastError()


osErrorMsg(osLastError()) but not $.

cheatfate · 2021-03-22T08:41:46Z

chronos/transports/stream.nim

@@ -1072,7 +1092,12 @@ when defined(windows):
                                             nil, flags)
          # Start tracking transport
          trackStream(ntransp)
-          server.createAcceptPipe()
+          try:
+            server.createAcceptPipe()


trackStream(ntransp) should be inside try/except branch, so only successful attempts will be tracked.

moved it after the try/catch which should have that same effect?

cheatfate · 2021-03-22T08:41:54Z

chronos/transports/stream.nim

+          try:
+            server.createAcceptPipe()
+          except CatchableError as exc:
+            # TODO close transp!


closeHandle(sock) ?

ok, pushed something, but take a look - the flow here is quite hard to follow

* Revisit Floodsub (#543) Fixes #525 add coverage to unsubscribeAll and testing * add mounted protos to identify message (#546) * add stable/unstable auto bumps * fix auto-bump CI * merge nbc auto bump with CI in order to bump only on CI success * put conditional locks on nbc bump (#549) * Fix minor exception issues (#550) Makes code compatible with status-im/nim-chronos#166 without requiring it. * fix nimbus ref for auto-bump stable's PR * Split dialer (#542) * extracting dialing logic to dialer * exposing upgrade methods on transport * cleanup * fixing tests to use new interfaces * add comments * add base exception class and fix hierarchy * fix imports * `doAssert` is `ValueError` not `AssertionError`? * revert back to `AssertionError` Co-authored-by: Giovanni Petrantoni <7008900+sinkingsugar@users.noreply.github.com> Co-authored-by: Jacek Sieka <jacek@status.im>

* use a builder pattern to build the switch (#551) * use a builder pattern to build the switch * with with * more refs * Merge master (#555) * Revisit Floodsub (#543) Fixes #525 add coverage to unsubscribeAll and testing * add mounted protos to identify message (#546) * add stable/unstable auto bumps * fix auto-bump CI * merge nbc auto bump with CI in order to bump only on CI success * put conditional locks on nbc bump (#549) * Fix minor exception issues (#550) Makes code compatible with status-im/nim-chronos#166 without requiring it. * fix nimbus ref for auto-bump stable's PR * Split dialer (#542) * extracting dialing logic to dialer * exposing upgrade methods on transport * cleanup * fixing tests to use new interfaces * add comments * add base exception class and fix hierarchy * fix imports * `doAssert` is `ValueError` not `AssertionError`? * revert back to `AssertionError` Co-authored-by: Giovanni Petrantoni <7008900+sinkingsugar@users.noreply.github.com> Co-authored-by: Jacek Sieka <jacek@status.im> * `doAssert` is `ValueError` not `AssertionError`? * revert back to `AssertionError` * fix builders * more builder stuff * more builders Co-authored-by: Giovanni Petrantoni <7008900+sinkingsugar@users.noreply.github.com> Co-authored-by: Jacek Sieka <jacek@status.im>

* builders (#559) * More builders (#560) * address some issues pointed out in review * re-add to prevent breaking other projects * Merge master (#555) * Revisit Floodsub (#543) Fixes #525 add coverage to unsubscribeAll and testing * add mounted protos to identify message (#546) * add stable/unstable auto bumps * fix auto-bump CI * merge nbc auto bump with CI in order to bump only on CI success * put conditional locks on nbc bump (#549) * Fix minor exception issues (#550) Makes code compatible with status-im/nim-chronos#166 without requiring it. * fix nimbus ref for auto-bump stable's PR * Split dialer (#542) * extracting dialing logic to dialer * exposing upgrade methods on transport * cleanup * fixing tests to use new interfaces * add comments * add base exception class and fix hierarchy * fix imports * `doAssert` is `ValueError` not `AssertionError`? * revert back to `AssertionError` Co-authored-by: Giovanni Petrantoni <7008900+sinkingsugar@users.noreply.github.com> Co-authored-by: Jacek Sieka <jacek@status.im> Co-authored-by: Giovanni Petrantoni <7008900+sinkingsugar@users.noreply.github.com> Co-authored-by: Jacek Sieka <jacek@status.im>

* Revisit Floodsub (#543) Fixes #525 add coverage to unsubscribeAll and testing * add mounted protos to identify message (#546) * add stable/unstable auto bumps * fix auto-bump CI * merge nbc auto bump with CI in order to bump only on CI success * put conditional locks on nbc bump (#549) * Fix minor exception issues (#550) Makes code compatible with status-im/nim-chronos#166 without requiring it. * fix nimbus ref for auto-bump stable's PR * use a builder pattern to build the switch (#551) * use a builder pattern to build the switch * with with * more refs * builders (#559) * More builders (#560) * address some issues pointed out in review * re-add to prevent breaking other projects * mem usage cleanups for pubsub (#564) In `async` functions, a closure environment is created for variables that cross an await boundary - this closure environment is kept in memory for the lifetime of the associated future - this means that although _some_ variables are no longer used, they still take up memory for a long time. In Nimbus, message validation is processed in batches meaning the future of an incoming gossip message stays around for quite a while - this leads to memory consumption peaks of 100-200 mb when there are many attestations in the pipeline. To avoid excessive memory usage, it's generally better to move non-async code into proc's such that the variables therein can be released earlier - this includes the many hidden variables introduced by macro and template expansion (ie chronicles that does expensive exception handling) * move seen table salt to floodsub, use there as well * shorten seen table salt to size of hash * avoid unnecessary memory allocations and copies in a few places * factor out message scoring * avoid reencoding outgoing message for every peer * keep checking validators until reject (in case there's both reject and ignore) * `readOnce` avoids `readExactly` overhead for single-byte read * genericAssign -> assign2 * More gossip coverage (#553) * add floodPublish test * test delivery via control Iwant/have mechanics * fix issues in control, and add testing * fix possible backoff issue with pruned routine overriding it * fix control messages (#566) * remove unused control graft check in handleControl * avoid sending empty Iwant messages * Split dialer (#542) * extracting dialing logic to dialer * exposing upgrade methods on transport * cleanup * fixing tests to use new interfaces * add comments * add base exception class and fix hierarchy * fix imports * Merge master (#555) * Revisit Floodsub (#543) Fixes #525 add coverage to unsubscribeAll and testing * add mounted protos to identify message (#546) * add stable/unstable auto bumps * fix auto-bump CI * merge nbc auto bump with CI in order to bump only on CI success * put conditional locks on nbc bump (#549) * Fix minor exception issues (#550) Makes code compatible with status-im/nim-chronos#166 without requiring it. * fix nimbus ref for auto-bump stable's PR * Split dialer (#542) * extracting dialing logic to dialer * exposing upgrade methods on transport * cleanup * fixing tests to use new interfaces * add comments * add base exception class and fix hierarchy * fix imports * `doAssert` is `ValueError` not `AssertionError`? * revert back to `AssertionError` Co-authored-by: Giovanni Petrantoni <7008900+sinkingsugar@users.noreply.github.com> Co-authored-by: Jacek Sieka <jacek@status.im> * Builders (#558) * use a builder pattern to build the switch (#551) * use a builder pattern to build the switch * with with * more refs * Merge master (#555) * Revisit Floodsub (#543) Fixes #525 add coverage to unsubscribeAll and testing * add mounted protos to identify message (#546) * add stable/unstable auto bumps * fix auto-bump CI * merge nbc auto bump with CI in order to bump only on CI success * put conditional locks on nbc bump (#549) * Fix minor exception issues (#550) Makes code compatible with status-im/nim-chronos#166 without requiring it. * fix nimbus ref for auto-bump stable's PR * Split dialer (#542) * extracting dialing logic to dialer * exposing upgrade methods on transport * cleanup * fixing tests to use new interfaces * add comments * add base exception class and fix hierarchy * fix imports * `doAssert` is `ValueError` not `AssertionError`? * revert back to `AssertionError` Co-authored-by: Giovanni Petrantoni <7008900+sinkingsugar@users.noreply.github.com> Co-authored-by: Jacek Sieka <jacek@status.im> * `doAssert` is `ValueError` not `AssertionError`? * revert back to `AssertionError` * fix builders * more builder stuff * more builders Co-authored-by: Giovanni Petrantoni <7008900+sinkingsugar@users.noreply.github.com> Co-authored-by: Jacek Sieka <jacek@status.im> Co-authored-by: Giovanni Petrantoni <7008900+sinkingsugar@users.noreply.github.com> Co-authored-by: Jacek Sieka <jacek@status.im>