gh-144706: docs note about combining RLock and signal handlers#144736
gh-144706: docs note about combining RLock and signal handlers#144736ZeroIntensity merged 6 commits intopython:mainfrom
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.
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.
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.
- 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.
- 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.
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.
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.
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.
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.
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.
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.
|
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 945bf8c) 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/