-
-
Notifications
You must be signed in to change notification settings - Fork 34.1k
gh-144706: docs note about combining RLock and signal handlers #144736
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
Conversation
Doc/library/threading.rst
Outdated
|
|
||
| Because lock acquisition can be interrupted by signals, sharing reentrant | ||
| locks between :mod:`signal` handlers and the main thread can lead to | ||
| surprising behavior. Therefore, this is generally not recommended. |
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.
surprising behavior
any other surprise that is not main thread waiting forever for itself to release RLock?
ZeroIntensity
left a comment
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 think the note should go at the top of the file as a general warning about concurrency and signal handlers.
Doc/library/threading.rst
Outdated
| Because lock acquisition can be interrupted by signals, sharing reentrant | ||
| locks between :mod:`signal` handlers and the main thread can lead to | ||
| surprising behavior. Therefore, this is generally not recommended. |
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.
- This isn't limited to re-entrant locks.
- "generally not recommended" is not strong enough -- I can't think of a case where it's thread-safe to use locks in signal handlers.
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.
- You are right, I shall fix.
- sharing a synchronization primitive between the signal handler and some thread other than the main thread should work just fine, shouldn't it? Anyway, with regards to the main thread only, I can't think of a case either.
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.
This isn't limited to re-entrant locks.
what is surprising about normal locks (like returned by _thread.allocate_lock())?
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.
sharing a synchronization primitive between the signal handler and some thread other than the main thread should work just fine, shouldn't it?
I think you're missing the point. It might work right now, but using locks in signal handlers is not something we want to encourage or claim to support. We may want to change how we handle signals or what they affect someday.
what is surprising about normal locks (like returned by _thread.allocate_lock())?
By acquiring a lock in a signal handler, the lock is subject to deadlocks for any signal handled by the main thread. For example:
- Main thread acquires a lock.
- User sends signal.
- The signal handler is invoked, which tries to acquire that lock.
- Deadlock!
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 think you're missing the point. It might work right now, but using locks in signal handlers is not something we want to encourage or claim to support. We may want to change how we handle signals or what they affect someday.
Alright. In such case, I think this warning belongs into the signal module instead.
Doc/library/signal.rst
Outdated
| Synchronization primitives such as :class:`threading.Lock` should not be used | ||
| within signal handlers. Because blocking synchronization calls can be | ||
| interrupted by signals, such usage can lead to surprising dead locks. |
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 we need to be specific:
| Synchronization primitives such as :class:`threading.Lock` should not be used | |
| within signal handlers. Because blocking synchronization calls can be | |
| interrupted by signals, such usage can lead to surprising dead locks. | |
| Synchronization primitives such as :class:`threading.Lock` should not be used | |
| within signal handlers. Doing so can lead to deadlocks. |
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.
Thanks for the suggestion. I think if I were to read this for the first time I wouldn't understand the issue. And understanding something makes it easier to remember, too. I'd probably be thinking something like "well yeah, using synchronization primitives in general can lead to deadlocks. So what's different here?"
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.
Let's say "unexpected deadlocks" then. The situation in the issue is one of many problems that come up when using synchronization primitives in signal handlers, so it's misleading to say that interruptibility is the only concern.
ZeroIntensity
left a comment
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.
Thanks, LGTM.
|
Thanks @robsdedude for the PR, and @ZeroIntensity for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14. |
… signal handlers (pythonGH-144736) (cherry picked from commit 945bf8ce1bf7ee3881752c2ecc129e35ab818477) Co-authored-by: Robsdedude <dev@rouvenbauer.de>
… signal handlers (pythonGH-144736) (cherry picked from commit 945bf8ce1bf7ee3881752c2ecc129e35ab818477) Co-authored-by: Robsdedude <dev@rouvenbauer.de>
|
GH-144767 is a backport of this pull request to the 3.14 branch. |
|
GH-144768 is a backport of this pull request to the 3.13 branch. |
Document potentially surprising interplay or
threading.RLockandsignalas discussed in #144706 (comment).📚 Documentation preview 📚: https://cpython-previews--144736.org.readthedocs.build/