bpo-30768: Recompute timeout on interrupted lock by vstinner · Pull Request #4103 · python/cpython

vstinner · 2017-10-24T10:28:09Z

Fix the pthread 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.

https://bugs.python.org/issue30768

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.

pitrou · 2017-10-24T10:32:09Z

Python/thread_pthread.h

-        if (microseconds > 0)
+
+        if (!intr_flag) {
+            _PyTime_t timeout = _PyTime_FromNanoseconds(microseconds * 1000);


What if microseconds * 1000 overflows?

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

pitrou · 2017-10-24T13:50:51Z

Python/pytime.c

I don't think it will be safe to raise an exception from a PyThread function (which could be called without the GIL).

Oops, you're right. I forgot that.

pitrou · 2017-10-24T13:51:49Z

Python/pytime.c

Isn't it too late? You've already raised the exception.

I removed _PyTime_FromMicroseconds(), it was a bad idea.

vstinner · 2017-10-24T15:44:04Z

I read again the code. PyThread_acquire_lock_timed() doc is explicit about the timeout limit: "microseconds must be less than PY_TIMEOUT_MAX. Behaviour otherwise is undefined."

I removed _PyTime_FromMicroseconds(), it was a bad idea. Instead, I ajusted PY_TIMEOUT_MAX.

By the way, the main consumer of PyThread_acquire_lock_timed() is threading.Lock.acquire() and this function is careful on integer overflow. This function already stores internally the timeout as a _PyTime_t and explicitely ensures that "microseconds <= PY_TIMEOUT_MAX".

vstinner · 2017-10-24T15:48:24Z

My patch doesn't change the value of threading.TIMEOUT_MAX, it's still a value larger than 292 years :-)

>>> import threading
>>> threading.TIMEOUT_MAX
9223372036.0
>>> (2**63-1) // 10**9
9223372036
>>> threading.TIMEOUT_MAX / (3600 * 24 * 365.25) # years
292.27102301822697

If you ignore PyThread_acquire_lock_timed() doc and pass a timeout larger than 292 years, you get an overflow. Sorry for you. But why do you use such crazy timeout? :-)

The pthread implementation of PyThread_acquire_lock() now fails with a fatal error if the timeout is larger than PY_TIMEOUT_MAX, as done in the Windows implementation. The check prevents any risk of overflow in PyThread_acquire_lock(). Add also PY_DWORD_MAX constant.

vstinner · 2017-10-24T16:51:18Z

Python/thread_pthread.h

+            }
+            else if (dt > 0) {
+                _PyTime_t realtime_deadline = _PyTime_GetSystemClock() + dt;
+                if (_PyTime_AsTimespec(realtime_deadline, &ts) < 0) {


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

Which new commit? :-)

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"); + }

Ok, thank you for adding the comments :-)

pitrou · 2017-10-24T21:26:38Z

My patch doesn't change the value of threading.TIMEOUT_MAX, it's still a value larger than 292 years :-)

I see, thank you. I guess it's large enough :-)

pitrou · 2017-10-24T21:28:30Z

Include/pythread.h

+#  define PY_TIMEOUT_MAX (PY_LLONG_MAX / 1000)
+#elif defined (NT_THREADS)
+   /* In the NT API, the timeout is a DWORD and is expressed in milliseconds */
+#  if 0xFFFFFFFFLL * 1000 < PY_LLONG_MAX


Which integer width does the C preprocessor use? :-) Can 0xFFFFFFFFLL * 1000 be truncated?

I didn't write this code, I only moved it.

The "LL" suffix is for "long long" which should be at least 64-bit signed integer, no?

Are you suggesting to replace the code with "0xFFFFFFFFLL < PY_LLONG_MAX / 1000" to prevent any risk of integer overflow?

Ah, ok. No, nevermind.

vstinner · 2017-10-24T23:26:36Z

Oh, I forgot to regenerate generated file using "make clinic" when I added the PY_DWORD_MAX constant. It's now fixed. I also merged master into this PR.

pitrou · 2017-10-24T23:30:43Z

Hmm... I think this PR looks ok now.

vstinner · 2017-10-24T23:52:10Z

I tested manually the PR using files attached to the issue: "Apply PR 4103, apply attached test.patch, recompile Python, and run interrupted_lock.py to test the PR." It works well. Result:

haypo@selma$ ./python interrupted_lock.py 
acquire(timeout=1.0) took 1.073 seconds and got 105 signals

I modified interrupted_lock.py with:

def burn_cpu():
    for i in range(10**4 * 3):
        pass

the-knights-who-say-ni added the CLA signed label Oct 24, 2017

bedevere-bot added the awaiting merge label Oct 24, 2017

bpo-30768: Recompute timeout on interrupted lock

3551353

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.

pitrou reviewed Oct 24, 2017

View reviewed changes

Adjust PY_TIMEOUT_MAX to prevent overflow

764d0cd

vstinner added 2 commits October 24, 2017 18:08

Fix _thread.TIMEOUT_MAX rounding on Windows

17aba7c

vstinner commented Oct 24, 2017

View reviewed changes

pitrou reviewed Oct 24, 2017

View reviewed changes

vstinner added 3 commits October 25, 2017 01:21

Remove now unused "exit" label

68e3f02

Update generated code: run "make clinic"

1fb576c

Merge branch 'master' into sem_timedwait

7232c61

vstinner merged commit 850a18e into python:master Oct 24, 2017

bedevere-bot removed the awaiting merge label Oct 24, 2017

vstinner deleted the sem_timedwait branch October 24, 2017 23:53

vstinner mentioned this pull request Jun 21, 2022

PyThread_acquire_lock_timed() should recompute the timeout when interrupted by a signal #74953

Closed

Uh oh!

Conversation

vstinner commented Oct 24, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Oct 24, 2017

Uh oh!

vstinner commented Oct 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou commented Oct 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Oct 24, 2017

Uh oh!

pitrou commented Oct 24, 2017

Uh oh!

vstinner commented Oct 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vstinner commented Oct 24, 2017 •

edited by bedevere-bot

Loading