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

`withTimeout` doesn't wait for cancellation to be complete #334

Closed etan-status closed 1 year ago

etan-status commented 1 year ago

When cancelling the future returned from withTimeout, the inner future is cancelled (as if the timeout was hit). However, the cancellation may be incomplete. This may lead to unexpected behaviour, as cleanup logic may get triggered with a delay.

Example:

import chronos

proc a() {.async.} =
  try:
    await sleepAsync(seconds(1))
  except CatchableError as exc:
    echo "A"
    raise exc

proc b() {.async.} =
  try:
    await a()
  except CatchableError as exc:
    echo "B"
    raise exc

proc c() {.async.} =
  try:
    echo $(await b().withTimeout(seconds(2)))
  except CatchableError as exc:
    echo "C"
    raise exc

let x = c()
x.cancel()
try:
  waitFor x
except CatchableError:
  echo "D"
echo "E"
waitFor sleepAsync(milliseconds(50))

Output:

A
C
D
E
B

Expected:

A
B
C
D
E

Nim versions:

Chronos code (asyncloop.nim)

  proc cancellation(udata: pointer) {.gcsafe, raises: [Defect].} =
    if not isNil(timer):
      clearTimer(timer)
    if not(fut.finished()):
      fut.removeCallback(continuation)
      fut.cancel()          ## Here Chronos doesn't wait for cancellation to complete

Workaround:

Using waitFor fut.cancelAndWait() at highlighted line above works around the problem, but leads to nested waitFor (and requires converting CatchableError to Defect), so is not a clean fix.

cheatfate commented 1 year ago

Fixed in https://github.com/status-im/nim-chronos/commit/2e8551b0d973cfbebfab3be7f3329e11b9049007