From 5ebd771d35464832eb9edf603b616fd34ad158cd Mon Sep 17 00:00:00 2001 From: Jacek Sieka Date: Wed, 8 Nov 2023 15:12:32 +0100 Subject: [PATCH] per-function `Exception` handling (#457) This PR replaces the global strict exception mode with an option to handle `Exception` per function while at the same time enabling strict exception checking globally by default as has been planned for v4. `handleException` mode raises `AsyncExceptionError` to distinguish it from `ValueError` which may originate from user code. * remove obsolete 1.2 config options --- README.md | 105 ++++++++++++++--------- chronos/config.nim | 145 ++++++++++++++------------------ chronos/internal/asyncmacro.nim | 40 ++++++--- chronos/internal/errors.nim | 4 + tests/testmacro.nim | 11 +++ 5 files changed, 168 insertions(+), 137 deletions(-) diff --git a/README.md b/README.md index f59a6c8..c80f826 100644 --- a/README.md +++ b/README.md @@ -9,12 +9,12 @@ Chronos is an efficient [async/await](https://en.wikipedia.org/wiki/Async/await) framework for Nim. Features include: -* Efficient dispatch pipeline for asynchronous execution +* Asynchronous socket and process I/O * HTTP server with SSL/TLS support out of the box (no OpenSSL needed) -* Cancellation support * Synchronization primitivies like queues, events and locks -* FIFO processing order of dispatch queue -* Minimal exception effect support (see [exception effects](#exception-effects)) +* Cancellation +* Efficient dispatch pipeline with excellent multi-platform support +* Exception effect support (see [exception effects](#exception-effects)) ## Installation @@ -152,16 +152,13 @@ feet, in a certain section, is to not use `await` in it. ### Error handling -Exceptions inheriting from `CatchableError` are caught by hidden `try` blocks -and placed in the `Future.error` field, changing the future's status to -`Failed`. +Exceptions inheriting from [`CatchableError`](https://nim-lang.org/docs/system.html#CatchableError) +interrupt execution of the `async` procedure. The exception is placed in the +`Future.error` field while changing the status of the `Future` to `Failed` +and callbacks are scheduled. -When a future is awaited, that exception is re-raised only to be caught again -by a hidden `try` block in the calling async procedure. That's how these -exceptions move up the async chain. - -A failed future's callbacks will still be scheduled, but it's not possible to -resume execution from the point an exception was raised. +When a future is awaited, the exception is re-raised, traversing the `async` +execution chain until handled. ```nim proc p1() {.async.} = @@ -206,11 +203,11 @@ proc p3() {.async.} = await fut2 ``` -Chronos does not allow that future continuations and other callbacks raise -`CatchableError` - as such, calls to `poll` will never raise exceptions caused -originating from tasks on the dispatcher queue. It is however possible that -`Defect` that happen in tasks bubble up through `poll` as these are not caught -by the transformation. +Because `chronos` ensures that all exceptions are re-routed to the `Future`, +`poll` will not itself raise exceptions. + +`poll` may still panic / raise `Defect` if such are raised in user code due to +undefined behavior. #### Checked exceptions @@ -230,6 +227,53 @@ proc p2(): Future[void] {.async, (raises: [IOError]).} = Under the hood, the return type of `p1` will be rewritten to an internal type which will convey raises informations to `await`. +#### The `Exception` type + +Exceptions deriving from `Exception` are not caught by default as these may +include `Defect` and other forms undefined or uncatchable behavior. + +Because exception effect tracking is turned on for `async` functions, this may +sometimes lead to compile errors around forward declarations, methods and +closures as Nim conservatively asssumes that any `Exception` might be raised +from those. + +Make sure to excplicitly annotate these with `{.raises.}`: + +```nim +# Forward declarations need to explicitly include a raises list: +proc myfunction() {.raises: [ValueError].} + +# ... as do `proc` types +type MyClosure = proc() {.raises: [ValueError].} + +proc myfunction() = + raise (ref ValueError)(msg: "Implementation here") + +let closure: MyClosure = myfunction +``` + +For compatibility, `async` functions can be instructed to handle `Exception` as +well, specifying `handleException: true`. `Exception` that is not a `Defect` and +not a `CatchableError` will then be caught and remapped to +`AsyncExceptionError`: + +```nim +proc raiseException() {.async: (handleException: true, raises: [AsyncExceptionError]).} = + raise (ref Exception)(msg: "Raising Exception is UB") + +proc callRaiseException() {.async: (raises: []).} = + try: + raiseException() + except AsyncExceptionError as exc: + # The original Exception is available from the `parent` field + echo exc.parent.msg +``` + +This mode can be enabled globally with `-d:chronosHandleException` as a help +when porting code to `chronos` but should generally be avoided as global +configuration settings may interfere with libraries that use `chronos` leading +to unexpected behavior. + ### Raw functions Raw functions are those that interact with `chronos` via the `Future` type but @@ -302,27 +346,6 @@ annotated as raising `CatchableError` only raise on _some_ platforms - in order to work on all platforms, calling code must assume that they will raise even when they don't seem to do so on one platform. -### Strict exception mode - -`chronos` currently offers minimal support for exception effects and `raises` -annotations. In general, during the `async` transformation, a generic -`except CatchableError` handler is added around the entire function being -transformed, in order to catch any exceptions and transfer them to the `Future`. -Because of this, the effect system thinks no exceptions are "leaking" because in -fact, exception _handling_ is deferred to when the future is being read. - -Effectively, this means that while code can be compiled with -`{.push raises: []}`, the intended effect propagation and checking is -**disabled** for `async` functions. - -To enable checking exception effects in `async` code, enable strict mode with -`-d:chronosStrictException`. - -In the strict mode, `async` functions are checked such that they only raise -`CatchableError` and thus must make sure to explicitly specify exception -effects on forward declarations, callbacks and methods using -`{.raises: [CatchableError].}` (or more strict) annotations. - ### Cancellation support Any running `Future` can be cancelled. This can be used for timeouts, @@ -379,9 +402,9 @@ waitFor(cancellationExample()) Even if cancellation is initiated, it is not guaranteed that the operation gets cancelled - the future might still be completed or fail depending on the ordering of events and the specifics of -the operation. +the operation. -If the future indeed gets cancelled, `await` will raise a +If the future indeed gets cancelled, `await` will raise a `CancelledError` as is likely to happen in the following example: ```nim proc c1 {.async.} = diff --git a/chronos/config.nim b/chronos/config.nim index bd6c2b9..6af3e31 100644 --- a/chronos/config.nim +++ b/chronos/config.nim @@ -11,100 +11,77 @@ ## `chronosDebug` can be defined to enable several debugging helpers that come ## with a runtime cost - it is recommeneded to not enable these in production ## code. -when (NimMajor, NimMinor) >= (1, 4): - const - chronosStrictException* {.booldefine.}: bool = defined(chronosPreviewV4) - ## Require that `async` code raises only derivatives of `CatchableError` - ## and not `Exception` - forward declarations, methods and `proc` types - ## used from within `async` code may need to be be explicitly annotated - ## with `raises: [CatchableError]` when this mode is enabled. +const + chronosHandleException* {.booldefine.}: bool = false + ## Remap `Exception` to `AsyncExceptionError` for all `async` functions. + ## + ## This modes provides backwards compatibility when using functions with + ## inaccurate `{.raises.}` effects such as unannotated forward declarations, + ## methods and `proc` types - it is recommened to annotate such code + ## explicitly as the `Exception` handling mode may introduce surprising + ## behavior in exception handlers, should `Exception` actually be raised. + ## + ## The setting provides the default for the per-function-based + ## `handleException` parameter which has precedence over this global setting. + ## + ## `Exception` handling may be removed in future chronos versions. - chronosStrictFutureAccess* {.booldefine.}: bool = defined(chronosPreviewV4) + chronosStrictFutureAccess* {.booldefine.}: bool = defined(chronosPreviewV4) - chronosStackTrace* {.booldefine.}: bool = defined(chronosDebug) - ## Include stack traces in futures for creation and completion points + chronosStackTrace* {.booldefine.}: bool = defined(chronosDebug) + ## Include stack traces in futures for creation and completion points - chronosFutureId* {.booldefine.}: bool = defined(chronosDebug) - ## Generate a unique `id` for every future - when disabled, the address of - ## the future will be used instead + chronosFutureId* {.booldefine.}: bool = defined(chronosDebug) + ## Generate a unique `id` for every future - when disabled, the address of + ## the future will be used instead - chronosFutureTracking* {.booldefine.}: bool = defined(chronosDebug) - ## Keep track of all pending futures and allow iterating over them - - ## useful for detecting hung tasks + chronosFutureTracking* {.booldefine.}: bool = defined(chronosDebug) + ## Keep track of all pending futures and allow iterating over them - + ## useful for detecting hung tasks - chronosDumpAsync* {.booldefine.}: bool = defined(nimDumpAsync) - ## Print code generated by {.async.} transformation + chronosDumpAsync* {.booldefine.}: bool = defined(nimDumpAsync) + ## Print code generated by {.async.} transformation - chronosProcShell* {.strdefine.}: string = - when defined(windows): - "cmd.exe" + chronosProcShell* {.strdefine.}: string = + when defined(windows): + "cmd.exe" + else: + when defined(android): + "/system/bin/sh" else: - when defined(android): - "/system/bin/sh" - else: - "/bin/sh" - ## Default shell binary path. - ## - ## The shell is used as command for command line when process started - ## using `AsyncProcessOption.EvalCommand` and API calls such as - ## ``execCommand(command)`` and ``execCommandEx(command)``. + "/bin/sh" + ## Default shell binary path. + ## + ## The shell is used as command for command line when process started + ## using `AsyncProcessOption.EvalCommand` and API calls such as + ## ``execCommand(command)`` and ``execCommandEx(command)``. - chronosEventsCount* {.intdefine.} = 64 - ## Number of OS poll events retrieved by syscall (epoll, kqueue, poll). + chronosEventsCount* {.intdefine.} = 64 + ## Number of OS poll events retrieved by syscall (epoll, kqueue, poll). - chronosInitialSize* {.intdefine.} = 64 - ## Initial size of Selector[T]'s array of file descriptors. + chronosInitialSize* {.intdefine.} = 64 + ## Initial size of Selector[T]'s array of file descriptors. - chronosEventEngine* {.strdefine.}: string = - when defined(linux) and not(defined(android) or defined(emscripten)): - "epoll" - elif defined(macosx) or defined(macos) or defined(ios) or - defined(freebsd) or defined(netbsd) or defined(openbsd) or - defined(dragonfly): - "kqueue" - elif defined(android) or defined(emscripten): - "poll" - elif defined(posix): - "poll" - else: - "" - ## OS polling engine type which is going to be used by chronos. + chronosEventEngine* {.strdefine.}: string = + when defined(linux) and not(defined(android) or defined(emscripten)): + "epoll" + elif defined(macosx) or defined(macos) or defined(ios) or + defined(freebsd) or defined(netbsd) or defined(openbsd) or + defined(dragonfly): + "kqueue" + elif defined(android) or defined(emscripten): + "poll" + elif defined(posix): + "poll" + else: + "" + ## OS polling engine type which is going to be used by chronos. -else: - # 1.2 doesn't support `booldefine` in `when` properly - const - chronosStrictException*: bool = - defined(chronosPreviewV4) or defined(chronosStrictException) - chronosStrictFutureAccess*: bool = - defined(chronosPreviewV4) or defined(chronosStrictFutureAccess) - chronosStackTrace*: bool = defined(chronosDebug) or defined(chronosStackTrace) - chronosFutureId*: bool = defined(chronosDebug) or defined(chronosFutureId) - chronosFutureTracking*: bool = - defined(chronosDebug) or defined(chronosFutureTracking) - chronosDumpAsync*: bool = defined(nimDumpAsync) - chronosProcShell* {.strdefine.}: string = - when defined(windows): - "cmd.exe" - else: - when defined(android): - "/system/bin/sh" - else: - "/bin/sh" - chronosEventsCount*: int = 64 - chronosInitialSize*: int = 64 - chronosEventEngine* {.strdefine.}: string = - when defined(linux) and not(defined(android) or defined(emscripten)): - "epoll" - elif defined(macosx) or defined(macos) or defined(ios) or - defined(freebsd) or defined(netbsd) or defined(openbsd) or - defined(dragonfly): - "kqueue" - elif defined(android) or defined(emscripten): - "poll" - elif defined(posix): - "poll" - else: - "" +when defined(chronosStrictException): + {.warning: "-d:chronosStrictException has been deprecated in favor of handleException".} + # In chronos v3, this setting was used as the opposite of + # `chronosHandleException` - the setting is deprecated to encourage + # migration to the new mode. when defined(debug) or defined(chronosConfig): import std/macros @@ -113,7 +90,7 @@ when defined(debug) or defined(chronosConfig): hint("Chronos configuration:") template printOption(name: string, value: untyped) = hint(name & ": " & $value) - printOption("chronosStrictException", chronosStrictException) + printOption("chronosHandleException", chronosHandleException) printOption("chronosStackTrace", chronosStackTrace) printOption("chronosFutureId", chronosFutureId) printOption("chronosFutureTracking", chronosFutureTracking) diff --git a/chronos/internal/asyncmacro.nim b/chronos/internal/asyncmacro.nim index c110847..11daf33 100644 --- a/chronos/internal/asyncmacro.nim +++ b/chronos/internal/asyncmacro.nim @@ -33,7 +33,9 @@ proc processBody(node, setResultSym, baseType: NimNode): NimNode {.compileTime.} node[i] = processBody(node[i], setResultSym, baseType) node -proc wrapInTryFinally(fut, baseType, body, raises: NimNode): NimNode {.compileTime.} = +proc wrapInTryFinally( + fut, baseType, body, raises: NimNode, + handleException: bool): NimNode {.compileTime.} = # creates: # try: `body` # [for raise in raises]: @@ -92,15 +94,15 @@ proc wrapInTryFinally(fut, baseType, body, raises: NimNode): NimNode {.compileTi newCall(ident "fail", fut, excName) )) - let raises = if raises == nil: - const defaultException = - when defined(chronosStrictException): "CatchableError" - else: "Exception" - nnkTupleConstr.newTree(ident(defaultException)) + var raises = if raises == nil: + nnkTupleConstr.newTree(ident"CatchableError") elif isNoRaises(raises): nnkTupleConstr.newTree() else: - raises + raises.copyNimTree() + + if handleException: + raises.add(ident"Exception") for exc in raises: if exc.eqIdent("Exception"): @@ -115,7 +117,9 @@ proc wrapInTryFinally(fut, baseType, body, raises: NimNode): NimNode {.compileTi newCall(ident "fail", fut, nnkStmtList.newTree( nnkAsgn.newTree(closureSucceeded, ident"false"), - quote do: (ref ValueError)(msg: `excName`.msg, parent: `excName`))) + quote do: + (ref AsyncExceptionError)( + msg: `excName`.msg, parent: `excName`))) ) elif exc.eqIdent("CancelledError"): addCancelledError @@ -132,6 +136,8 @@ proc wrapInTryFinally(fut, baseType, body, raises: NimNode): NimNode {.compileTi newCall(ident "fail", fut, excName) )) + addDefect # Must not complete future on defect + nTry.add nnkFinally.newTree( nnkIfStmt.newTree( nnkElifBranch.newTree( @@ -193,7 +199,13 @@ proc cleanupOpenSymChoice(node: NimNode): NimNode {.compileTime.} = for child in node: result.add(cleanupOpenSymChoice(child)) -proc decodeParams(params: NimNode): tuple[raw: bool, raises: NimNode] = +type + AsyncParams = tuple + raw: bool + raises: NimNode + handleException: bool + +proc decodeParams(params: NimNode): AsyncParams = # decodes the parameter tuple given in `async: (name: value, ...)` to its # recognised parts params.expectKind(nnkTupleConstr) @@ -201,6 +213,7 @@ proc decodeParams(params: NimNode): tuple[raw: bool, raises: NimNode] = var raw = false raises: NimNode = nil + handleException = chronosHandleException for param in params: param.expectKind(nnkExprColonExpr) @@ -216,10 +229,12 @@ proc decodeParams(params: NimNode): tuple[raw: bool, raises: NimNode] = elif param[0].eqIdent("raw"): # boolVal doesn't work in untyped macros it seems.. raw = param[1].eqIdent("true") + elif param[0].eqIdent("handleException"): + handleException = param[1].eqIdent("true") else: warning("Unrecognised async parameter: " & repr(param[0]), param) - (raw, raises) + (raw, raises, handleException) proc isEmpty(n: NimNode): bool {.compileTime.} = # true iff node recursively contains only comments or empties @@ -261,7 +276,7 @@ proc asyncSingleProc(prc, params: NimNode): NimNode {.compileTime.} = let baseTypeIsVoid = baseType.eqIdent("void") - (raw, raises) = decodeParams(params) + (raw, raises, handleException) = decodeParams(params) internalFutureType = if baseTypeIsVoid: newNimNode(nnkBracketExpr, prc). @@ -406,7 +421,8 @@ proc asyncSingleProc(prc, params: NimNode): NimNode {.compileTime.} = castFutureSym, baseType, if baseTypeIsVoid: procBody # shortcut for non-generic `void` else: newCall(setResultSym, procBody), - raises + raises, + handleException ) closureBody = newStmtList(resultDecl, setResultDecl, completeDecl) diff --git a/chronos/internal/errors.nim b/chronos/internal/errors.nim index 083f7a2..8e6443e 100644 --- a/chronos/internal/errors.nim +++ b/chronos/internal/errors.nim @@ -3,3 +3,7 @@ type ## Generic async exception AsyncTimeoutError* = object of AsyncError ## Timeout exception + + AsyncExceptionError* = object of AsyncError + ## Error raised in `handleException` mode - the original exception is + ## available from the `parent` field. diff --git a/tests/testmacro.nim b/tests/testmacro.nim index c9b45dd..1361193 100644 --- a/tests/testmacro.nim +++ b/tests/testmacro.nim @@ -533,3 +533,14 @@ suite "Exceptions tracking": expect(Defect): f.fail((ref CatchableError)(), warn = false) check: not f.finished() + + test "handleException behavior": + proc raiseException() {. + async: (handleException: true, raises: [AsyncExceptionError]).} = + raise (ref Exception)(msg: "Raising Exception is UB and support for it may change in the future") + + proc callCatchAll() {.async: (raises: []).} = + expect(AsyncExceptionError): + await raiseException() + + waitFor(callCatchAll())