Corrections to timedwait methods (#35103)

* Refactor methods of sleep, Timer, and timedwait

Made argument names consistent with documentation where reasonable.

* Support pollint keyword in timedwait using Periods

* Allow timedwait to take Real

* Add tests for timedwait

* Fix bug with sub-millisecond pollint

* Test timedwait with Period parameters

* Rethrow exceptions raised by timedwait callback

Author: omus

base/asyncevent.jl

@@ -255,39 +255,45 @@ function Timer(cb::Function, timeout::Real; interval::Real=0.0)
 end
 
 """
-    timedwait(testcb::Function, secs::Float64; pollint::Float64=0.1)
+    timedwait(testcb::Function, timeout::Real; pollint::Real=0.1)
 
-Waits until `testcb` returns `true` or for `secs` seconds, whichever is earlier.
-`testcb` is polled every `pollint` seconds.
+Waits until `testcb` returns `true` or for `timeout` seconds, whichever is earlier.
+`testcb` is polled every `pollint` seconds. The minimum duration for `timeout` and `pollint`
+is 1 millisecond or `0.001`.
 
-Returns :ok, :timed_out, or :error
+Returns :ok or :timed_out
 """
-function timedwait(testcb::Function, secs::Float64; pollint::Float64=0.1)
-    pollint > 0 || throw(ArgumentError("cannot set pollint to $pollint seconds"))
+function timedwait(testcb::Function, timeout::Real; pollint::Real=0.1)
+    pollint >= 1e-3 || throw(ArgumentError("pollint must be ≥ 1 millisecond"))
     start = time_ns()
-    nsecs = 1e9 * secs
+    ns_timeout = 1e9 * timeout
     done = Channel(1)
     function timercb(aw)
         try
             if testcb()
-                put!(done, :ok)
-            elseif (time_ns() - start) > nsecs
-                put!(done, :timed_out)
+                put!(done, (:ok, nothing))
+            elseif (time_ns() - start) > ns_timeout
+                put!(done, (:timed_out, nothing))
             end
         catch e
-            put!(done, :error)
+            put!(done, (:error, CapturedException(e, catch_backtrace())))
         finally
             isready(done) && close(aw)
         end
         nothing
     end
 
-    if !testcb()
-        t = Timer(timercb, pollint, interval = pollint)
-        ret = fetch(done)::Symbol
-        close(t)
-    else
-        ret = :ok
+    try
+        testcb() && return :ok
+    catch e
+        throw(CapturedException(e, catch_backtrace()))
     end
+
+    t = Timer(timercb, pollint, interval = pollint)
+    ret, e = fetch(done)
+    close(t)
+
+    ret === :error && throw(e)
+
     return ret
 end

stdlib/Dates/src/types.jl

@@ -420,11 +420,15 @@ Base.hash(x::Time, h::UInt) =
     hash(hour(x), hash(minute(x), hash(second(x),
         hash(millisecond(x), hash(microsecond(x), hash(nanosecond(x), h))))))
 
-import Base: sleep, Timer, timedwait
-sleep(time::Period) = sleep(toms(time) / 1000)
-Timer(time::Period; interval::Period = Second(0)) =
-    Timer(toms(time) / 1000, interval = toms(interval) / 1000)
-timedwait(testcb::Function, time::Period) = timedwait(testcb, toms(time) / 1000)
+Base.sleep(duration::Period) = sleep(toms(duration) / 1000)
+
+function Base.Timer(delay::Period; interval::Period=Second(0))
+    Timer(toms(delay) / 1000, interval=toms(interval) / 1000)
+end
+
+function Base.timedwait(testcb::Function, timeout::Period; pollint::Period=Millisecond(100))
+    timedwait(testcb, toms(timeout) / 1000, pollint=toms(pollint) / 1000)
+end
 
 Base.OrderStyle(::Type{<:AbstractTime}) = Base.Ordered()
 Base.ArithmeticStyle(::Type{<:AbstractTime}) = Base.ArithmeticWraps()

stdlib/Dates/test/types.jl

@@ -266,4 +266,8 @@ end
 
 end
 
+@testset "timedwait" begin
+    @test timedwait(() -> false, Second(0); pollint=Millisecond(1)) === :timed_out
+end
+
 end

test/channels.jl

@@ -252,7 +252,47 @@ using Distributed
     end
 end
 
-using Dates
+@testset "timedwait" begin
+    @test timedwait(() -> true, 0) === :ok
+    @test timedwait(() -> false, 0) === :timed_out
+    @test_throws ArgumentError timedwait(() -> true, 0; pollint=0)
+
+    # Allowing a smaller positive `pollint` results in `timewait` hanging
+    @test_throws ArgumentError timedwait(() -> true, 0, pollint=1e-4)
+
+    # Callback passed in raises an exception
+    failure_cb = function (fail_on_call=1)
+        i = 0
+        function ()
+            i += 1
+            i >= fail_on_call && error("callback failed")
+            return false
+        end
+    end
+
+    try
+        timedwait(failure_cb(1), 0)
+        @test false
+    catch e
+        @test e isa CapturedException
+        @test e.ex isa ErrorException
+    end
+
+    try
+        timedwait(failure_cb(2), 0)
+        @test false
+    catch e
+        @test e isa CapturedException
+        @test e.ex isa ErrorException
+    end
+
+    duration = @elapsed timedwait(() -> false, 1)  # Using default pollint of 0.1
+    @test duration ≈ 1 atol=0.4
+
+    duration = @elapsed timedwait(() -> false, 0; pollint=1)
+    @test duration ≈ 1 atol=0.4
+end
+
 @testset "timedwait on multiple channels" begin
     @Experimental.sync begin
         rr1 = Channel(1)
@@ -262,13 +302,13 @@ using Dates
         callback() = all(map(isready, [rr1, rr2, rr3]))
         # precompile functions which will be tested for execution time
         @test !callback()
-        @test timedwait(callback, 0.0) === :timed_out
+        @test timedwait(callback, 0) === :timed_out
 
         @async begin sleep(0.5); put!(rr1, :ok) end
         @async begin sleep(1.0); put!(rr2, :ok) end
         @async begin sleep(2.0); put!(rr3, :ok) end
 
-        et = @elapsed timedwait(callback, Dates.Second(1))
+        et = @elapsed timedwait(callback, 1)
 
         # assuming that 0.5 seconds is a good enough buffer on a typical modern CPU
         try