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

bpo-30768: Recompute timeout on interrupted lock #4103

Merged
merged 7 commits into from
Oct 24, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion Include/pythread.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,13 @@ PyAPI_FUNC(int) PyThread_acquire_lock(PyThread_type_lock, int);
and floating-point numbers allowed.
*/
#define PY_TIMEOUT_T long long
#define PY_TIMEOUT_MAX PY_LLONG_MAX
#if defined(_POSIX_THREADS)
/* PyThread_acquire_lock_timed() uses _PyTime_FromNanoseconds(us * 1000),
convert microseconds to nanoseconds. */
# define PY_TIMEOUT_MAX (PY_LLONG_MAX / 1000)
#else
# define PY_TIMEOUT_MAX PY_LLONG_MAX
#endif

/* In the NT API, the timeout is a DWORD and is expressed in milliseconds */
#if defined (NT_THREADS)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fix the pthread+semaphore implementation of PyThread_acquire_lock_timed() when
called with timeout > 0 and intr_flag=0: recompute the timeout if
sem_timedwait() is interrupted by a signal (EINTR). See also the :pep:`475`.
6 changes: 4 additions & 2 deletions Modules/_threadmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -1363,9 +1363,11 @@ PyInit__thread(void)
if (m == NULL)
return NULL;

timeout_max = PY_TIMEOUT_MAX / 1000000;
time_max = floor(_PyTime_AsSecondsDouble(_PyTime_MAX));
timeout_max = (double)PY_TIMEOUT_MAX * 1e-6;
time_max = _PyTime_AsSecondsDouble(_PyTime_MAX);
timeout_max = Py_MIN(timeout_max, time_max);
/* Round towards minus infinity */
timeout_max = floor(timeout_max);

v = PyFloat_FromDouble(timeout_max);
if (!v)
Expand Down
52 changes: 46 additions & 6 deletions Python/thread_pthread.h
Original file line number Diff line number Diff line change
Expand Up @@ -318,23 +318,62 @@ PyThread_acquire_lock_timed(PyThread_type_lock lock, PY_TIMEOUT_T microseconds,
sem_t *thelock = (sem_t *)lock;
int status, error = 0;
struct timespec ts;
_PyTime_t deadline = 0;

(void) error; /* silence unused-but-set-variable warning */
dprintf(("PyThread_acquire_lock_timed(%p, %lld, %d) called\n",
lock, microseconds, intr_flag));

if (microseconds > 0)
if (microseconds > 0) {
MICROSECONDS_TO_TIMESPEC(microseconds, ts);
do {
if (microseconds > 0)

if (!intr_flag) {
/* the caller must ensures that microseconds <= PY_TIMEOUT_MAX
and so microseconds * 1000 cannot overflow. PY_TIMEOUT_MAX
is defined to prevent this specific overflow. */
_PyTime_t timeout = _PyTime_FromNanoseconds(microseconds * 1000);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if microseconds * 1000 overflows?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added _PyTime_FromMicroseconds() to detect overflow on "us * 1000". But I chose to not detect overflow on "_PyTime_GetMonotonicClock() + timeout".

deadline = _PyTime_GetMonotonicClock() + timeout;
}
}

while (1) {
if (microseconds > 0) {
status = fix_status(sem_timedwait(thelock, &ts));
else if (microseconds == 0)
}
else if (microseconds == 0) {
status = fix_status(sem_trywait(thelock));
else
}
else {
status = fix_status(sem_wait(thelock));
}

/* Retry if interrupted by a signal, unless the caller wants to be
notified. */
} while (!intr_flag && status == EINTR);
if (intr_flag || status != EINTR) {
break;
}

if (microseconds > 0) {
/* wait interrupted by a signal (EINTR): recompute the timeout */
_PyTime_t dt = deadline - _PyTime_GetMonotonicClock();
if (dt < 0) {
status = ETIMEDOUT;
break;
}
else if (dt > 0) {
_PyTime_t realtime_deadline = _PyTime_GetSystemClock() + dt;
if (_PyTime_AsTimespec(realtime_deadline, &ts) < 0) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this function raises an exception on overflow. This bug is now fixed in the new commit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which new commit? :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4th commit of this PR, commit 7c428b7 which added the following code at the top of the function:

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thank you for adding the comments :-)

success = PY_LOCK_FAILURE;
goto exit;
}
/* no need to update microseconds value, the code only care
if (microseconds > 0 or (microseconds == 0). */
}
else {
microseconds = 0;
}
}
}

/* Don't check the status if we're stopping because of an interrupt. */
if (!(intr_flag && status == EINTR)) {
Expand All @@ -359,6 +398,7 @@ PyThread_acquire_lock_timed(PyThread_type_lock lock, PY_TIMEOUT_T microseconds,
success = PY_LOCK_FAILURE;
}

exit:
dprintf(("PyThread_acquire_lock_timed(%p, %lld, %d) -> %d\n",
lock, microseconds, intr_flag, success));
return success;
Expand Down