status-im / nim-chronos

Chronos - An efficient library for asynchronous programming
https://status-im.github.io/nim-chronos/
Apache License 2.0
362 stars 51 forks source link

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

Closed Menduist closed 1 year ago

Menduist commented 1 year ago

Alternative to https://github.com/status-im/nim-chronos/pull/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
arnetheduck commented 1 year ago

lgtm but needs a round of testing with nimbus-eth2

Menduist commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

Menduist commented 1 year ago

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 commented 1 year ago

libp2p & nimbus tests now pass with this branch