GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-71052: Use `raise_signal` rather than `kill` in `ThreadSignals.test_signals` by mhsmith · Pull Request #116423 · python/cpython

Conversation

@mhsmith

@mhsmith mhsmith commented Mar 6, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

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.

  • Issue: Meta-issue: support of the android platform #71052

Sorry, something went wrong.

Uh oh!

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

All reactions
@mhsmith
Use raise_signal rather than kill in ThreadSignals.test_signals
6c248bc
@bedevere-app bedevere-app Bot added awaiting review tests Tests in the Lib/test dir labels Mar 6, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Mar 6, 2024
Meta-issue: support of the android platform #71052
Closed
@mhsmith

mhsmith commented Mar 6, 2024

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@mhsmith

mhsmith commented Mar 11, 2024

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner
vstinner approved these changes Mar 11, 2024
View reviewed changes

@vstinner vstinner left a comment

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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Mar 11, 2024
@vstinner vstinner added the skip news label Mar 11, 2024
@vstinner
vstinner merged commit 34920f3 into python:main Mar 11, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Mar 11, 2024
@vstinner vstinner added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Mar 11, 2024
@miss-islington-app

miss-islington-app Bot commented Mar 11, 2024

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@miss-islington-app

miss-islington-app Bot commented Mar 11, 2024

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 11, 2024
@mhsmith @miss-islington
pythongh-71052: Use raise_signal in ThreadSignals.test_signals (p…
f1d3a9a
…ythonGH-116423)

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

Co-authored-by: Malcolm Smith <[email protected]>
@bedevere-app

bedevere-app Bot commented Mar 11, 2024

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 11, 2024
@mhsmith @miss-islington
pythongh-71052: Use raise_signal in ThreadSignals.test_signals (p…
d72d072
…ythonGH-116423)

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

Co-authored-by: Malcolm Smith <[email protected]>
@bedevere-app bedevere-app Bot removed the needs backport to 3.11 only security fixes label Mar 11, 2024
@bedevere-app

bedevere-app Bot commented Mar 11, 2024

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label Mar 11, 2024
@vstinner

vstinner commented Mar 11, 2024

Copy link
Copy Markdown
Member

Merged, thanks!

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot

bedevere-bot commented Mar 11, 2024

Copy link
Copy Markdown

⚠️⚠️⚠️ 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'
All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner pushed a commit that referenced this pull request Mar 11, 2024
@miss-islington @mhsmith
[3.11] gh-71052: Use raise_signal in ThreadSignals.test_signals (G…
88ed804
…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 <[email protected]>
vstinner pushed a commit that referenced this pull request Mar 11, 2024
@miss-islington @mhsmith
[3.12] gh-71052: Use raise_signal in ThreadSignals.test_signals (G…
2c1a817
…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 <[email protected]>
This was referenced Mar 11, 2024
gh-113538: Allow client connections to be closed #114432
Merged
Add support for Android as a target platform #116622
Closed
gvanrossum added a commit to gvanrossum/cpython that referenced this pull request Mar 11, 2024
@gvanrossum
Revert "pythongh-113538: Add asycio.Server.{close,abort}_clients (pyt…
3c0ecf5
…hon#114432)"

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

This reverts commit 1d0d49a.
gvanrossum added a commit that referenced this pull request Mar 12, 2024
@gvanrossum
gh-113538: Revert "gh-113538: Add asycio.Server.{close,abort}_clients (…
ba13215
…#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
@mhsmith @adorilson
pythongh-71052: Use raise_signal in ThreadSignals.test_signals (p…
0a92c03
…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
@gvanrossum @adorilson
pythongh-113538: Revert "pythongh-113538: Add asycio.Server.{close,ab…
e8dd85d
…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
@mhsmith @diegorusso
pythongh-71052: Use raise_signal in ThreadSignals.test_signals (p…
bfdebd4
…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
@gvanrossum @diegorusso
pythongh-113538: Revert "pythongh-113538: Add asycio.Server.{close,ab…
08737d3
…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.
@mhsmith mhsmith mentioned this pull request Sep 11, 2024
gh-116622: Switch test_stress_delivery_simultaneous from SIGUSR1 to SIGUSR2 #123981
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

skip news tests Tests in the Lib/test dir

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.

3 participants

@mhsmith @vstinner @bedevere-bot
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.