-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
bpo-41710: PyThread_acquire_lock_timed() uses sem_clockwait() #28662
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
Conversation
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.
Python/thread_pthread.h
Outdated
deadline = _PyTime_GetMonotonicClock() + timeout; | ||
} | ||
|
||
while (1) { | ||
if (timeout > 0) { | ||
#ifdef HAVE_SEM_CLOCKWAIT | ||
struct timespec abs_timeout; | ||
_PyTime_AsTimespec_clamp(deadline, &abs_timeout); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if sem_clockwait() timeout argument can be modified by the function, for example if the wait is interrupted by a signal. In case of doubt, I prefer to compute abs_timeout before each call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the same file, pthread_cond_timedwait() absolute timeout is only computed once outside the loop. I understand that we can also compute abs_timeout once outside the loop.
The new implementation of time.sleep(), pysleep() function, now also uses an absolute timeout (timespec) computed once outside the loop. It uses clock_nanosleep() or nanosleep(). These functions have an argument for the remaining time, it's not used by pysleep().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sem_clockwait() documentation doesn't say that the absolute timeout is modified, which confirms that it can be only computed once:
https://www.gnu.org/software/libc/manual/html_node/Waiting-with-Explicit-Clocks.html
Python/thread_pthread.h
Outdated
#ifdef HAVE_SEM_CLOCKWAIT | ||
struct timespec abs_timeout; | ||
_PyTime_AsTimespec_clamp(deadline, &abs_timeout); | ||
status = fix_status(sem_clockwait(thelock, CLOCK_MONOTONIC, &abs_timeout)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if sem_clockwait() can fail with ENOSYS if Python is built with a recent glibc and then run with an older glibc or a different kernel version.
Should we attempt to fallback to sem_timedwait() if sem_clockwait() "doesn't work"? How can we determine if sem_clockwait() "doesn't work"? So far, I failed to find a sem_clockwait() documentation or manual page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case of doubt, I prefer to leave the code as it is, and only implement a fallback once we can test it on a real system.
Python/thread_pthread.h
Outdated
@@ -486,6 +503,9 @@ PyThread_acquire_lock_timed(PyThread_type_lock lock, PY_TIMEOUT_T microseconds, | |||
break; | |||
} | |||
|
|||
// sem_clockwait() uses an absolution timeout, there is no need |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/absolution/absolute
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nobody expects the Spanish Absolution!
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