search

status-im / nim-chronos

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

Finish closures before completing futures (fix #415) #418

Closed Menduist closed 11 months ago

Menduist commented 1 year ago

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 the return 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

arnetheduck commented 1 year ago

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

Menduist commented 1 year ago

This branch is failing on nimbus:

/home/tavon/Status/nimbus-eth2/beacon_chain/el/el_manager.nim(724, 18) template/generic instantiation of `err` from here
/home/tavon/Status/nimbus-eth2/beacon_chain/el/el_manager.nim(717, 42) Error: cannot instantiate Future [type declared in /home/tavon/Status/nimbus-eth2/vendor/nim-chronos/chronos/futures.nim(68, 3)]
got: <T>
but expected: <T>

Some generic fun to fix

EDIT: caused by https://github.com/nim-lang/Nim/issues/22645

arnetheduck commented 1 year ago

this should have a test that shows the previously broken code snippet

Menduist commented 1 year ago

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 result variable.

arnetheduck commented 11 months ago

Superseded by #449