Skip to content
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

Complete futures in closure finally (fix #415) #449

Merged
merged 8 commits into from
Oct 16, 2023
Merged

Conversation

Menduist
Copy link
Contributor

@Menduist Menduist commented Sep 26, 2023

Alternative to #418
Fixes #415

This moves the error handling to the closure (instead of the futureContinue). In the finally, we complete with result in case of success. Example:

proc test415: Future[int] {.async.} =
  return 5

# becomes (stripped the `when`s for clarity):
proc test415(): Future[int] {.stackTrace: off, gcsafe, raises: [].} =
  iterator test415_788529156(chronosInternalRetFuture: FutureBase): FutureBase {.
      closure, gcsafe, raises: [].} =
    var result {.used.}: int
    try:
       result = 5
       return nil
    except CancelledError:
      cancelAndSchedule(cast[Future[int]](chronosInternalRetFuture))
    except CatchableError as exc:
      fail(cast[Future[int]](chronosInternalRetFuture), exc)
    finally:
      if not finished(cast[Future[int]](chronosInternalRetFuture)):
        complete(cast[Future[int]](chronosInternalRetFuture), result)

  let resultFuture = newFuture[int]("test415")
  resultFuture.internalClosure = test415_788529156
  futureContinue(resultFuture)
  return resultFuture

@Menduist Menduist changed the title Complete in closure finally (fix #415) Complete futures in closure finally (fix #415) Sep 26, 2023
@Menduist Menduist mentioned this pull request Sep 27, 2023
@arnetheduck
Copy link
Member

lgtm but needs a round of testing with nimbus-eth2

@Menduist
Copy link
Contributor Author

Defects are now handled by failing the future with a new exception, similarly to other exceptions
Could also leave the future running, but since the chronosStrictException will become default, not sure it's worth the trouble

@arnetheduck
Copy link
Member

Could also leave the future running, but since the chronosStrictException will become default, not sure it's worth the trouble

let's leave the future running and propagate to poll like it was before

@Menduist
Copy link
Contributor Author

This is causing some regression in the libp2p CI, investigating..

@Menduist
Copy link
Contributor Author

Menduist commented Oct 3, 2023

Fixed the regression, I also took the opportunity to "improve" #401, since we now need a template to set the result anyway

The issue was:

    proc returnResult: Future[int] {.async.} =
      var result: int
      result = 12
      return result

was not working properly anymore, since return result was transformed to result = result; return nil
which was not setting the correct result

The fix is:

    var result {.used.}: int
    template setResult(code: untyped) {.used.} =
      result = code
    block:
       var result: int
       result = 12
       setResult(12)

since result is bound in the template, this will set the correct result.

And I took the opportunity to handle the implicit returns directly inside setResult:

    template setResult(code: untyped) {.used.} =
      when typeof(code) is void:
        code
      else:
        result = code

LMKWYT

@Menduist
Copy link
Contributor Author

Menduist commented Oct 3, 2023

libp2p & nimbus tests now pass with this branch

@arnetheduck arnetheduck merged commit 253bc3c into master Oct 16, 2023
12 checks passed
@arnetheduck arnetheduck deleted the fix415_v2 branch October 16, 2023 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

return statement in try/finally block breaks executing multiple await statements in finally block
2 participants