Sitelet https://web.archive.org/web/20220203074608/https://github.com/python/cpython/pull/28671
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

[3.10] bpo-41710: PyThread_acquire_lock_timed() uses sem_clockwait() #28671

Merged
merged 1 commit into from Oct 1, 2021

Conversation

@vstinner
Copy link
Member

@vstinner vstinner commented Oct 1, 2021 •

On Unix, if the sem_clockwait() function is available in the C
library (glibc 2.30 and newer), the threading.Lock.acquire() method
now uses the monotonic clock (time.CLOCK_MONOTONIC) for the timeout,
rather than using the system clock (time.CLOCK_REALTIME), to not be
affected by system clock changes.

configure now checks if the sem_clockwait() function is available.

https://bugs.python.org/issue41710

On Unix, if the sem_clockwait() function is available in the C
library (glibc 2.30 and newer), the threading.Lock.acquire() method
now uses the monotonic clock (time.CLOCK_MONOTONIC) for the timeout,
rather than using the system clock (time.CLOCK_REALTIME), to not be
affected by system clock changes.

configure now checks if the sem_clockwait() function is available.
@vstinner
Copy link
Member Author

@vstinner vstinner commented Oct 1, 2021

@gpshead @corona10: Do you think that it's worth it to backport the sem_clockwait() change from main the 3.9 and 3.10? I see it as a bugfix for https://bugs.python.org/issue41710 bug.


In the main branch, I wrote pytime_add() and _PyTime_AsTimespec_clamp() to handle timeout and clock values close to the C type limits. It should make the code safer. I made tons of changes in pytime.c for that. I don't think that it's worth it to backport these enhancements. Just don't use crazy timeouts :-D The current code doesn't clamp values, so this change doesn't add any regression.

In Modules/_threadmodule.c, lock_acquire_parse_args() makes sure that the timeout is less than PY_TIMEOUT_MAX. Moreover, PyThread_acquire_lock_timed() has a sanity check:

    if (microseconds > PY_TIMEOUT_MAX) {
        Py_FatalError("Timeout larger than PY_TIMEOUT_MAX");
    }

In main, I removed the Py_FatalError() call thanks to the new _PyTime_AsTimespec_clamp() function.

By the way, I documented pytime.c in pytime.h ;-) (in the main branch)

@corona10
Copy link
Member

@corona10 corona10 commented Oct 1, 2021

Do you think that it's worth it to backport the sem_clockwait() change from main the 3.9 and 3.10?

I am +0 on this.
Since using sem_clockwait can be a new feature from some views even though that feature can fix the bug.

@gpshead
Copy link
Member

@gpshead gpshead commented Oct 1, 2021

I think it is reasonable as a bugfix. sem_clockwait appears to be a pure libc only function, not a new syscall requiring new os kernel support so the risk seems low. When compiled on a system with headers defining it the binary already requires that min. library version to run. So it should be fine.

gpshead
gpshead approved these changes Oct 1, 2021
@vstinner
Copy link
Member Author

@vstinner vstinner commented Oct 1, 2021

I think it is reasonable as a bugfix. sem_clockwait appears to be a pure libc only function, not a new syscall requiring new os kernel support so the risk seems low. When compiled on a system with headers defining it the binary already requires that min. library version to run. So it should be fine.

Yeah, when I wrote my change for the main branch, I read the sem_clockwait() source code in the glibc. It shares code with other semaphore functions and it doesn't seem to be a new syscall. At least, I failed to find a specific syscall call in the code or using strace.

@vstinner vstinner merged commit 6df8c32 into python:3.10 Oct 1, 2021
13 checks passed
@vstinner vstinner deleted the sem_clockwait310 branch Oct 1, 2021
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Oct 1, 2021

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Oct 1, 2021

GH-28683 is a backport of this pull request to the 3.9 branch.

miss-islington added a commit to miss-islington/cpython that referenced this issue Oct 1, 2021
…GH-28671)

On Unix, if the sem_clockwait() function is available in the C
library (glibc 2.30 and newer), the threading.Lock.acquire() method
now uses the monotonic clock (time.CLOCK_MONOTONIC) for the timeout,
rather than using the system clock (time.CLOCK_REALTIME), to not be
affected by system clock changes.

configure now checks if the sem_clockwait() function is available.
(cherry picked from commit 6df8c32)

Co-authored-by: Victor Stinner <vstinner@python.org>
vstinner added a commit that referenced this issue Oct 1, 2021
…) (GH-28683)

On Unix, if the sem_clockwait() function is available in the C
library (glibc 2.30 and newer), the threading.Lock.acquire() method
now uses the monotonic clock (time.CLOCK_MONOTONIC) for the timeout,
rather than using the system clock (time.CLOCK_REALTIME), to not be
affected by system clock changes.

configure now checks if the sem_clockwait() function is available.
(cherry picked from commit 6df8c32)

Co-authored-by: Victor Stinner <vstinner@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants