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

Jump to bottom

threading.Lock.acquire(timeout) should use sem_clockwait(CLOCK_MONOTONIC) #85876

Closed
wocket mannequin opened this issue Sep 4, 2020 · 22 comments
Closed

threading.Lock.acquire(timeout) should use sem_clockwait(CLOCK_MONOTONIC) #85876

wocket mannequin opened this issue Sep 4, 2020 · 22 comments
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@wocket
Copy link
Mannequin

wocket mannequin commented Sep 4, 2020

BPO 41710
Nosy @vstinner, @methane, @miss-islington, @mikecrowe
PRs
  • bpo-41710: Add _PyTime_AsTimespec_clamp() #28629
  • bpo-41710: Add pytime_add() and pytime_mul() #28642
  • bpo-41710: PyThread_acquire_lock_timed() clamps the timout #28643
  • bpo-41710: Fix building pytime.c on Windows #28644
  • bpo-41710: Document _PyTime_t API in pytime.h #28647
  • bpo-41710: PyThread_acquire_lock_timed() uses sem_clockwait() #28662
  • [3.10] bpo-41710: PyThread_acquire_lock_timed() uses sem_clockwait() #28671
  • [3.10] bpo-41710: Fix PY_TIMEOUT_MAX value on Windows #28672
  • bpo-41710: Fix PY_TIMEOUT_MAX on Windows #28673
  • bpo-41710: Add private _PyDeadline_Get() function #28674
  • bpo-41710: gc_collect_main() uses _PyTime_GetPerfCounter() #28676
  • [3.9] bpo-41710: PyThread_acquire_lock_timed() uses sem_clockwait() (GH-28671) #28683
  • bpo-41710: Fix What's New Entry credit #28962
    		•
    Files
  • time_test.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 2021-10-01.16:52:29.583>
created_at = <Date 2020-09-04.08:34:54.835>
labels = ['interpreter-core', 'type-bug', '3.9', '3.10', '3.11']
title = 'threading.Lock.acquire(timeout) should use sem_clockwait(CLOCK_MONOTONIC)'
updated_at = <Date 2021-12-06.13:27:11.081>
user = 'https://bugs.python.org/wocket'

    bugs.python.org fields:

    activity = <Date 2021-12-06.13:27:11.081>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2021-10-01.16:52:29.583>
closer = 'vstinner'
components = ['Interpreter Core']
creation = <Date 2020-09-04.08:34:54.835>
creator = 'wocket'
dependencies = []
files = ['49440']
hgrepos = []
issue_num = 41710
keywords = ['patch']
message_count = 22.0
messages = ['376338', '376340', '376394', '399405', '402925', '402926', '402931', '402939', '402942', '402987', '402995', '402997', '403001', '403003', '403009', '403011', '403012', '403021', '403022', '403023', '403962', '407802']
nosy_count = 5.0
nosy_names = ['vstinner', 'methane', 'miss-islington', 'wocket', 'mikecrowe']
pr_nums = ['28629', '28642', '28643', '28644', '28647', '28662', '28671', '28672', '28673', '28674', '28676', '28683', '28962']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue41710'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']
    @wocket
    Copy link
    Mannequin Author

    wocket mannequin commented Sep 4, 2020

    The timeout for threading.Lock, threading.Condition, etc, is not using a monotonic clock — it is affected if the system time (realtime clock) is set.

    The attached program can be used to show the problem. It is expected to print "Took 2.000 s" repeatedly, but if run with permissions to set the system time, it prints:
    $ sudo ./time_test.py
    Took 2.400 s
    Took 1.657 s
    Took 2.044 s
    Took 2.401 s
    ...

    (the 1.6 s time can be explained by NTP correcting the clock)

    There are already a number of closed bugs for this and related issues: bpo-23428, bpo-31267, bpo-35747.

    This happens in Python 3.7.7 (ARM32, Yocto Warrior), Python 3.8.2 (AMD64, Ubuntu Linux 20.04) and Python v3.9.0rc1 (AMD64, Ubuntu Linux 20.04).

    @wocket wocket mannequin added 3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Sep 4, 2020
    @vstinner
    Copy link
    Member

    vstinner commented Sep 4, 2020

    bpo-23428 modified the pthread implementation of conditional variable to use pthread_condattr_setclock(&ca, CLOCK_MONOTONIC) is available: commit 001fee1. The change should be part of Python 3.8.

    What is your sys.thread_info value? Example on Fedora 32 with Python 3.8.5:

    >>> sys.thread_info
