Corrections to `timedwait` methods #35103

omus · 2020-03-13T17:55:43Z

Mainly I wanted to update the timedwait function to be able to take Real instead of just Float64. During this process I found that the timedwait method which takes Period was missing the pollint keyword and I made some adjustments there as well.

The timedwait function currently doesn't have any tests so I'll probably also do that as part of this PR.

JeffBezanson · 2020-03-30T14:53:20Z

Bump

omus · 2020-04-01T14:27:14Z

I'll add some tests today

omus · 2020-04-01T18:49:07Z

Alright, tests have now been added. One thing to note is that timedwait now rethrows exceptions raised by the callback function instead of returning :error. The rational behind this change is that if the callback function would error on the first call then an exception would be raised instead of returning :error. This approach is now consistent and should provide details as to what the exception is.

Also note I've removed the seconds function as the changes I made probably need additional discussion. A quick overview of the issue was that toms truncates units smaller than milliseconds instead of fractional number.

omus · 2020-04-01T18:52:25Z

stdlib/Dates/src/types.jl

+
+function Base.timedwait(testcb::Function, timeout::Period; pollint::Period=Millisecond(100))
+ timedwait(testcb, toms(timeout) / 1000, pollint=toms(pollint) / 1000)
+end


I'll call out that toms truncates to milliseconds which means we're losing precision if a user passes in say Nanosecond(1) for either the timeout or pollint. However, since timedwait can only actually support millseconds this isn't really an issue.

This is one of those cases where two wrongs can make a right ;)

base/asyncevent.jl

JeffBezanson · 2020-04-01T19:07:27Z

base/asyncevent.jl

@@ -284,7 +285,8 @@ function timedwait(testcb::Function, secs::Float64; pollint::Float64=0.1)

 if !testcb()
 t = Timer(timercb, pollint, interval = pollint)
- ret = fetch(done)::Symbol
+ ret = fetch(done)
+ ret isa Exception && rethrow(ret)


I don't think we should rely on the type of e; can use a boolean flag instead.

How about if we store a Tuple{Symbol, Union{Nothing, Exception}} in the channel? Then this code would look like:

ret, e = fetch(done) ret === :error && rethrow(e)

omus · 2020-04-02T18:35:01Z

Looks like I need to increase my timing tolerance:

Test Failed at /Users/julia/buildbot/worker-tabularasa/tester_macos64/build/share/julia/test/channels.jl:268
  Expression: ≈(duration, 1, atol = 0.05)
   Evaluated: 1.182448603 ≈ 1 (atol=0.05)

omus · 2020-04-03T13:59:17Z

Tolerance is still too small:

Test Failed at /Users/julia/buildbot/worker-tabularasa/tester_macos64/build/share/julia/test/channels.jl:265
  Expression: ≈(duration, 1, atol = 0.2)
   Evaluated: 1.229485612 ≈ 1 (atol=0.2)
Error in testset channels:
Test Failed at /Users/julia/buildbot/worker-tabularasa/tester_macos64/build/share/julia/test/channels.jl:268
  Expression: ≈(duration, 1, atol = 0.2)
   Evaluated: 1.207294196 ≈ 1 (atol=0.2)

omus · 2020-04-03T18:21:26Z

I'm either really lucky or really unlucky:

Test Failed at /buildworker/worker/tester_linuxaarch64/build/share/julia/test/channels.jl:265
  Expression: ���(duration, 1, atol = 0.25)
   Evaluated: 1.252809605 ��� 1 (atol=0.25)
Error in testset channels:
Test Failed at /buildworker/worker/tester_linuxaarch64/build/share/julia/test/channels.jl:268
  Expression: ���(duration, 1, atol = 0.25)
   Evaluated: 1.297370699 ��� 1 (atol=0.25)

I'll go to a tolerance of 0.4

JeffBezanson · 2020-04-03T20:22:08Z

base/asyncevent.jl

@@ -284,7 +285,8 @@ function timedwait(testcb::Function, secs::Float64; pollint::Float64=0.1)

 if !testcb()
 t = Timer(timercb, pollint, interval = pollint)
