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

Use time.steady() to implement timeout #58430

Closed
tasslehoff mannequin opened this issue Mar 7, 2012 · 19 comments
Closed

Use time.steady() to implement timeout #58430

tasslehoff mannequin opened this issue Mar 7, 2012 · 19 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@tasslehoff
Copy link
Mannequin

tasslehoff mannequin commented Mar 7, 2012

BPO 14222
Nosy @rhettinger, @pitrou, @vstinner, @anacrolix
Files
  • queue_monotonic.patch
  • threading_monotonic.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/rhettinger'
    closed_at = <Date 2012-04-08.01:06:21.786>
    created_at = <Date 2012-03-07.19:38:39.928>
    labels = ['type-bug', 'library']
    title = 'Use time.steady() to implement timeout'
    updated_at = <Date 2014-02-25.22:26:22.207>
    user = 'https://bugs.python.org/tasslehoff'

    bugs.python.org fields:

    activity = <Date 2014-02-25.22:26:22.207>
    actor = 'vstinner'
    assignee = 'rhettinger'
    closed = True
    closed_date = <Date 2012-04-08.01:06:21.786>
    closer = 'rhettinger'
    components = ['Library (Lib)']
    creation = <Date 2012-03-07.19:38:39.928>
    creator = 'tasslehoff'
    dependencies = []
    files = ['24834', '24835']
    hgrepos = []
    issue_num = 14222
    keywords = ['patch']
    message_count = 19.0
    messages = ['155106', '155129', '155139', '155701', '155703', '155770', '155828', '155833', '156330', '157521', '157522', '157532', '157564', '157565', '157766', '157767', '157782', '212225', '212227']
    nosy_count = 8.0
    nosy_names = ['rhettinger', 'pitrou', 'vstinner', 'anacrolix', 'neologix', 'aljungberg', 'python-dev', 'tasslehoff']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = None
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue14222'
    versions = ['Python 3.3']

    @tasslehoff
    Copy link
    Mannequin Author

    tasslehoff mannequin commented Mar 7, 2012

    Queue.get([block[, timeout]]) uses time.time() for calculations of the timeout. If someone changes the system time this breaks. If a timeout of 1 second is set and someone (on a unix system) uses date to set the time 1 hour back, Queue.get will use 1 hour and 1 second to time out.

    I'm guessing the fix is just to use another library function to measure time, but I don't know if one exists that works on all platforms.

    @tasslehoff tasslehoff mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Mar 7, 2012
    @pitrou
    Copy link
    Member

    pitrou commented Mar 7, 2012

    Well, in 3.3 we could use clock_gettime(CLOCK_MONOTONIC) where available.
    That said, this is not specific to Queue.get() and will probably happen with many similar functions taking a timeout parameter.

    @vstinner
    Copy link
    Member

    vstinner commented Mar 8, 2012

    Well, in 3.3 we could use clock_gettime(CLOCK_MONOTONIC) where available.

    It's better to use time.monotonic().

    That said, this is not specific to Queue.get() and will probably happen with many similar functions taking a timeout parameter.

    Yep, it may be used for lock.acquire(timeout=timeout) for example. It
    would help to have a portable monotonic clock API in C.

    @vstinner
    Copy link
    Member

    queue_monotonic.patch: simple patch using time.monotonic(), with a fallback to time.time() if monotonic failed or is not available.

    @vstinner
    Copy link
    Member

    threading_monotonic.patch: similar patch for the threading module.

    Drawback of these patchs: they now call time.monotonic() when the module is loaded (another useless syscall?).

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Mar 14, 2012

    Random thoughts:

    • as noted by Antoine, it probably affects a lot of code throughout
      the standard library
    • sem_timedwait() (used by lock.acquire() on POSIX) is affected (the
      implementation using a condition variable could use
      pthread_condattr_setclock(), see issue bpo-12822)
    • there's a side effect to that change: on Linux, when the system is
      suspended, CLOCK_MONOTONIC stops: so on resume, you'd have to wait for
      the complete timeout to expire, whereas with time.time() you'll break
      out early

    That being said, it's probably a good idea.

    As for the patches, I don't really like the idea of having to use this
    idiom everywhere:

    try:
    from time import monotonic as _time
    _time()
    except (ImportError, OSError):
    from time import _time

    @rhettinger rhettinger self-assigned this Mar 14, 2012
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 15, 2012

    New changeset d97ae725814c by Victor Stinner in branch 'default':
    Issue bpo-14222: Use the new time.steady() function instead of time.time() for
    http://hg.python.org/cpython/rev/d97ae725814c

    @rhettinger
    Copy link
    Contributor

    Yo, I had marked this as something I was going to review once the python-dev discussion to complete. FWIW, I'm the maintainer of the Queue module.

    @vstinner
    Copy link
    Member

    Yo, I had marked this as something I was going to review once
    the python-dev discussion to complete. FWIW, I'm the maintainer
    of the Queue module.

    And so, do you agree with the change?

    @vstinner vstinner changed the title Using time.time() in Queue.get breaks when system time is changed Use time.steady() to implement timeout Mar 19, 2012
    @vstinner
    Copy link
    Member

    vstinner commented Apr 4, 2012

    Other modules that should use a monotonic clock instead of the system time:

    • timeit: default_timer = time.time (but time.clock is still the best clock on Windows for this mmodule)
    • subprocess for Popen.communicate() timeoout
    • sched
    • socket, ssl: timeout. need a C function which uses a monotonic clock if available, or fallback to the system clock
    • trace

    See also issues bpo-12338 and bpo-14309.

    @rhettinger
    Copy link
    Contributor

    I don't think *trace* and *timeit* should be changed.

    @rhettinger
    Copy link
    Contributor

    Why do you think monotonic time is needed for the Queue module? If time.time() goes backwards for some reason, the only consequence is that the timeouts take longer to cross the timeout boundard. On the other hand, it monotonic is used, then time won't go backwards but it won't go forwards either, so the consequence is the same.

    AFAICT, this patch doesn't fix any bug and doesn't make the module better in any observable way.

    @vstinner
    Copy link
    Member

    vstinner commented Apr 5, 2012

    Why do you think monotonic time is needed for the Queue module?

    The duration of a timeout of N seconds should be N seconds even if the system clock is updated (e.g. a daylight saving time (DST) change).

    This feature is checked by an unit test in the patch attached to the isuse bpo-14428 (implementation of the PEP-418):

    + def test_monotonic_settime(self):
    + t1 = time.monotonic()
    + realtime = time.clock_gettime(time.CLOCK_REALTIME)
    + # jump backward with an offset of 1 hour
    + time.clock_settime(time.CLOCK_REALTIME, realtime - 3600)
    + t2 = time.monotonic()
    + time.clock_settime(time.CLOCK_REALTIME, realtime)
    + # monotonic must not be affected by system clock updates
    + self.assertGreaterEqual(t2, t1)

    You can imagine a similar patch for Queue timeout.

    @vstinner
    Copy link
    Member

    vstinner commented Apr 5, 2012

    You can imagine a similar patch for Queue timeout.

    Oops: You can imagine a similar *test* for Queue timeout.

    @rhettinger
    Copy link
    Contributor

    [Victor]

    The duration of a timeout of N seconds should be N seconds even
    if the system clock is updated (e.g. a daylight saving time
    (DST) change).

    IIRC, time.time() is not a local time and it is unaffected by a DST change.

    @rhettinger
    Copy link
    Contributor

    I'm closing this one. The "not subject to adjustment" property is more desirable than not. The "undefined reference point" property isn't harmful in this context (the start time isn't exposed).

    I'll follow the python-dev thread and re-open this if it becomes clear that there are any issues for steady() being used for timeout logic.

    @pitrou
    Copy link
    Member

    pitrou commented Apr 8, 2012

    The "not subject to adjustment" property is more desirable than not.
    The "undefined reference point" property isn't harmful in this context
    (the start time isn't exposed).

    So you're closing the issue while you're in agreement with it?

    @aljungberg
    Copy link
    Mannequin

    aljungberg mannequin commented Feb 25, 2014

    This still appears to be an issue in Python 2.7. Queue.get routinely hangs for a very long time on the Raspberry Pi as it doesn't have a clock battery and often ends up significantly adjusting its system time soon after startup.

    @vstinner
    Copy link
    Member

    You should upgrade to python 3.3! The PEP-418 mentions different available
    modules for python 2. My new trollius project has for example an
    implementation in asyncio.time_monotonic.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants