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

bug# 1607041: Condition.wait timeout fails on clock change #44296

Closed
hashstat mannequin opened this issue Dec 1, 2006 · 14 comments
Closed

bug# 1607041: Condition.wait timeout fails on clock change #44296

hashstat mannequin opened this issue Dec 1, 2006 · 14 comments
Labels
stdlib Python modules in the Lib dir

Comments

@hashstat
Copy link
Mannequin

hashstat mannequin commented Dec 1, 2006

BPO 1607149
Nosy @loewis, @rhettinger, @josiahcarlson
Files
  • threading.py.patch: Patch for Condition.wait in threading.py
  • 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 = None
    closed_at = <Date 2007-04-13.20:38:21.000>
    created_at = <Date 2006-12-01.22:32:21.000>
    labels = ['library']
    title = 'bug# 1607041: Condition.wait timeout fails on clock change'
    updated_at = <Date 2007-04-13.20:38:21.000>
    user = 'https://bugs.python.org/hashstat'

    bugs.python.org fields:

    activity = <Date 2007-04-13.20:38:21.000>
    actor = 'loewis'
    assignee = 'none'
    closed = True
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2006-12-01.22:32:21.000>
    creator = 'hashstat'
    dependencies = []
    files = ['7631']
    hgrepos = []
    issue_num = 1607149
    keywords = ['patch']
    message_count = 14.0
    messages = ['51423', '51424', '51425', '51426', '51427', '51428', '51429', '51430', '51431', '51432', '51433', '51434', '51435', '51436']
    nosy_count = 4.0
    nosy_names = ['loewis', 'rhettinger', 'josiahcarlson', 'hashstat']
    pr_nums = []
    priority = 'normal'
    resolution = 'rejected'
    stage = None
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue1607149'
    versions = ['Python 2.4']

    @hashstat
    Copy link
    Mannequin Author

    hashstat mannequin commented Dec 1, 2006

    This patch if for bug# 1607041.

    If the system clock is adjusted after Condition.wait is called with a timeout, the timeout does not expire as expected. This appears to be due to the threading.Condition class using the system clock to calculate the timeout expiration without taking system clock changes into account.

    No matter what timeout is used, setting the system clock ahead reduces or eliminates the wait while setting the system clock back increases the wait. So if the clock is set back one hour in the middle of a 1 microsecond wait (c.wait(1)), wait will return in an hour and 1 microsecond rather than after 1 microsecond.

    This patch modifies the Condition classes wait method to check for variations in the clock between calls to sleep and ajust for abnormalities.

    @hashstat hashstat mannequin closed this as completed Dec 1, 2006
    @hashstat hashstat mannequin added the stdlib Python modules in the Lib dir label Dec 1, 2006
    @hashstat hashstat mannequin closed this as completed Dec 1, 2006
    @hashstat hashstat mannequin added the stdlib Python modules in the Lib dir label Dec 1, 2006
    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Dec 3, 2006

    It would be better if this was based on a monotonic clock if available on the system (such as the POSIX CLOCK_MONOTONIC argument to clock_gettime). "Guessing" jumps in the time is inherently unreliable; in your code, you won't notice changes that involve less than 10s.

    Notice that the same problem exists for Thread.join; it would be good if they were fixed together.

    @josiahcarlson
    Copy link
    Mannequin

    josiahcarlson mannequin commented Feb 10, 2007

    Really, it would be better if time.sleep was based off of a monotonic clock, then everything could use the single working API, and we wouldn't need to monkey-patch everything that uses sleep.

    Unfortunately, due to the insanity that is the underlying Windows platform, the bios clock is set to local time and is converted to UTC (which is then used natively by NT). There is a work-around involving setting the system clock to UTC and setting a registry setting, but it doesn't always work, can cause 100% CPU usage during DST changes, and doesn't propagate time changes from NT to the underlying clock.

    http://www.cl.cam.ac.uk/~mgk25/mswish/ut-rtc.html

    An alternate (or additional option) is to just disable DST changes, manually adjust times as necessary (or using a script that runs only at boot), and never run any software while adjusting the time.

    http://www.pcreview.co.uk/forums/thread-531048-2.php

    Considering the recent changes to DST, the latter link is probably more applicable.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Feb 10, 2007

    DST is irrelevant for this discussion. On WIndows NT+, the system clock (as returned by libc ftime/windows GetSystemTime) is always in UTC (broken down to a Gregorian date). It is thus not affected by DST changes.

    What does matter for this bug is administrator changes to the system time (like manual changes), and time adjustments due to NTP (although the Windows NTP implementation will perform a non-monototic change only during boot, AFAIK).

    @josiahcarlson
    Copy link
    Mannequin

    josiahcarlson mannequin commented Feb 10, 2007

    Hrm. I know I've had issues with file modification times "changing" during a DST change on Windows in a Python process, but that may be due to Python only checking the timezone at startup.

    What you are saying sounds suspiciously like, "don't change the system clock when your program is running", which seems reasonable to me.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Feb 11, 2007

    Indeed, you would have issues with file stamps changing on Windows NT+ if the file system is FAT. However, this is not because the system time moves forth and back, but because FAT time stamps get reinterpreted according to the current time zone, not according to the time zone in force when the file was created (as NT tries to convert the FAT time stamps to UTC). On NTFS, this is not an issue, as NTFS file stamps are in UTC already (units of 100ns since Jan 1, 1601).

    So indeed, the submitter complains that Condition.wait doesn't time out correctly if the system clock is changed while the wait occurs. I think it is reasonable to make this complaint, however, I don't like to the patch proposed, and I do think this can't really be fixed without the help of the operating system (so I wouldn't mind if it was unfixed on systems that don't help). The point is that .wait() really takes a duration, not a time stamp, for timeout, and we should honor that. The duration should expire after the given amount of physical time has passed, independent of the system clock. As I said, one way to implement this would be to use a monotonic clock if the system has one.

    @josiahcarlson
    Copy link
    Mannequin

    josiahcarlson mannequin commented Feb 11, 2007

    I've never run FAT in Windows 2000. While I am pretty sure that the underlying platform doesn't have issues, Python keeps its DST offsets when it first starts up, so if you have a Python process that persists across DST changes, the modification time changes, even if the actual file modification time didn't (quitting and restarting Python "fixes" the issue).

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Feb 11, 2007

    Ah. I fixed the "changing stat result problem" in Python 2.5, by not using the C library's stat implementation anymore.

    @rhettinger
    Copy link
    Contributor

    I think we should punt on this one. When the clock changes, it is hard to detect when it happens, how much it changed, what the interactions are with other time dependent routines (logging, etc), and what to do about it. Much of this is pure guesswork.

    It would be different if there we some API in-place for clock changes to notify all of its clients when a change occurs. But there isn't. The responsibility for handling clock changes has to lie with the changer, not with all possible clients that call time() or sleep.

    Recommend closing as WontFix.

    @hashstat
    Copy link
    Mannequin Author

    hashstat mannequin commented Apr 13, 2007

    I admit that the patch isn't ideal. But it is a simple approach to fix the issue using pure Python. A better solution would be to use synchronization routines provided by the OS, such as the pthread condition functions on *NIX systems and the Win32 API synchronization functions on Windows.

    I wrote this patch to use with a simulator that may run for months at a time and will definitely run for days at a time. If the run happens to occur when DST ends and the clock automatically changes, as is the case for most systems, the simulation could stall for one hour and produce undesirable results.

    Having a sleeps depend on the wall clock is a bad idea. But since that is what we are working with, wouldn't it be better to have results skewed by a fraction of a minute or even a second rather than be off by an hour?

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Apr 13, 2007

    Please let me repeat that DST changes are *not* an issue. If the system runs for many months, and has a condition timeout set that passes the DST change boundary, the condition *will* work correctly in the code as is. I don't know where you got the idea that DST changes are a problem, please rest ensured that they are no problem whatsoever, for the issue under discussion. The clock does *not* change when the time zone changes. The timeout will *not* be off by an hour.

    Rejecting the patch as "won't fix".

    @hashstat
    Copy link
    Mannequin Author

    hashstat mannequin commented Apr 13, 2007

    You are right loewis. DST does not have any effect on the wait time. I haven't looked at this in a while and I shot off a reply too quickly. The point is that a sleep (or wait) of any sort should not depend on the wall clock. While I can undertand rejecting this patch, the underlying bug still exists and I hope a solution can be provided. I plan to look into a better fix myself. Thanks.

    @rhettinger
    Copy link
    Contributor

    This needs to stay closed. It not the responsibility of the Condition code to make guesses about whether the clock has changed, by how much, and what to do about it.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Apr 13, 2007

    I agree with hashstat that this is a bug. The application specified a time-out as a duration, not as a point in time. A duration is independent of what the clock says: 10s are 10s, even if the clock gets adjusted.

    The proper fix would be that locks themselves get a time-out, so that Condition.wait could pass the timeout on to waiter.acquire. For thread_nt.h, the timeout could get passed to WaitForSingleObject; for thread_pthread, it could get passed to sem_timedwait (if available). Not sure about the other threading libraries.

    In any case, *this* issue needs to stay closed. If anybody can provide an alternative solution, please submit a new issue.

    @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
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant