Skip to content
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

gh-71052: Use raise_signal rather than kill in ThreadSignals.test_signals #116423

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

mhsmith
Copy link
Member

@mhsmith mhsmith commented Mar 6, 2024

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 (see complete_signal in kernel/signal.c), then the other signal may be delivered to Android’s sigwait 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 than os.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 is delivered to a background thread, and not merely sent 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.

@mhsmith
Copy link
Member Author

mhsmith commented Mar 6, 2024

This is a minor change to a test, so no news entry is required.

@mhsmith
Copy link
Member Author

mhsmith commented Mar 11, 2024

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

Copy link
Member

@vstinner vstinner left a 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.

@vstinner vstinner merged commit 34920f3 into python:main Mar 11, 2024
34 checks passed
@vstinner vstinner added needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Mar 11, 2024
@miss-islington-app
Copy link

Thanks @mhsmith for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Thanks @mhsmith for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 11, 2024
…ythonGH-116423)

Use `raise_signal` rather than `kill` in `ThreadSignals.test_signals`
(cherry picked from commit 34920f3)

Co-authored-by: Malcolm Smith <smith@chaquo.com>
@bedevere-app
Copy link

bedevere-app bot commented Mar 11, 2024

GH-116617 is a backport of this pull request to the 3.11 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 11, 2024
…ythonGH-116423)

Use `raise_signal` rather than `kill` in `ThreadSignals.test_signals`
(cherry picked from commit 34920f3)

Co-authored-by: Malcolm Smith <smith@chaquo.com>
@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Mar 11, 2024
@bedevere-app
Copy link

bedevere-app bot commented Mar 11, 2024

GH-116618 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Mar 11, 2024
@vstinner
Copy link
Member

Merged, thanks!

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot ARM64 macOS 3.x has failed when building commit 34920f3.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/725/builds/7335) and take a look at the build logs.
  4. Check if the failure is related to this commit (34920f3) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/725/builds/7335

Failed tests:

  • test.test_asyncio.test_server

Failed subtests:

  • test_abort_clients - test.test_asyncio.test_server.TestServer2.test_abort_clients

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/unittest/async_case.py", line 93, in _callTestMethod
    if self._callMaybeAsync(method) is not None:
       ~~~~~~~~~~~~~~~~~~~~^^^^^^^^
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/unittest/async_case.py", line 115, in _callMaybeAsync
    return self._asyncioRunner.run(
           ~~~~~~~~~~~~~~~~~~~~~~~^
        func(*args, **kwargs),
        ^^^^^^^^^^^^^^^^^^^^^^
        context=self._asyncioTestContext,
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/asyncio/runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/asyncio/base_events.py", line 721, in run_until_complete
    return future.result()
           ~~~~~~~~~~~~~^^
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/test/test_asyncio/test_server.py", line 231, in test_abort_clients
    s_sock = s_wr.get_extra_info('socket')
             ^^^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'get_extra_info'

vstinner pushed a commit that referenced this pull request Mar 11, 2024
…H-116423) (#116617)

gh-71052: Use `raise_signal` in `ThreadSignals.test_signals` (GH-116423)

Use `raise_signal` rather than `kill` in `ThreadSignals.test_signals`
(cherry picked from commit 34920f3)

Co-authored-by: Malcolm Smith <smith@chaquo.com>
vstinner pushed a commit that referenced this pull request Mar 11, 2024
…H-116423) (#116618)

gh-71052: Use `raise_signal` in `ThreadSignals.test_signals` (GH-116423)

Use `raise_signal` rather than `kill` in `ThreadSignals.test_signals`
(cherry picked from commit 34920f3)

Co-authored-by: Malcolm Smith <smith@chaquo.com>
gvanrossum added a commit to gvanrossum/cpython that referenced this pull request Mar 11, 2024
gvanrossum added a commit that referenced this pull request Mar 12, 2024
…#114432)" (#116632)

Revert "gh-113538: Add asycio.Server.{close,abort}_clients (#114432)"

Reason: The new test doesn't always pass:
#116423 (comment)

This reverts commit 1d0d49a.
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
…ython#116423)

Use `raise_signal` rather than `kill` in `ThreadSignals.test_signals`
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
…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 commit 1d0d49a.
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
…ython#116423)

Use `raise_signal` rather than `kill` in `ThreadSignals.test_signals`
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
…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 commit 1d0d49a.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants