Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.4k
gh-71052: Useraise_signal
rather thankill
inThreadSignals.test_signals
#116423
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This is a minor change to a test, so no news entry is required. |
@vstinner: There's no CODEOWNERS entry for signals, but you've done some work in this area recently. Are you able to review this PR? |
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.
LGTM.
The change works as expected. I stressed the test with./python -m test test_threadsignals -m test_signals -j250 -F
. The system load was around 225.00 on a laptop with 12 logicial CPUs (6 cores), the test ran successfully 1676 times in 2 minutes.
…ythonGH-116423)Use `raise_signal` rather than `kill` in `ThreadSignals.test_signals`(cherry picked from commit34920f3)Co-authored-by: Malcolm Smith <smith@chaquo.com>
GH-116617 is a backport of this pull request to the3.11 branch. |
…ythonGH-116423)Use `raise_signal` rather than `kill` in `ThreadSignals.test_signals`(cherry picked from commit34920f3)Co-authored-by: Malcolm Smith <smith@chaquo.com>
GH-116618 is a backport of this pull request to the3.12 branch. |
Merged, thanks! |
bedevere-bot commentedMar 11, 2024
|
…hon#114432)"Reason: The new test doesn't always pass:python#116423 (comment)This reverts commit1d0d49a.
…ython#116423)Use `raise_signal` rather than `kill` in `ThreadSignals.test_signals`
…ort}_clients (python#114432)" (python#116632)Revert "pythongh-113538: Add asycio.Server.{close,abort}_clients (python#114432)"Reason: The new test doesn't always pass:python#116423 (comment)This reverts commit1d0d49a.
…ython#116423)Use `raise_signal` rather than `kill` in `ThreadSignals.test_signals`
…ort}_clients (python#114432)" (python#116632)Revert "pythongh-113538: Add asycio.Server.{close,abort}_clients (python#114432)"Reason: The new test doesn't always pass:python#116423 (comment)This reverts commit1d0d49a.
Uh oh!
There was an error while loading.Please reload this page.
On Android, we run the test suite embedded in a standard app, in order to be representative of the environment where Python is most likely to be used. This means there's a native thread that uses
sigwait
to listen for SIGUSR1 in a loop, and responds by triggering the Java garbage collector and logging a message. This is only used for debugging purposes, and in most cases it doesn't matter because any signal handler will take priority over it.However, in
ThreadSignals.test_signals
there's a background thread which sends both SIGUSR1 and SIGUSR2, and then immediately exits. So if the background thread is unavailable because it’s exited, and the main thread is unavailable because it’s already processing one of the signals (seecomplete_signal
inkernel/signal.c), then the other signal may be delivered to Android’ssigwait
loop instead of the test's own handler. This happens about 1 time out of 4.This PR fixes that by sending the signals using
raise_signal
(which is directed at the current thread) rather thanos.kill
(which is directed at the whole process). Not only does this avoid the above scenario, it also strengthens the test by verifying that a signal which isdelivered to a background thread, and not merelysent by it, still runs the Python-level handler on the main thread.Since
raise
"shall not return until after the signal handler does", this also allows for the removal of a whole block of code which waits for the signal to arrive.