GeezerVison is showing github.com — PyThread_acquire_lock_timed() should recompute the timeout when interrupted by a signal · Issue #74953 · python/cpython
PyThread_acquire_lock_timed() should recompute the timeout when interrupted by a signal #74953Description
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 = None
created_at = <Date 2017-06-26.13:50:01.691>
labels = ['3.7']
title = 'PyThread_acquire_lock_timed() should recompute the timeout when interrupted by a signal'
updated_at = <Date 2017-10-25.00:15:14.862>
user = 'https://github.com/vstinner' bugs.python.org fields:
activity = <Date 2017-10-25.00:15:14.862>
actor = 'vstinner'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = []
creation = <Date 2017-06-26.13:50:01.691>
creator = 'vstinner'
dependencies = []
files = ['47233', '47234']
hgrepos = []
issue_num = 30768
keywords = ['patch']
message_count = 10.0
messages = ['296896', '296898', '296899', '304892', '304893', '304895', '304896', '304951', '304952', '304953']
nosy_count = 3.0
nosy_names = ['pitrou', 'vstinner', 'neologix']
pr_nums = ['4103']
priority = 'normal'
resolution = None
stage = 'resolved'
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue30768'
versions = ['Python 3.7'] Activityvstinner commented on Jun 26, 2017 The current code of PyThread_acquire_lock_timed() (the implementation not using semaphore) doesn't compute correctly the timeout when pthread_cond_timedwait() is interrupted by a signal. We should recompute the timeout using a deadline.
Something like select.select():
if (tvp)
deadline = _PyTime_GetMonotonicClock() + timeout;
do {
... use tvp
if (errno != EINTR)
break;
/* select() was interrupted by a signal */
if (PyErr_CheckSignals())
goto finally;
if (tvp) {
timeout = deadline - _PyTime_GetMonotonicClock();
if (timeout < 0) {
n = 0;
break;
}
_PyTime_AsTimeval_noraise(timeout, &tv, _PyTime_ROUND_CEILING);
/* retry select() with the recomputed timeout */
}
} while (1);vstinner commented on Jun 26, 2017 See also the old bpo-12822: "NewGIL should use CLOCK_MONOTONIC if possible.". vstinner commented on Jun 26, 2017 See also the PEP-475 "Retry system calls failing with EINTR" and PEP-418 (time.monotonic). vstinner commented on Oct 24, 2017 interrupted_lock.py: test threading.Lock.acquire(timeout=1.0) with SIGALRM sent every 1 ms (so up to 1000 times in total). Example:
haypo@selma$ ./python interrupted_lock.py
acquire(timeout=1.0) took 1.0 seconds and got 1000 signals
Oh, in fact, threading.Lock.acquire(timeout=1.0) already recomputes the timeout when interrupted.
In Python stdlib, PyThread_acquire_lock_timed() is only called from one place with intr_flag=0: faulthandler watchdog thread, but this thread blocks all signals:
/* we don't want to receive any signal */
sigfillset(&set);
pthread_sigmask(SIG_SETMASK, &set, NULL);
vstinner commented on Oct 24, 2017 Ah, I found another caller of PyThread_acquire_lock_timed() with a timeout > 0 and intr_flag=0: _enter_buffered_busy() of Modules/_io/bufferedio.c:
/* When finalizing, we don't want a deadlock to happen with daemon
* threads abruptly shut down while they owned the lock.
* Therefore, only wait for a grace period (1 s.).
* Note that non-daemon threads have already exited here, so this
* shouldn't affect carefully written threaded I/O code.
*/
st = PyThread_acquire_lock_timed(self->lock, (PY_TIMEOUT_T)1e6, 0);
vstinner commented on Oct 24, 2017 I wrote PR 4103 to fix the pthread+semaphore implementation of PyThread_acquire_lock_timed().
Apply PR 4103, apply attached test.patch, recompile Python, and run interrupted_lock.py to test the PR. Result:
haypo@selma$ ./python interrupted_lock.py
acquire(timeout=1.0) took 1.0 seconds and got 911 signals vstinner commented on Oct 24, 2017 To check if you are using pthread+semaphore, use:
haypo@selma$ ./python -c 'import sys; print(sys.thread_info)'
sys.thread_info(name='pthread', lock='semaphore', version='NPTL 2.25')
Here you have pthread+semaphore. It's Fedora 26 running Linux kernel 4.13. vstinner commented on Oct 24, 2017 vstinner commented on Oct 25, 2017 I merged my PR. Thanks Antoine Pitrou for the review!
This change only impacts the io.BufferedWriter and io.BufferedReader during Python finalization. It has no effect on theading.Lock.acquire(). It might impact faulthandler.dump_traceback_later(), but in practice, it shouldn't change anything since the internal faulthandler watchdog thread blocks all signals.
If I understand correctly, if the system clock is stepped back by 1 hour during Python finalization, after PyThread_acquire_lock_timed() computed the deadline, but before sem_timedwait() completed, _enter_buffered_busy() can be blocked during 1 hour.
Moving the system clock backward by 1 hour occurs once a year on the DST change. But the race condition is unlikely since the size of the time window is only 1 second. The DST change should occur at Python shutdown when an io.BufferedReader or io.BufferedWriter is used.
Since the race condition seems very unlikely and was never reported by another user, I propose to not backward this change. Moreover, I'm not confident to modify locks in a stable release :-) vstinner commented on Oct 25, 2017 Oh, the pthread condvar+mutex implementation still has the bug, so I reopen the issue. added 2 commits that reference this issue on Jun 17, 2022gh-74953: Fix PyThread_acquire_lock_timed() code recomputing the time… added 2 commits that reference this issue on Jun 17, 2022pythongh-74953: Fix PyThread_acquire_lock_timed() code recomputing th… gh-74953: Fix PyThread_acquire_lock_timed() code recomputing the time… vstinner commented on Jun 21, 2022 I looked again to this issue. IMO it's now fully fixed, see my comment for the rationale: #93946 (comment)
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|