Use time.monotonic() where available. #583

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
@apenwarr

This patch changes tornado to use time.monotonic() wherever it can, rather than time.time(). This allows it to survive better when the normal clock jumps around (because of NTP or other reasons).

time.monotonic() is part of python 3.3, but you can also get my implementation for earlier versions from here: http://pypi.python.org/pypi/Monotime/ . This patch causes tornado to try that implementation if time.monotonic() is not otherwise available.

apenwarr added some commits Aug 14, 2012

Disable tests when old versions of twisted are installed.
Twisted 10.0 (on an older version of Ubuntu) doesn't seem to work with class
decorators, even on python 2.6.  This makes the tests fail with a TypeError:

Traceback (most recent call last):
  File "tornado/test/import_test.py", line 59, in test_import_twisted
    import tornado.platform.twisted
  File "tornado/platform/twisted.py", line 108, in <module>
    TornadoDelayedCall = implementer(IDelayedCall)(TornadoDelayedCall)
  File "/usr/lib/python2.6/dist-packages/zope/interface/declarations.py",
line 496, in __call__
    raise TypeError("Can't use implementer with classes.  Use one of "
TypeError: Can't use implementer with classes.  Use one of the
class-declaration functions instead.

If we catch a typeerror while importing twisted, act like twisted is not
installed.
tornado: use time.monotonic() where available.
In ioloop.add_timeout(), we still support adding timeouts using time.time(),
but we actually convert them to time.monotonic() before running.  If you
don't explicitly set monotonic=False when calling add_timeout(), you'll get
a warning when that happens.

But mostly, you should really be passing in a datetime.timedelta object,
because almost always you want to use relative time instead of absolute
time.
@apenwarr

This comment has been minimized.

Show comment Hide comment
@apenwarr

apenwarr Aug 14, 2012

See also facebook#558

See also facebook#558

@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Aug 14, 2012

This pull request passes (merged a17cc80 into 4daeaeb).

This pull request passes (merged a17cc80 into 4daeaeb).

@bdarnell

This comment has been minimized.

Show comment Hide comment
@bdarnell

bdarnell Aug 14, 2012

Member

Cool. I like this change, although I think it's probably too aggressive in encouraging the use of monotonic time. Logging a warning for each use of add_timeout(int) that doesn't specify monotonic is excessive (also the code doesn't seem to distinguish between None and False like the docstring says). I also wouldn't log at startup if neither time.monotonic or monotime is available (using the logging module at import time can mess up the logging configuration. I know we do it in ioloop.py but at least in that case it's limited to installations that are somehow broken).

In pull request #558, the time function is an attribute of the IOLoop instead of a global, which would let you have one thread's IOLoop using monotonic time and another thread on time.time(). I can see where that would be useful but it is kind of esoteric.

I don't really like the monotonic argument to add_timeout - it does allow for both monotonic and time.time-based code to coexist on the same IOLoop, but the new argument is effectively mandatory, which will get annoying. I'd almost say that add_timeout(int) should always use time.time (for cron-like tasks, "run this at midnight"), even when add_timeout(timedelta) uses monotonic time (and most instances of add_timeout should be converted to the timedelta form).

For the accompanying twisted change, a TypeError on importing tornado.platform.twisted could be a bug in that module, so I think this is a case where explicit version checks are better than catching the exception. I believe 11.0.0 is the first twisted version we support (it's at least the oldest version still in the tox config).

Member

bdarnell commented Aug 14, 2012

Cool. I like this change, although I think it's probably too aggressive in encouraging the use of monotonic time. Logging a warning for each use of add_timeout(int) that doesn't specify monotonic is excessive (also the code doesn't seem to distinguish between None and False like the docstring says). I also wouldn't log at startup if neither time.monotonic or monotime is available (using the logging module at import time can mess up the logging configuration. I know we do it in ioloop.py but at least in that case it's limited to installations that are somehow broken).

In pull request #558, the time function is an attribute of the IOLoop instead of a global, which would let you have one thread's IOLoop using monotonic time and another thread on time.time(). I can see where that would be useful but it is kind of esoteric.

I don't really like the monotonic argument to add_timeout - it does allow for both monotonic and time.time-based code to coexist on the same IOLoop, but the new argument is effectively mandatory, which will get annoying. I'd almost say that add_timeout(int) should always use time.time (for cron-like tasks, "run this at midnight"), even when add_timeout(timedelta) uses monotonic time (and most instances of add_timeout should be converted to the timedelta form).

For the accompanying twisted change, a TypeError on importing tornado.platform.twisted could be a bug in that module, so I think this is a case where explicit version checks are better than catching the exception. I believe 11.0.0 is the first twisted version we support (it's at least the oldest version still in the tox config).

@apenwarr

This comment has been minimized.

Show comment Hide comment
@apenwarr

apenwarr Aug 14, 2012

Thanks for the feedback. I figured the warnings might be too aggressive,
but I thought I'd give it a try. It helped a lot when debugging the code
to find all the leftover uses of non-monotonic time, and it discourages
anyone from adding new non-monotonic timeouts without explicitly saying
that's what they wanted.

Note that if you supply a timedelta() (which is the safest option) you
don't have to set monotonic=anything. Again, that's some polite
encouragement to do it the right way.

The place where monotonic=True is needed is when setting periodic events.
Those are the ones most likely to be mangled by a jump in time.time(), but
to avoid losing a few milliseconds here and there, we also can't use a
timedelta(). I suppose we could have an add_timeout_monotonic() for that
purpose or something.

What if we print only a single warning the first time the monotonic=
parameter is omitted but time.monotonic() is available? That should
encourage people to double check their code.

Or maybe I can remove the warnings - most people aren't badly affected by
NTP time jumps. It just happens to be a serious problem in our application.

I can change the twisted fix to check version number instead, no problem
there.

Thoughts?

On Tue, Aug 14, 2012 at 12:29 AM, bdarnell notifications@github.com wrote:

Cool. I like this change, although I think it's probably too aggressive in
encouraging the use of monotonic time. Logging a warning for each use of
add_timeout(int) that doesn't specify monotonic is excessive (also the code
doesn't seem to distinguish between None and False like the docstring
says). I also wouldn't log at startup if neither time.monotonic or monotime
is available (using the logging module at import time can mess up the
logging configuration. I know we do it in ioloop.py but at least in that
case it's limited to installations that are somehow broken).

In pull request #558 facebook#558,
the time function is an attribute of the IOLoop instead of a global, which
would let you have one thread's IOLoop using monotonic time and another
thread on time.time(). I can see where that would be useful but it is kind
of esoteric.

I don't really like the monotonic argument to add_timeout - it does allow
for both monotonic and time.time-based code to coexist on the same IOLoop,
but the new argument is effectively mandatory, which will get annoying. I'd
almost say that add_timeout(int) should always use time.time (for cron-like
tasks, "run this at midnight"), even when add_timeout(timedelta) uses
monotonic time (and most instances of add_timeout should be converted to
the timedelta form).

For the accompanying twisted change, a TypeError on importing
tornado.platform.twisted could be a bug in that module, so I think this is
a case where explicit version checks are better than catching the
exception. I believe 11.0.0 is the first twisted version we support (it's
at least the oldest version still in the tox config).


Reply to this email directly or view it on GitHubhttps://github.com/facebook/tornado/pull/583#issuecomment-7715971.

Thanks for the feedback. I figured the warnings might be too aggressive,
but I thought I'd give it a try. It helped a lot when debugging the code
to find all the leftover uses of non-monotonic time, and it discourages
anyone from adding new non-monotonic timeouts without explicitly saying
that's what they wanted.

Note that if you supply a timedelta() (which is the safest option) you
don't have to set monotonic=anything. Again, that's some polite
encouragement to do it the right way.

The place where monotonic=True is needed is when setting periodic events.
Those are the ones most likely to be mangled by a jump in time.time(), but
to avoid losing a few milliseconds here and there, we also can't use a
timedelta(). I suppose we could have an add_timeout_monotonic() for that
purpose or something.

What if we print only a single warning the first time the monotonic=
parameter is omitted but time.monotonic() is available? That should
encourage people to double check their code.

Or maybe I can remove the warnings - most people aren't badly affected by
NTP time jumps. It just happens to be a serious problem in our application.

I can change the twisted fix to check version number instead, no problem
there.

Thoughts?

On Tue, Aug 14, 2012 at 12:29 AM, bdarnell notifications@github.com wrote:

Cool. I like this change, although I think it's probably too aggressive in
encouraging the use of monotonic time. Logging a warning for each use of
add_timeout(int) that doesn't specify monotonic is excessive (also the code
doesn't seem to distinguish between None and False like the docstring
says). I also wouldn't log at startup if neither time.monotonic or monotime
is available (using the logging module at import time can mess up the
logging configuration. I know we do it in ioloop.py but at least in that
case it's limited to installations that are somehow broken).

In pull request #558 facebook#558,
the time function is an attribute of the IOLoop instead of a global, which
would let you have one thread's IOLoop using monotonic time and another
thread on time.time(). I can see where that would be useful but it is kind
of esoteric.

I don't really like the monotonic argument to add_timeout - it does allow
for both monotonic and time.time-based code to coexist on the same IOLoop,
but the new argument is effectively mandatory, which will get annoying. I'd
almost say that add_timeout(int) should always use time.time (for cron-like
tasks, "run this at midnight"), even when add_timeout(timedelta) uses
monotonic time (and most instances of add_timeout should be converted to
the timedelta form).

For the accompanying twisted change, a TypeError on importing
tornado.platform.twisted could be a bug in that module, so I think this is
a case where explicit version checks are better than catching the
exception. I believe 11.0.0 is the first twisted version we support (it's
at least the oldest version still in the tox config).


Reply to this email directly or view it on GitHubhttps://github.com/facebook/tornado/pull/583#issuecomment-7715971.

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