MagicStack / uvloop Public
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
add support for <timer handle>.when() #428
Conversation
uvloop/handles/timer.pyx
Outdated
| cdef timespec ts | ||
|
|
||
| timespec_get(&ts, 1) | ||
| self.when = ts.tv_sec + ts.tv_nsec*1e-9 + self.timeout*1e-3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! I'd make this calculation on demand. Add a method to the Timer handle get_when(). Also we need tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok instead of calculating the thole thing there you're thinking I should cache the start time (as double instead of the timespec?) and then calculate off of the self.time in get_when, do I understand correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any update @1st1 ? Just looking for a little guidance before I reform my patch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay! My idea was to do as little work as possible in __cinit__ or __init__, offsetting the bulk of the work to when().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, so I need to at a minimum keep the current time I've sampled with timespec_get(...), so one option to avoid any calculation would be to just keep the timespec struct. I didn't go that route because it would require twice the storage as sizeof(timespec) == 16 vs. sizeof(self.when) == 8. Buf if it's your preference to cache the bytes and defer the calculation then I'll change it as you say. Is that your preference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should call loop.time() when the handle is created and store that time in the handle. The method returns monotonic time that's cached for the current iteration and that's what libuv is using internally to compute the timeouts. Then you'd simply need to add the timeout to that captured creation time in .when()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also using loop.time() is the only way to get consistency with loop.call_at().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sure, fair enough. That's certainly consistent with the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, pushed another commit. Works the way you mention and I added one test, not sure what other sort of testing will be worthwhile.
uvloop/cbhandles.pyx
Outdated
| @@ -308,6 +308,9 @@ cdef class TimerHandle: | |||
| def cancel(self): | |||
| self._cancel() | |||
|
|
|||
| def when(self): | |||
| return self.timer.get_when()*1e-3 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return self.timer.get_when()*1e-3 | |
| return self.timer.get_when() * 1e-3 |
|
ok, did that, so we're there?! :-D |
|
Tests seem to be failing in CI: https://github.com/MagicStack/uvloop/pull/428/checks?check_run_id=3292953606#step:7:2062 |


here's a simple implementation to add support for when() on a timer handle