Allow specifying the function to use to get the current timestamp #558

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@mgenti
Contributor

mgenti commented Jul 3, 2012

In our application where we use Tornado we routinely synchronize our system clock with an NTP server. Since this application is on an embedded device our system clock is very sloppy. If the time sync happens while the Tornado application is running, events may not run as expected. To help with that we have the IOLoop use a function that returns a timestamp that is monotonic.

This particular commit allows a user to specify the function to use to get the timestamp but by default uses time.time.

Pass in a timestamp function to use
When creating an IOLoop instance, allow specifying the function to use
to get the current timestamp.
@bdarnell

This comment has been minimized.

Show comment Hide comment
@bdarnell

bdarnell Jul 4, 2012

Member

This seems like a good idea, especially with the new monotonic clock functions coming in python 3.3 (pep 418). However, backwards compatibility is a problem - the absolute form of IOLoop.add_timeout won't work on an IOLoop that's using something other than time.time. In order to make this change we'd need to basically deprecate the common practice of add_timeout(time.time() + offset). We'd need to make sure that tornado itself uses the relative form where appropriate, and expose the time function from IOLoop so it can be used to construct absolute timestamps where needed (as in your change to PeriodicCallback). This sort of change would probably need to wait for tornado 3.0.

Member

bdarnell commented Jul 4, 2012

This seems like a good idea, especially with the new monotonic clock functions coming in python 3.3 (pep 418). However, backwards compatibility is a problem - the absolute form of IOLoop.add_timeout won't work on an IOLoop that's using something other than time.time. In order to make this change we'd need to basically deprecate the common practice of add_timeout(time.time() + offset). We'd need to make sure that tornado itself uses the relative form where appropriate, and expose the time function from IOLoop so it can be used to construct absolute timestamps where needed (as in your change to PeriodicCallback). This sort of change would probably need to wait for tornado 3.0.

@mgenti

This comment has been minimized.

Show comment Hide comment
@mgenti

mgenti Jul 5, 2012

Contributor

I would hope that any user that goes to the trouble to specify a different timestamp function would know not to call add_timeout with time.time(). ;)

Contributor

mgenti commented Jul 5, 2012

I would hope that any user that goes to the trouble to specify a different timestamp function would know not to call add_timeout with time.time(). ;)

@bdarnell

This comment has been minimized.

Show comment Hide comment
@bdarnell

bdarnell Jul 5, 2012

Member

Yes, but sometimes tornado calls add_timeout for you (e.g. in the httpclients), and when it does it currently uses time.time().

Member

bdarnell commented Jul 5, 2012

Yes, but sometimes tornado calls add_timeout for you (e.g. in the httpclients), and when it does it currently uses time.time().

@apenwarr

This comment has been minimized.

Show comment Hide comment
@apenwarr

apenwarr Aug 14, 2012

See also: facebook#583

See also: facebook#583

bdarnell added a commit that referenced this pull request Oct 1, 2012

Add time_func parameter to IOLoop, and make it possible to use time.m…
…onotonic.

This means that calls to IOLoop.add_timeout that pass a number must be
updated to use IOLoop.time instead of time.time.

There are still some places where we use time.time in the code, but they
are either places where wall time is desired, or non-critical deltas (e.g.
printing elapsed time at the end of a request).

Thanks to apenwarr and mgenti for pull requests and discussion relating to
this change. (#558 and #583)
@bdarnell

This comment has been minimized.

Show comment Hide comment
@bdarnell

bdarnell Oct 1, 2012

Member

I've committed a time.monotonic change drawing on both pull requests. Like mgenti's version, I added a time_func argument to IOLoop's constructor. Instead of a monotonic argument to add_timeout, I simply said that all calls to add_timeout must be relative to the IOLoop's clock. This is going to be harder to audit for, but since use of a monotonic clock is opt-in I don't think it's too unreasonable (but I'm open to feedback on this point; it's not too late to change). Thanks to both of you for the pull requests and feedback.

Member

bdarnell commented Oct 1, 2012

I've committed a time.monotonic change drawing on both pull requests. Like mgenti's version, I added a time_func argument to IOLoop's constructor. Instead of a monotonic argument to add_timeout, I simply said that all calls to add_timeout must be relative to the IOLoop's clock. This is going to be harder to audit for, but since use of a monotonic clock is opt-in I don't think it's too unreasonable (but I'm open to feedback on this point; it's not too late to change). Thanks to both of you for the pull requests and feedback.

@bdarnell bdarnell closed this Oct 1, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment