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

Jump to bottom

signal.signal may throw when on threading.main_thread #91880

Closed
davidhewitt opened this issue Apr 24, 2022 · 3 comments
Closed

signal.signal may throw when on threading.main_thread #91880

davidhewitt opened this issue Apr 24, 2022 · 3 comments
Labels
type-bug An unexpected behavior, bug, or error

Comments

@davidhewitt
Copy link
Contributor

davidhewitt commented Apr 24, 2022

Following on from #32105 (comment)

Bug report

It is possible for signal.signal to throw even when checked against threading.main_thread. For example, in this pattern:

if threading.current_thread() is threading.main_thread() :
    signal.signal(signal.SIGINT, sigint_handler)

For example, in the case when the Python interpreter was started as an embedded interpreter with Py_InitializeEx(0) (0 meaning don't register signal handlers), then even though the threading check above will succeed signal.signal will throw because the "main thread" does not support signals.

It was suggested in #32105 (comment) that it might be desirable to add a new API to the signal module which would return True only if the current thread supports signals.
@davidhewitt davidhewitt added the type-bug An unexpected behavior, bug, or error label Apr 24, 2022
davidhewitt added a commit to davidhewitt/cpython that referenced this issue Apr 24, 2022
@davidhewitt
pythongh-91880: add try/except around signal.signal
2d95fb2
@MojoVampire
Copy link
Contributor

MojoVampire commented Apr 30, 2022

There's a mistake in this patch that I believe undoes at least part of the intent of the patch. When the exception is caught, the code sets:

signal_handler = None

but signal_handler is not actually used anywhere. Almost certainly the intent was to do:

sigint_handler = None

so as to prevent the attempt to check the currently set SIGINT handler later (and potentially restore the default handler). As is, it leaves sigint_handler as the functools.partial instance previously constructed, causing it to go down a code path that makes no sense when the main thread doesn't support signals at all.

davidhewitt added a commit to davidhewitt/cpython that referenced this issue Apr 30, 2022
@davidhewitt
pythongh-91880 - fix typo
2745418
@davidhewitt davidhewitt mentioned this issue Apr 30, 2022
Merged
@davidhewitt
Copy link
Contributor Author

davidhewitt commented Apr 30, 2022

Thanks, I've opened a PR to correct my error.

@gvanrossum
Copy link
Member

gvanrossum commented Apr 30, 2022

Ow, I should have caught that in review.

miss-islington pushed a commit that referenced this issue Apr 30, 2022
@davidhewitt
gh-91880 - fix typo (GH-92069)
c7b7f12 
#91880 (comment) - With thanks to @MojoVampire for spotting this.

Automerge-Triggered-By: GH:gvanrossum
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@davidhewitt @gvanrossum @MojoVampire