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

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

Closed
wants to merge 1 commit into from

Conversation

mgenti
Copy link
Contributor

@mgenti 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.

When creating an IOLoop instance, allow specifying the function to use
to get the current timestamp.
@bdarnell
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link

See also: #583

bdarnell added a commit that referenced this pull request Oct 1, 2012
…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
Copy link
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants