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.

Sign up for GitHub

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

bpo-30768: Recompute timeout on interrupted lock #4103

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3551353
bpo-30768: Recompute timeout on interrupted lock
vstinner Oct 24, 2017
764d0cd
Adjust PY_TIMEOUT_MAX to prevent overflow
vstinner Oct 24, 2017
17aba7c
Fix _thread.TIMEOUT_MAX rounding on Windows
vstinner Oct 24, 2017
7c428b7
Fatal error if timeout > PY_TIMEOUT_MAX
vstinner Oct 24, 2017
68e3f02
Remove now unused "exit" label
vstinner Oct 24, 2017
1fb576c
Update generated code: run "make clinic"
vstinner Oct 24, 2017
7232c61
Merge branch 'master' into sem_timedwait
vstinner Oct 24, 2017
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
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Library/2017-10-24-12-00-16.bpo-30768.Om8Yj_.rst
View file
Open in desktop
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`.
49 changes: 43 additions & 6 deletions Python/thread_pthread.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,23 +318,59 @@ 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) {
_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 +395,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