sys.thread_info(name='pthread', lock='semaphore', version='NPTL 2.31')

    Sadly, the semaphore implementation doesn't use monotonic clock. See glibc issues:

    @wocket
    Copy link
    Mannequin Author

    wocket mannequin commented Sep 4, 2020 

    sys.thread_info = sys.thread_info(name='pthread', lock='semaphore', version='NPTL 2.31') on my system. Looking at the source I think the semaphore implementation will be used on all modern Linux systems.

    In my tests it works as expected on a Macintosh (3.8.5 with lock='mutex+cond') and also if I force a Linux build to use the mutex+cond implementation by defining HAVE_BROKEN_POSIX_SEMAPHORES.

    Doesn't look like those glibc and Linux bug reports will get any attention anytime soon. I will find a workaround instead :-/

    @mikecrowe
    Copy link
    Mannequin

    mikecrowe mannequin commented Aug 11, 2021

    glibc v2.30 onwards provides sem_clockwait which can wait on either CLOCK_MONOTONIC or CLOCK_REALTIME. I failed to notice that https://sourceware.org/bugzilla/show_bug.cgi?id=14717 existed until today. :(

    @vstinner vstinner changed the title Timeout is affected by jumps in system time acquire(timeout) of threading.Lock and threading.Condition is affected by jumps in system time: Python should use sem_clockwait(CLOCK_MONOTONIC) Sep 13, 2021
    @vstinner vstinner changed the title Timeout is affected by jumps in system time acquire(timeout) of threading.Lock and threading.Condition is affected by jumps in system time: Python should use sem_clockwait(CLOCK_MONOTONIC) Sep 13, 2021
    @vstinner
    Copy link
    Member

    New changeset 09796f2 by Victor Stinner in branch 'main':
    bpo-41710: Add _PyTime_AsTimespec_clamp() (GH-28629)
    09796f2

    @vstinner
    Copy link
    Member

    New changeset d62d925 by Victor Stinner in branch 'main':
    bpo-41710: Add pytime_add() and pytime_mul() (GH-28642)
    d62d925

    @vstinner
    Copy link
    Member

    New changeset 0231b6d by Victor Stinner in branch 'main':
    bpo-41710: Fix building pytime.c on Windows (GH-28644)
    0231b6d

    @vstinner
    Copy link
    Member

    New changeset 37b8294 by Victor Stinner in branch 'main':
    bpo-41710: PyThread_acquire_lock_timed() clamps the timout (GH-28643)
    37b8294

    @vstinner
    Copy link
    Member

    New changeset b34dd58 by Victor Stinner in branch 'main':
    bpo-41710: Document _PyTime_t API in pytime.h (GH-28647)
    b34dd58

    @vstinner
    Copy link
    Member

    On Unix, PyCOND_TIMEDWAIT() is implemented with pthread_cond_timedwait(). If pthread_condattr_setclock() is available, it uses CLOCK_MONOTONIC. Otherwise, it uses CLOCK_REALTIME.

    The glibc 2.30 adds pthread_cond_clockwait() which could be used to use CLOCK_MONOTONIC. But if pthread_cond_clockwait() is available (glibc 2.30 or newer), it expects that pthread_condattr_setclock() is also available. So I'm not sure that it's worth it to change PyCOND_TIMEDWAIT().

    See the _PyThread_cond_after() function which computes an absolute timestamp (timespec) from a relative timeout in microseconds.

    @mikecrowe
    Copy link
    Mannequin

    mikecrowe mannequin commented Oct 1, 2021

    vstinner wrote:

    The glibc 2.30 adds pthread_cond_clockwait() which could be used to use
    CLOCK_MONOTONIC. But if pthread_cond_clockwait() is available (glibc
    2.30 or newer), it expects that pthread_condattr_setclock() is also
    available. So I'm not sure that it's worth it to change
    PyCOND_TIMEDWAIT().

    That's correct. The only time that pthread_cond_clockwait is essential is if you don't know which clock you're going to need to use for the wait at the time of condition variable creation. (This is true for a generic condition variable wrapper that can be passed absolute timeouts using different clocks like the C++ std::condition_variable.)

    However, sem_clockwait is the only way to wait on a semaphore using CLOCK_MONOTONIC, so it is worth using that rather than sem_timedwait if it's available. Similarly for pthread_mutex_clocklock.

    @vstinner
    Copy link
    Member

    vstinner commented Oct 1, 2021

    New changeset 1ee0f94 by Victor Stinner in branch 'main':
    bpo-41710: PyThread_acquire_lock_timed() uses sem_clockwait() (GH-28662)
    1ee0f94

    @vstinner
    Copy link
    Member

    vstinner commented Oct 1, 2021

    With sem_clockwait(CLOCK_MONOTONIC) on Fedora 34 (glibc-2.33-20.fc34.x86_64, Linux kernel 5.13.19-200.fc34.x86_64), time_test.py now works as expected:

    $ sudo ./python time_test.py
Took 2.000 s
Took 2.000 s
Took 2.000 s
Took 2.000 s
Took 2.000 s
Took 2.000 s
(...)

    @vstinner vstinner added 3.11 only security fixes and removed 3.8 only security fixes labels Oct 1, 2021
    @vstinner vstinner changed the title acquire(timeout) of threading.Lock and threading.Condition is affected by jumps in system time: Python should use sem_clockwait(CLOCK_MONOTONIC) threading.Lock.acquire(timeout) should use sem_clockwait(CLOCK_MONOTONIC) Oct 1, 2021
    @vstinner vstinner added the 3.11 only security fixes label Oct 1, 2021
    @vstinner
    Copy link
    Member

    vstinner commented Oct 1, 2021

    There are already a number of closed bugs for this and related issues: bpo-23428, bpo-31267, bpo-35747.

    Fixed bpo-12822 modified threading.Condition.wait(timeout) to use a monotonic clock: use pthread_condattr_setclock(CLOCK_MONOTONIC). bpo-23428, bpo-31267 and bpo-35747 are duplicates of bpo-12822.

    This issue is about threading.Lock.acquire(timeout) which has a different implementation.

    @vstinner
    Copy link
    Member

    vstinner commented Oct 1, 2021

    New changeset 98d2827 by Victor Stinner in branch 'main':
    bpo-41710: Fix PY_TIMEOUT_MAX on Windows (GH-28673)
    98d2827

    @vstinner
    Copy link
    Member

    vstinner commented Oct 1, 2021

    New changeset 54957f1 by Victor Stinner in branch 'main':
    bpo-41710: gc_collect_main() uses _PyTime_GetPerfCounter() (GH-28676)
    54957f1

    @vstinner
    Copy link
    Member

    vstinner commented Oct 1, 2021

    New changeset 833fdf1 by Victor Stinner in branch 'main':
    bpo-41710: Add private _PyDeadline_Get() function (GH-28674)
    833fdf1

    @vstinner
    Copy link
    Member

    vstinner commented Oct 1, 2021

    New changeset 6df8c32 by Victor Stinner in branch '3.10':
    bpo-41710: PyThread_acquire_lock_timed() uses sem_clockwait() (GH-28671)
    6df8c32

    @vstinner
    Copy link
    Member

    vstinner commented Oct 1, 2021

    New changeset 0e1aeab by Miss Islington (bot) in branch '3.9':
    bpo-41710: PyThread_acquire_lock_timed() uses sem_clockwait() (GH-28671) (GH-28683)
    0e1aeab

    @vstinner
    Copy link
    Member

    vstinner commented Oct 1, 2021

    Jonas Norling: Thanks for the bug report! It's now fixed in 3.9, 3.10 and main branches.

    @vstinner vstinner added 3.9 only security fixes 3.10 only security fixes labels Oct 1, 2021
    @vstinner vstinner closed this as completed Oct 1, 2021
    @vstinner
    Copy link
    Member

    New changeset 03bbc60 by Victor Stinner in branch 'main':
    bpo-41710: Fix What's New Entry credit (GH-28962)
    03bbc60

    @vstinner
    Copy link
    Member

    vstinner commented Dec 6, 2021

    bpo-33632 "undefined behaviour: signed integer overflow in threadmodule.c" has been fixed by this change:

    New changeset 833fdf1 by Victor Stinner in branch 'main':
    bpo-41710: Add private _PyDeadline_Get() function (GH-28674)
    833fdf1

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @aludwig98 aludwig98 mentioned this issue May 19, 2023
    Open
    vstinner added a commit to vstinner/cpython that referenced this issue Nov 17, 2023
    @vstinner
    pythongh-108724: Use _PyTime_GetMonotonicClock() in parking_lot.c
    71a000c 
    Using a system clock causes issues when it's being updated by the
system administrator, NTP, Daylight Saving Time, or anything
else. Sadly, on Windows, the monotonic clock resolution is around
15.6 ms.

Example: python#85876
    @vstinner vstinner mentioned this issue Nov 17, 2023
    Closed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.9 only security fixes 3.10 only security fixes 3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    1 participant
    @vstinner