- ret = fetch(done)::Symbol
+ ret, e = fetch(done)
+ ret === :error && rethrow(e)


rethrow doesn't work outside a catch block (it's throwing ERROR: rethrow(exc) not allowed outside a catch block) so you have to use throw.

Bad mistake on my part. Unfortunately this means that the exception thrown won't have the original stacktrace

Only if the callback raises an exception on the first call will generate the original stack trace. Exceptions raised after will not have the original stack trace. This seems better than silencing all exceptions from the callback.

Alternatively I can have timedwait return :error again and just capture exceptions from the first testcb call

You could capture the stack trace and store it in a CapturedException?

I've updated the code to always return a CapturedException

omus · 2020-04-06T18:04:46Z

CI failures look network related

Made argument names consistent with documentation where reasonable.

omus · 2020-04-07T19:23:02Z

Rebased changes

* 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

omus added domain:dates Dates, times, and the Dates stdlib module needs tests Unit tests are required for this change labels Mar 13, 2020

omus force-pushed the cv/timedwait-period-poll branch from d372b20 to 70d15d4 Compare April 1, 2020 18:42

omus commented Apr 1, 2020

View reviewed changes

omus removed the needs tests Unit tests are required for this change label Apr 1, 2020

JeffBezanson reviewed Apr 1, 2020

View reviewed changes

base/asyncevent.jl Outdated Show resolved Hide resolved

JeffBezanson reviewed Apr 1, 2020

View reviewed changes

omus force-pushed the cv/timedwait-period-poll branch from 70d15d4 to 34eb22b Compare April 2, 2020 16:20

JeffBezanson reviewed Apr 3, 2020

View reviewed changes

JeffBezanson closed this Apr 6, 2020

JeffBezanson reopened this Apr 6, 2020

omus added 9 commits April 7, 2020 14:22

Refactor methods of sleep, Timer, and timedwait

9d875ee

Made argument names consistent with documentation where reasonable.

Support pollint keyword in timedwait using Periods

2efb37a

Allow timedwait to take Real

2ed9668

Avoid using stdlib timedwait method

767fe7d

Add tests for timedwait

7629a26

Fix bug with sub-millisecond pollint

f5eab15

Test timedwait with Period parameters

7963f72

Rethrow exceptions raised by timedwait callback

d1896ae

Rename secs to timeout

906f6d2

omus force-pushed the cv/timedwait-period-poll branch from 39d4cd9 to 906f6d2 Compare April 7, 2020 19:22

JeffBezanson merged commit 241947f into master Apr 7, 2020

JeffBezanson deleted the cv/timedwait-period-poll branch April 7, 2020 23:52

KristofferC mentioned this pull request Apr 24, 2020

bump Pkg version #35584

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Corrections to `timedwait` methods #35103

Corrections to `timedwait` methods #35103

omus commented Mar 13, 2020

JeffBezanson commented Mar 30, 2020

omus commented Apr 1, 2020

omus commented Apr 1, 2020

omus Apr 1, 2020 •

edited

Loading

JeffBezanson Apr 1, 2020

omus Apr 1, 2020

omus commented Apr 2, 2020

omus commented Apr 3, 2020 •

edited

Loading

omus commented Apr 3, 2020

JeffBezanson Apr 3, 2020

omus Apr 6, 2020

omus Apr 6, 2020

omus Apr 6, 2020

iamed2 Apr 6, 2020

omus Apr 6, 2020

omus commented Apr 6, 2020

omus commented Apr 7, 2020

Corrections to timedwait methods #35103

Corrections to timedwait methods #35103

Conversation

omus commented Mar 13, 2020

JeffBezanson commented Mar 30, 2020

omus commented Apr 1, 2020

omus commented Apr 1, 2020

omus Apr 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omus commented Apr 2, 2020

omus commented Apr 3, 2020 • edited Loading

omus commented Apr 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omus commented Apr 6, 2020

omus commented Apr 7, 2020

Corrections to `timedwait` methods #35103

Corrections to `timedwait` methods #35103

omus Apr 1, 2020 •

edited

Loading

omus commented Apr 3, 2020 •

edited

Loading