Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sleepAsync #17250

Closed
wants to merge 1 commit into from
Closed

Fix sleepAsync #17250

wants to merge 1 commit into from

Conversation

juancarlospaco
Copy link
Collaborator

@juancarlospaco juancarlospaco commented Mar 4, 2021

import asyncdispatch, times

proc main(a: float or int) {.async.} =
  let t = now()
  echo "BEFORE\T", t, '\T', a
  await sleepAsync(a)
  echo "AFTER\T", t - now(), '\n'

waitFor main(1)  # Control
waitFor main(0.0)
waitFor main(-0.0)
waitFor main(1.0)
waitFor main(-1.0)
waitFor main(-1.0)
waitFor main(NaN)
waitFor main(+Inf)
waitFor main(-Inf)
waitFor main(Inf)
  • Only package ws is failing CI. /c @treeform

@juancarlospaco juancarlospaco marked this pull request as ready for review March 4, 2021 05:07
@treeform
Copy link
Contributor

treeform commented Mar 4, 2021

So sleepAsync used to take a float? API change?

@treeform
Copy link
Contributor

treeform commented Mar 4, 2021

proc sleepAsync(ms: int): owned(Future[void])
    first type mismatch at position: 1
    required type for ms: int
    but expression '1000.0 * seconds' is of type: float
  
  expression: sleepAsync(1000.0 * seconds)

Really strange?

@ringabout ringabout requested a review from timotheecour March 5, 2021 03:47
@timotheecour
Copy link
Member

the proper fix IMO is timotheecour#617

we should have type safe API's that are self-documenting and hard to misuse by using values with time units, the only question is whether it should be TimeInterval or Duration.

My recommendation is to add an overload that takes a TimeInterval or Duration (we need to figure out which of those 2 makes sense).

then users can use sleepAsync(145.microseconds) or sleepAsync(initDuration(seconds = 2)) and everything is self documenting.

sleepAsync(1000.0 * seconds)
Really strange?

well that stems from this PR... i guess your point is that it's a breaking change.

note

current API seems to truncate at 1ms smallest resolution, but that shouldn't dictate API, a better implementation should honor wait times smaller than 1ms, which this PR doesn't account for.

@juancarlospaco juancarlospaco deleted the o_o branch March 5, 2021 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants