GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-30768: Recompute timeout on interrupted lock by vstinner · Pull Request #4103 · python/cpython

Conversation

vstinner commented Oct 24, 2017 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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).

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

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

Comment thread

Python/thread_pthread.h

		if (microseconds > 0)

		if (!intr_flag) {
		_PyTime_t timeout = _PyTime_FromNanoseconds(microseconds * 1000);

pitrou Oct 24, 2017

Copy link

Copy Markdown

Member

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.

What if microseconds * 1000 overflows?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Oct 24, 2017

Copy link

Copy Markdown

Member Author

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 added _PyTime_FromMicroseconds() to detect overflow on "us * 1000". But I chose to not detect overflow on "_PyTime_GetMonotonicClock() + timeout".

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pitrou reviewed

Oct 24, 2017

View reviewed changes

Comment thread

Python/pytime.c Outdated

pitrou Oct 24, 2017

Copy link

Copy Markdown

Member

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 think it will be safe to raise an exception from a PyThread function (which could be called without the GIL).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Oct 24, 2017

Copy link

Copy Markdown

Member Author

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.

Oops, you're right. I forgot that.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pitrou reviewed

Oct 24, 2017

View reviewed changes

Comment thread

Python/pytime.c Outdated

pitrou Oct 24, 2017

Copy link

Copy Markdown

Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Oct 24, 2017

Copy link

Copy Markdown

Member Author

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 removed _PyTime_FromMicroseconds(), it was a bad idea.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Adjust PY_TIMEOUT_MAX to prevent overflow

764d0cd

vstinner commented Oct 24, 2017

Copy link

Copy Markdown

Member Author

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".

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner commented Oct 24, 2017

Copy link

Copy Markdown

Member Author

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? :-)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner added 2 commits

October 24, 2017 18:08


          Fix _thread.TIMEOUT_MAX rounding on Windows

17aba7c


          Fatal error if timeout > PY_TIMEOUT_MAX

7c428b7

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 commented

Oct 24, 2017

View reviewed changes

Comment thread

Python/thread_pthread.h

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

vstinner Oct 24, 2017

Copy link

Copy Markdown

Member Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pitrou Oct 24, 2017

Copy link

Copy Markdown

Member

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.

Which new commit? :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Oct 24, 2017

Copy link

Copy Markdown

Member Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pitrou Oct 24, 2017

Copy link

Copy Markdown

Member

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.

Ok, thank you for adding the comments :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pitrou commented Oct 24, 2017

Copy link

Copy Markdown

Member

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 :-)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pitrou reviewed

Oct 24, 2017

View reviewed changes

Comment thread

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

pitrou Oct 24, 2017

Copy link

Copy Markdown

Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Oct 24, 2017

Copy link

Copy Markdown

Member Author

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 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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pitrou Oct 24, 2017

Copy link

Copy Markdown

Member

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.

Ah, ok. No, nevermind.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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 commented Oct 24, 2017

Copy link

Copy Markdown

Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pitrou commented Oct 24, 2017

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner commented Oct 24, 2017

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Reviewers

pitrou

pitrou left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vstinner commented Oct 24, 2017 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.