From 4e2810cfe02b5f2daf45cceb2ce3a2bc0708ed38 Mon Sep 17 00:00:00 2001
From: Eugene Kabanov <eugene.kabanov@status.im>
Date: Mon, 6 Apr 2020 13:56:24 +0300
Subject: [PATCH] Fix issue with allFinished(), allFutures(), one() behavior
 when Futures passed are already finished. (#89)

Added test.
---
 chronos/asyncfutures2.nim | 22 ++++++++++++++-----
 tests/testfut.nim         | 45 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 5 deletions(-)

diff --git a/chronos/asyncfutures2.nim b/chronos/asyncfutures2.nim
index 40754b4..4e52a4d 100644
--- a/chronos/asyncfutures2.nim
+++ b/chronos/asyncfutures2.nim
@@ -693,10 +693,13 @@ proc allFutures*[T](futs: varargs[Future[T]]): Future[void] =
         nfuts[i].removeCallback(cb)
 
   for fut in nfuts:
-    fut.addCallback(cb)
+    if not(fut.finished()):
+      fut.addCallback(cb)
+    else:
+      inc(completedFutures)
 
   retFuture.cancelCallback = cancellation
-  if len(nfuts) == 0:
+  if len(nfuts) == 0 or len(nfuts) == completedFutures:
     retFuture.complete()
 
   return retFuture
@@ -730,10 +733,13 @@ proc allFinished*[T](futs: varargs[Future[T]]): Future[seq[Future[T]]] =
         fut.removeCallback(cb)
 
   for fut in nfuts:
-    fut.addCallback(cb)
+    if not(fut.finished()):
+      fut.addCallback(cb)
+    else:
+      inc(completedFutures)
 
   retFuture.cancelCallback = cancellation
-  if len(nfuts) == 0:
+  if len(nfuts) == 0 or len(nfuts) == completedFutures:
     retFuture.complete(nfuts)
 
   return retFuture
@@ -744,7 +750,7 @@ proc one*[T](futs: varargs[Future[T]]): Future[Future[T]] =
   ##
   ## If the argument is empty, the returned future FAILS immediately.
   ##
-  ## On success returned Future will hold index in ``futs`` array.
+  ## On success returned Future will hold finished Future[T].
   ##
   ## On cancel futures in ``futs`` WILL NOT BE cancelled.
   var retFuture = newFuture[Future[T]]("chronos.one()")
@@ -769,6 +775,12 @@ proc one*[T](futs: varargs[Future[T]]): Future[Future[T]] =
       if not(nfuts[i].finished()):
         nfuts[i].removeCallback(cb)
 
+  # If one of the Future[T] already finished we return it as result
+  for fut in nfuts:
+    if fut.finished():
+      retFuture.complete(fut)
+      return retFuture
+
   for fut in nfuts:
     fut.addCallback(cb)
 
diff --git a/tests/testfut.nim b/tests/testfut.nim
index 87c8536..ec76bb3 100644
--- a/tests/testfut.nim
+++ b/tests/testfut.nim
@@ -520,6 +520,21 @@ suite "Future[T] behavior test suite":
     # 10 * 10 completed futures = 100
     result += completedFutures
 
+  proc testAllCompleted(operation: int): bool =
+    proc client1(): Future[int] {.async.} =
+      result = 1
+
+    proc client2(): Future[int] {.async.} =
+      if true:
+        raise newException(ValueError, "")
+
+    if operation == 1:
+      var fut = allFutures(client1(), client2())
+      result = fut.finished() and not(fut.failed())
+    elif operation == 2:
+      var fut = allFinished(client1(), client2())
+      result = fut.finished() and not(fut.failed()) and (len(fut.read()) == 2)
+
   proc testOneZero(): bool =
     var tseq = newSeq[Future[int]]()
     var fut = one(tseq)
@@ -645,6 +660,30 @@ suite "Future[T] behavior test suite":
 
     result = true
 
+  proc testOneCompleted(): bool =
+    proc client1(): Future[int] {.async.} =
+      result = 1
+
+    proc client2(): Future[int] {.async.} =
+      if true:
+        raise newException(ValueError, "")
+
+    proc client3(): Future[int] {.async.} =
+      await sleepAsync(100.milliseconds)
+      result = 3
+
+    var f10 = client1()
+    var f20 = client2()
+    var f30 = client3()
+    var fut1 = one(f30, f10, f20)
+    var f11 = client1()
+    var f21 = client2()
+    var f31 = client3()
+    var fut2 = one(f31, f21, f11)
+
+    result = fut1.finished() and not(fut1.failed()) and fut1.read() == f10 and
+             fut2.finished() and not(fut2.failed()) and fut2.read() == f21
+
   proc testCancelIter(): bool =
     var completed = 0
 
@@ -854,6 +893,10 @@ suite "Future[T] behavior test suite":
     check testAllFuturesVarargs() == 30
   test "allFutures(varargs) test":
     check testAllFuturesSeq() == 300
+  test "allFutures() already completed test":
+    check testAllCompleted(1) == true
+  test "allFinished() already completed test":
+    check testAllCompleted(2) == true
 
   test "one(zero) test":
     check testOneZero() == true
@@ -861,6 +904,8 @@ suite "Future[T] behavior test suite":
     check testOneVarargs() == true
   test "one(seq) test":
     check testOneSeq() == true
+  test "one() already completed test":
+    check testOneCompleted() == true
 
   test "cancel() async procedure test":
     check testCancelIter() == true