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 a monotonic clock to compute timeouts #66242

Closed
vstinner opened this issue Jul 23, 2014 · 18 comments
Closed

Use a monotonic clock to compute timeouts #66242

vstinner opened this issue Jul 23, 2014 · 18 comments
Labels
type-feature A feature request or enhancement

Comments

@vstinner
Copy link
Member

BPO 22043
Nosy @abalkin, @vstinner
Files
  • pymonotonic.patch
  • pytimespec.patch
  • pymonotonic-2.patch
  • pymonotonic-3.patch
  • pymonotonic-4.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 = None
    closed_at = <Date 2014-09-17.22:17:19.598>
    created_at = <Date 2014-07-23.02:10:01.628>
    labels = ['type-feature']
    title = 'Use a monotonic clock to compute timeouts'
    updated_at = <Date 2014-09-17.22:17:19.596>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2014-09-17.22:17:19.596>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2014-09-17.22:17:19.598>
    closer = 'vstinner'
    components = []
    creation = <Date 2014-07-23.02:10:01.628>
    creator = 'vstinner'
    dependencies = []
    files = ['36041', '36176', '36177', '36504', '36513']
    hgrepos = []
    issue_num = 22043
    keywords = ['patch']
    message_count = 18.0
    messages = ['223715', '223716', '224365', '224390', '224392', '224393', '224456', '225969', '226064', '226065', '226066', '226067', '226175', '226283', '226284', '226301', '226302', '227016']
    nosy_count = 4.0
    nosy_names = ['belopolsky', 'vstinner', 'neologix', 'python-dev']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue22043'
    versions = ['Python 3.5']

    @vstinner
    Copy link
    Member Author

    Currently, socket methods (ex: accept, recv, recvfrom, recvmsg, send, sendto, sendmsg), threading.Lock.acquire() and threading.RLock.acquire() use the system clock to compute their timeout. It's fine for the first call. But if the call is interrupted and the timeout need to be recomputed, it can wait too short or too long. Timeouts must use a monotonic clock, not the system clock. See the PEP-418 for more information.

    Python modules were already patched to use the time.monotonic() function implemented in Python 3.3.

    Attached patch fixes also functions which still use the system clock to compute timeouts.

    A major change of the patch is that a monotonic clock is now require to use Python 3.5. Last time I checked, there was only one OS without monotonic clock: GNU Hurd. Hurd maintainers can patch Python 3.5 to fallback on the system clock until Hurd provides a monotonic clock.

    Another important change is that Python now depends on the librt on Solaris and on Linux with glibc older than 2.17 (clock_gettime is now available directly in the libc since glibc 2.17).

    @vstinner vstinner added the type-feature A feature request or enhancement label Jul 23, 2014
    @vstinner
    Copy link
    Member Author

    On FreeBSD and OpenBSD, clock_gettime() is directly available in the libc.

    @vstinner
    Copy link
    Member Author

    @vstinner
    Copy link
    Member Author

    pymonotonic.patch is large and difficult to review. I prefer to split it into two parts:

    • pytimespec.patch: Change pytime.h to use _PyTimeSpec structure (nanosecond resolution) instead of _PyTime_timeval structure (microsecond resolution)

    • pymonotonic-2.patch: Add _PyTimeSpec_monotonic() and _PyTimeSpec_monotonic_info()

    @vstinner
    Copy link
    Member Author

    pytimespec.patch removes *private* functions which were exposed in the stable ABI:

    • _PyTime_gettimeofday()
    • _PyTime_gettimeofday_info()

    It also removes private macros:

    • _PyTime_ADD_SECONDS()
    • _PyTime_INTERVAL()

    In pymonotonic.patch, I reused the same name but I didn't notice that they were part of the stable ABI. In pymonotonic-2.patch, I changed names so the dynamic loader will not load a module and the compiler will complain that symbols are missing.

    Replaced/renamed functions:

    • _PyTime_gettimeofday() => _PyTimeSpec_get_time()
    • _PyTime_gettimeofday_info() => _PyTimeSpec_get_time_info()
    • _PyTime_ADD_SECONDS() => _PyTimeSpec_add_sec()
    • _PyTime_INTERVAL() => _PyTimeSpec_interval_sec()

    I aslo added new functions:

    • _PyTimeSpec_add_us()
    • _PyTimeSpec_interval_us()

    On overflow, _PyTimeSpec_add_sec() and _PyTimeSpec_add_us() sets the timestamp to the maximum value instead of returning an undefined value. I prefer functions over macros because I want to return an error on overflow (even if it's not used).

    _PyTimeSpec_interval_us() also returns the maximum value in case of overflow.

    _PyTimeSpec_interval_sec() should maybe take a mandatory round parameter instead of always rounding up.

    I added _PyTime_unit_t for _threadmodule.c because acquire_timed() has a PY_TIMEOUT_T type which can be a long long. Maybe acquire_timed() can return an error if the timeout is too large, or loop? I would prefer to avoid this custom _PyTime_unit_t type.

    I should maybe add unit tests for _PyTimeSpec_add_() and _PyTimeSpec_interval_() functions, as I did for _PyLong_FromTime_t(), _PyTime_ObjectToTimeval(), etc.

    @vstinner
    Copy link
    Member Author

    Oh, pymonotonic-2.patch didn't get its "review" link because it depends on pytimespec.patch which is not merged yet.

    @vstinner
    Copy link
    Member Author

    pytimespec.patch become too large, I splitted this patch into a new issue: issue bpo-22117. If this issue is rejected, I will rewrite pymonotonic-2.patch to use the _PyTime_timeval structure.

    @vstinner
    Copy link
    Member Author

    To have an even smaller patch, I created the issue bpo-22287 just to add the dependency to the librt module in pytime.c.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 29, 2014

    New changeset 668e0bf30042 by Victor Stinner in branch 'default':
    Issue bpo-22043: _PyTime_Init() now checks if the system clock works.
    http://hg.python.org/cpython/rev/668e0bf30042

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 29, 2014

    New changeset 76bc15c918b1 by Victor Stinner in branch 'default':
    Issue bpo-22043: Simplify time.perf_counter() on Windows
    http://hg.python.org/cpython/rev/76bc15c918b1

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 29, 2014

    New changeset ab81b4cdc33c by Victor Stinner in branch 'default':
    Issue bpo-22043: Oops, fix perf_counter() on UNIX if no monotonic clock is
    http://hg.python.org/cpython/rev/ab81b4cdc33c

    @vstinner
    Copy link
    Member Author

    Ok, I prepared Python for monotonic clock, I attached an updated patch. It is now much simpler.

    pymonotonic-3.patch:

    • time.monotonic() is now always available

    • _PyTime_Init() ensures that the operating system provides a monotonic clock and that the clock works

    • Python 3.5 now requires a monotonic clock. All operating systems supported by Python 3.5 provides a monotonic clock. GNU Hurd doesn't, but it is not supported.

    • drop try/except ImportError around "from time import monotonic"

    • use a monotonic clock in _thread, gc and socket modules to compute elapsed time and timeouts

    @vstinner
    Copy link
    Member Author

    pymonotonic-4.patch: Updated patch (version 4) to address Antoine Pitrou's comments on Rietveld.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 2, 2014

    New changeset 330bd57685fc by Victor Stinner in branch 'default':
    Issue bpo-22043: Fix _PyTime_gettimeofday() if HAVE_GETTIMEOFDAY
    http://hg.python.org/cpython/rev/330bd57685fc

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 2, 2014

    New changeset b12857782041 by Victor Stinner in branch 'default':
    Issue bpo-22043: time.monotonic() is now always available
    http://hg.python.org/cpython/rev/b12857782041

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 3, 2014

    New changeset 9deef14393d5 by Victor Stinner in branch 'default':
    Issue bpo-22043: Fix pymonotonic(), use tv_usec=-1 as a marker to skip
    http://hg.python.org/cpython/rev/9deef14393d5

    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 3, 2014

    Thanks for the review Antoine. I pushed the new version pymonotonic-4.patch with a minor change: in debug mode, pymonotonic() also ensures that the clock never goes backward!

    @vstinner
    Copy link
    Member Author

    Buildbots are happy, I close the 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
    type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant