nim-chronos
nim-chronos copied to clipboard
status-im
Raise tracking
EDIT: This PR adds the possibility to do check exceptions in async procedures. More info from the updated README:
By specifying a asyncraises list to an async procedure, you can check which
exceptions can be thrown by it.
proc p1(): Future[void] {.async, asyncraises: [IOError].} =
assert not (compiles do: raise newException(ValueError, "uh-uh"))
raise newException(IOError, "works") # Or any child of IOError
Under the hood, the return type of p1 will be rewritten to another type,
which will convey raises informations to await.
proc p2(): Future[void] {.async, asyncraises: [IOError].} =
await p1() # Works, because await knows that p1
# can only raise IOError
This is indeed one of the options we've discussed in the past, to annotate future with an error type akin to Result, and make it customizable.
However, it would indeed be nice to reuse the raises instead, and this should be doable eventually - the way forward here should be that we move any given raises annotations to the generated iterator such that the compiler enforces them - what remains is to introspect code snippets with the effectsOf pragma from future nim versions, to transport type information about caught exceptions to the continuation site.
There are smaller incremental steps we can take however: one is an {.atask.} replacement for {.async.} where the only difference is that it acts as a full exception barrier, enforcing the use of try/catch - its primary use case would be asyncSpawn. A strengthening of strict exceptions, this would change the raises of the generated iterator to raises: [Defect] - this is already a requirement that we enforce at runtime.
Finally, there's already a few places experimenting with Future[Result... - one pragmatic way forward thus becomes to combine atask with special Result support in the library to ensure that no exceptions leak when using Result.
This is one of the two possible approaches to solve this problem. Besides changing the Future type, it's also possible to introduce an additional wrapper type that will appear in place of the T in the Future instantiation. This has several potential benefits:
- Less disruption, existing code continues to compile as it is
- Ability to introduce gradually in selected modules first
- The wrapper type can enforce the error handling at the call site if desired
I've written about the design and implemented the key parts of it here:
https://github.com/status-im/nim-stew/blob/b3a6b52c7764da0b351e748b1080127677c734eb/docs/error_handling.md https://github.com/status-im/nim-stew/pull/26/files
Having that said, changing the Future type is quite reasonable as well.
it's also possible to introduce an additional wrapper type that will appear in place
this is good for experiments experiments and proof-of-concepts, but I think ultimately we will want to land in a "native" solution for nim-chronos where we don't pay the EH and wrapper overhead (cognitive and implementation-wise) in general, except where those features are used - right now, the flow with exceptions across await barriers is quite expensive because we need to catch exceptions and repropagate them even in code that doesn't raise - better would be that code that doesn't use EH doesn't have to pay the cost either, in any way (not even in the Future object - for that to work, we'd need a more fine-grained Future implementation - one that also maybe disconnects writers from readers akin to C++) - basically, if code can be deduced not to raise, it doesn't need to catch, then store, then reraise, and this should be part of the final implementation.
Having that said, changing the Future type is quite reasonable as well.
does it even work to do Future[T, E = CatchableError] with a default E? I wonder how much stuff breaks then.
does it even work to do Future[T, E = CatchableError] with a default E?
Couldn't make it work. However, I think we could do
type
EFuture[T, E] =...
Future[T] = EFuture[T, (CatchableError)]
And then, the async pragma could automatically transform the return Future into a EFuture using the raises as E
However, it would indeed be nice to reuse the raises instead
Issue is, iirc, the push pragma is applied after async transformation, so we don't know the raises coming from push yet.
transport type information about caught exceptions to the continuation site.
To my current knowledge, the only way to do this is how I did it here
We need to know, at compile time, which exceptions can be thrown by a Future given to await. Meaning, from the future type, we must be able to get the exception list
To my current knowledge, the only way to do this is how I did it here
that's what effectsOf does, in future nim versions - though I anticipate that it will work poorly, given that it has to fully resolve and deduce the effects of the called code, something that nim tends to do quite poorly.
so we don't know the raises coming from push yet.
a starting point might be to get explicit {.async, raises ...} to work
"native" solution
This is a false dilemma in general. You can implement exactly the same specializations of the Future type and the surrounding code using the additional wrapper type.
You can
I'm saying that we must, compared to the proposal in nim-stew, if we're going to actually land it in something like nim-chronos that's used for real.. it's not so much a dilemma as a requirement so as to turn the error handling proposal into a pragmatic and real solution
that's what effectsOf does, in future nim versions?
effectsOf has limited utility to the problem at hand here. It can be only useful if await is defined as macro that looks at the call expression being awaited. In other words, await foo() would discover that foo() can raise certain exceptions and it would modify the call-site accordingly. The usefulness is lost when you allow the result of foo() to be assigned to a future variable before being awaited and thus it cannot deliver a complete solution. On the other hand, a "compete" solution based on modifying the Future type as it's done here doesn't need to use effectsOf in any way because the same information is already encoded in the result type.
I'm saying that we must, compared to the proposal in nim-stew, if we're going to actually land it in something like nim-chronos that's used for real.. it's not so much a dilemma as a requirement so as to turn the error handling proposal into a pragmatic and real solution
I don't understand any bit of this sentence. What I said is that you can define the error-information-carrying wrapper type at a layer that is above Chronos (or within Chronos) and you can still implement features such as "A Future for an async op that can't raise an exception doesn't have an error field". This renders the argument claiming that there are certain "optimisation" benefits of the Future[T, E] approach invalid and I just wanted to clarify that in the interest of having a fair discussion.
I did a V2:
Future*[T] = ref object of FutureBase ## Typed future.
[..]
FuturEx*[T, E] = Future[T]
This allows it to be backward compatible, you can use a Future directly but loose raises info, or use FuturEx to have it
The async macro will read the raises pragma and put info in the return type:
proc test1(): Future[void] {.async, raises: [ValueError].} =
raise newException(ValueError, "Value")
Will become
proc test1(): FuturEx[void, (CancelledError, ValueError)] {.stackTrace: off,
raises: [], gcsafe.} =
iterator test1_436207619(chronosInternalRetFuture: Future[void]): FutureBase {.
closure, gcsafe, raises: [CancelledError, ValueError].} =
block:
raise newException(ValueError, "Value")
complete(chronosInternalRetFuture)
var resultFuture = newFuture[void]("test1")
resultFuture.closure = test1_436207619
futureContinue(resultFuture)
return resultFuture
If no raises are found, CatchableError is added for back. compatibility
Works:
proc test1(): Future[void] {.async, raises: [ValueError].} =
raise newException(ValueError, "Value")
proc test2() {.async, raises: [].} =
await sleepAsync(100.milliseconds)
let toto = test1()
try:
await toto
echo "Mhh?"
except ValueError as exc:
echo "Works"
(and also shows why the errors must be stored in the future type, and effectsOf may not help us if we keep our current syntax)
Doesn't compile:
proc test1(): Future[void] {.async, raises: [ValueError].} =
raise newException(ValueError, "Value")
proc test2() {.async, raises: [].} =
await test1()
Doesn't compile:
proc test1(): Future[void] {.async, raises: [ValueError].} =
raise newException(ValueError, "Value")
proc test2() {.async, raises: [ValueError].} =
let fut: Future[void] = test1() # We lost raise info here
await fut # can raise an unlisted exception: CatchableError
one is an {.atask.} replacement for {.async.} where the only difference is that it acts as a full exception barrier
You would still need to inform the caller that you can't raise, otherwise eg
proc test1() {.atask.} =
discard
proc test2() {.atask.} =
await test1() # Can't compile, can raise unlisted exceptions
So the usefulness would be very limited (only useful for asyncSpawn)
The branches I had to create to make this work with nimbus: https://github.com/status-im/nim-libp2p/commit/7eb3225d19d0f42be4345380dd0f91fe14069371 https://github.com/status-im/nim-websock/commit/b31ff99b9d5200eea175520c820a3c9f91093bbf [same issues in json_rpc and nim-presto, but didn't make a branch for theses one yet]
With theses branches, everything compiles correctly
As you can see, the major issue is people (including me) putting wrong raises in async We could use a different pragma in async context, to avoid this issue and keep backward compatibility, but not sure it make total sense
There is also a weird issue with libp2p on nim devel, need to investigate
(libp2p/stream/connection.nim(91, 1) Error: Defect can raise an unlisted exception: Defect ?)
(EDIT: it's actually because we push raises: [Defect] everywhere in libp2p :/ )
wdyt?
Overall this looks really promising! I'd love to see this moving forward and it would be invaluable given the current status-quo!
Thinking about the Futurex type, one of the goals I had for the future type family was that it should carry no exceptions for code that doesn't raise - raises has the weakness that it forces dynamic type on the user which prevents several forms of static analysis, and code that doesn't raise shouldn't have to pay this price.
There are two ways to go forward here, I think - either a base type or allowing E to be void - this is also one of the reasons Result is more fundamentally more powerful as a vehicle for transporting error information: it allows both static and dynamic typing to be used, with the former allowing more powerful static analysis down the line - in a base layer library / type structure like Future, it's an important flexibility to have.
We have to anticipate that whatever type we add here will end up being used in user code as well - although the majority of code uses the macros, there are special cases to consider where the transformation ends up generating poor code and manually written Future code simply is better.
As you can see, the major issue is people (including me) putting wrong raises in async
putting raises on async in the current version is wrong indeed - however, push raises should not interfere - at least it hasn't interfered prior to this branch.
it should carry no exceptions for code that doesn't raise
Every async procedure can raise CancelledError
push raises should not interfere
The issue I have in libp2p devel is that this PR will add to the async procedure (not the iterator):
1.2: raises: [Defect]
> 1.2: raises: []
But in libp2p, we push raises: [Defect] everywhere. This caused an issue in >1.2 when a async method (with raises: []) is extended by a non-async method (which inherits the pushed raises: [Defect])
Pretty rare case, I think, and libp2p faults for pushing push Defect on >1.2
Every async procedure can raise CancelledError
indeed - this would need addressing at the core level of future as well - this is a problem in general, with the current model: it lacks the ability to express cancellation distinctly from "user code" errors - basically, if the user code raises a CancelledError, the model breaks, and this introduces unwelcome fragility at the core.
The fully expressive model would be something like: FutureNoRaises[Option[Result[T, E]]]] (where E can be an exception, in which case the "exception mode" kicks in and raises, as a way to coat the user code with sugar) - here, every concept is kept apart: the "future" aspect, the "cancellation aspect" and the "outcome of running the user code" aspect - it's verbose and ugly of course, which would need addressing, but that's a separate concern - the semantics of the await keyword itself perform the same sin: cancellation is intertwined with "general user error" using the same "reporting" mechanism for both. Key here would be to have primitives in the library that reasonably allow discovery of each of these concepts separately - then, on top one can add sugar for the most common "idioms".
Anyway, I'm writing it down mainly to provide a dump of how far I came along - it's not a blocker per se for improved raises tracking, but it should be considered when chaning the Future type overall.
Yes, current implementation is more geared toward backward compatibility than correctness, unfortunately After a while it would make sense to do a chronos V2 and break everything we don't like, but that's for another day..
Ok so we said goodbye to backward compatibility, now we must limit the breakage. We have a few options:
Option 1
type
FuturEx[T, E] = ref object of FutureBase
[...]
Future[T] = FuturEx[T, (CatchableError,)]
What it breaks:
proc test1: Future[void] {.async, raises: [].}
proc test2: Future[void] {.async.}
let
a: Future[void] = test1() # Won't work, because FuturEx[void, (CatchableError,)] is not compatible with FuturEx[void, (,)]
b: Future[void] = test2() # Works
Option 2
(major props to @markspanbroek for finding this one)
Future*[T] = ref object of FutureBase ## Typed future.
[..]
FuturEx*[T, E] = ref object of Future[T]
What it breaks:
type
broken = proc: Future[void]
works = proc: Future[void] {.async.} # modified chronos to make it work on proc types
proc test: Future[void] {.async.}
let
a: broken = test # Proc type are not compatible, even though they should be
b: works = test # works
Option 3
Future*[T] = ref object of FutureBase
[..]
Raising[T, E] = distinct Option[T] # or other
Haven't look to much at it yet, but it will break at least what option 1 & 2 does. Though, it will happen only when we start to modify the procedures, it will probably work as-is (just like option 1 does)
Ok so we said goodbye to backward compatibility,
well, one strategy is to develop everything on the side to completion using other names (async2 etc), so there's a fully working option to look towards, then think about how to minimize breakage.,
We can leave backward compatibility (meaning return Future instead of FuturEx) when there is no raise specified. Everything will continue to compile as is (minus the wrong raises discussed earlier), and we can fix the few incompatibilities (for reference, 5 in libp2p with option 2) while we add the raises through the codebase
Didn't we already establish that Option 2 cannot work?
For Option 3, the Raising type is defined as follows actually:
type
Raising[T, E] = distinct T
await just inserts a coercion to T to strip away the distinct type.
We can leave backward compatibility (meaning return Future instead of FuturEx) when there is no raise specified.
I suspect this might be underwhelming - it would mean that when raises is specified on the async proc, it'll have to do rewriting of the return type from Future[X] to the other one, which is surprising (async, for better or worse, does not do return type rewriting) - or the user has to type FutureEx instead of raising (ie proc f(): FutureEx[T, E] {.async.}) - one of the difficulties here is interop with "manual" code which is needed / useful in say .. 10% of the cases. This is why it's attractive to develop a complete solution on the side then worry about backwards compat.
@zah
Didn't we already establish that Option 2 cannot work?
I think option 2 works, it's not an alias but another type
I think option 2 works, it's not an alias but another type
Yes, indeed. I was too quick to comment.
I've added tests, updated the readme & the original message of this PR.
PTAL
nimbus-eth2 does compile, with a wall of Warnings because of async procedure using .raises.
btw, asyncraises - I just realised it should work for non-async functions as well - the use case is "manual" async functions that return Future[..] - these should then be rewritten to use RaisesTracking.. (and nothing else) - this way, asyncraises becomes the official syntax for raises tracking in manual functions as well, allowing those to keep compatibility with future changes as well as offering a way to write "manual" without relying on the semi-private type