-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Finish closures before completing futures (fix #415) #418
Conversation
chronos/asyncfutures2.nim
Outdated
@@ -317,6 +317,9 @@ proc futureContinue*(fut: FutureBase) {.raises: [], gcsafe.} = | |||
# `await` typically) or completes / fails / is cancelled | |||
next = fut.internalClosure(fut) | |||
if fut.internalClosure.finished(): # Reached the end of the transformed proc | |||
if not fut.finished: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, this is actually a really good point: we should not finish the future until the iterator itself has finished or an order inversion will indeed happen! it deserves to be documented.
it means that the above should not be a conditional but rather the norm - we should always arrive at this point with an unfinished future.
That in turn means that the expression return stuff needs to be rewritten as well to assign to value rather than calling complete
- ditto error paths which need to set the error field but not complete the future etc..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's actually a classic bug not to run iterators until completion and thus missing part of the code - the same happens with simple for
loop iterators that have code after the last yield
: if you break the for loop that code will not run..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, though it's messing up the location[Finish]
. Not sure what's the cleanest way forward, setFinishLocation
called from the macro, and a completeWithInternalValue
called from futureContinue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setFinishLocation
or just use internalLocation[...]
- ie none of this should be exposed in any public functions or API so it doesn't greatly matter - we can design a better public API for these kinds of things once the dust has settled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this slightly changed the Finish location (the proc
line instead of the first line of the proc)
Not sure this greatly matters
chronos/asyncfutures2.nim
Outdated
@@ -317,6 +317,8 @@ proc futureContinue*(fut: FutureBase) {.raises: [], gcsafe.} = | |||
# `await` typically) or completes / fails / is cancelled | |||
next = fut.internalClosure(fut) | |||
if fut.internalClosure.finished(): # Reached the end of the transformed proc | |||
#TODO hacky |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs a comment explaining why it's important that the future remains unfinished until the iterator is done (ie to run finally
code fully, or really, any code after the last yield
) - it's not actually hacky, it follows the way the compiler generates code for result - ie return xxx
is typically rewritten as result = xxx; return
- it's nuts but here we are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
funny enough, this PR should also improve performance because we will no longer generate a temporary variable that must be zero:ed then copied to the future |
This branch is failing on nimbus:
Some generic fun to fix EDIT: caused by nim-lang/Nim#22645 |
chronos/asyncmacro2.nim
Outdated
quote do: {.pop.}, | ||
) | ||
quote do: | ||
template result: auto {.used.} = `castFutureSym`.internalValue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever you want to do here, please avoid quote do
statements because this statements makes all our stack traces to point into asyncmacro2.nim file instead of pointing to actual source.
let | ||
res = newNimNode(nnkStmtList, node) | ||
res.add completeWithNode(fut, baseType, processBody(node[0], fut, baseType)) | ||
let res = newNimNode(nnkStmtList, node) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the comment here with generated source code.
this should have a test that shows the previously broken code snippet |
chronos/asyncmacro2.nim
Outdated
) | ||
), | ||
nnkElseExpr.newTree( | ||
newCall(ident "complete", fut, node) | ||
newAssignment(ident "result", node) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should directly use internalValue
rather than going via the template (more templates = more problems)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and processBody..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This proc is only used with procBodyBlck
which is already went through processBody
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more templates = more problems
That's unfortunately true, since using internalValue instead of result
fixed some compilation issue in nimbus
Looks like something that might bite us later
chronos/asyncmacro2.nim
Outdated
nnkElifExpr.newTree( | ||
nnkInfix.newTree( | ||
ident "isnot", baseType, ident "void"), | ||
newAssignment(ident "result", node[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto, avoid result - also, node[0]
needs to pass through processBody
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the test suite:
return if 1 == 1:
return 42
else:
33
assert returnedValue == 42
ie nested returns are valid code
Co-authored-by: Jacek Sieka <[email protected]>
chronos/asyncfutures2.nim
Outdated
@@ -165,6 +165,12 @@ template complete*(future: Future[void]) = | |||
## Completes a void ``future``. | |||
complete(future, getSrcLocation()) | |||
|
|||
proc completeWithInternalData(future: FutureBase) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
proc completeWithInternalData(future: FutureBase) = | |
proc completeWithInternalValue(future: FutureBase) = |
chronos/asyncmacro2.nim
Outdated
) | ||
|
||
proc completeWithNode(fut, baseType, node: NimNode): NimNode {.compileTime.} = | ||
proc completeWithNode(node, resultNode: NimNode): NimNode {.compileTime.} = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assignOrProcessNode
proc completeWithNode(node, resultNode: NimNode): NimNode {.compileTime.} = | |
proc assignOrProcessNode(node, resultNode: NimNode): NimNode {.compileTime.} = |
chronos/asyncmacro2.nim
Outdated
nnkPragma.newTree( | ||
nnkExprColonExpr.newTree( | ||
newIdentNode("error"), | ||
newLit("no async return type declared")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is confusing error message for everybody, even i do not understand what it mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
proc x: void =
return 42
testit.nim(2, 3) Error: no return type declared
this is what the compiler normally prints - not great but... maybe "cannot return value in function returning void"
also, the call to error
should include node
so that line number is included
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The node doesn't seem necessary in this case, it already points to the right line (probably since it's just creating the pragma, that will pop the error later)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And lesson learned, shouldn't copy the compiler error messages 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compiler provides no return type declared
your code provides no async return type declared
.
Where you found any mention of async return type
and what the difference with return type
is unclear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of compiler error messages are "cryptic" and sometimes when you generate code using macro, compiler could generate absolutely inadequate error message which could be understand only by macro code author. In this case generating error message is author's duty, because at the end error message will be read by library user, and it should not be cryptic, it should has meaning in context.
try: | ||
return | ||
finally: | ||
await stepsAsync(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to see more than one await
in finally
. and also i want to see how it works in exception handler, e.g. more than one await
. Also we need to test defer
functionality.
|
||
# don't do anything with forward bodies (empty) | ||
if procBody.kind != nnkEmpty: | ||
let | ||
# fix #13899, `defer` should not escape its original scope | ||
procBodyBlck = nnkBlockStmt.newTree(newEmptyNode(), procBody) | ||
|
||
# workaround https://github.com/nim-lang/Nim/issues/22645 | ||
internalFutureTypeSym = genSym(nskType, "InternalFutureType") | ||
castFutureSym2 = nnkCast.newTree(internalFutureTypeSym, internalFutureSym) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expanded code comment
We found a failure with this PR, and no workaround yet: proc testReturner: Future[int] {.async.} =
template returner =
result = 5
returner I'll look a bit more for a workaround, otherwise the solution will be to retain the temporary |
Superseded by #449 |
The idea of this PR would be that the closure never completes the future, instead just writes to the internalValue, and the
futureContinue
would take care of completing the future (for now I've just applied this to thereturn
rewrite, but could be used everywhere)Besides fixing #415, it could also avoid a copy to the internalValue once the future is finished
However, might have some downsides