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 — Fix remove_signal_handler to not to crash after PyOS_FiniInterrupts by 1st1 · Pull Request #456 · python/asyncio

This repository was archived by the owner on Nov 23, 2017. It is now read-only.

Conversation

@1st1

@1st1 1st1 commented Nov 7, 2016

Copy link
Copy Markdown
Member

I think I've figured out what's going on with remove_signal_handler.

This PR fixes:

  • TypeError: signal handler must be signal.SIG_IGN, signal.SIG_DFL, or a callable object #396
  • http://bugs.python.org/issue23548
  • http://bugs.python.org/issue28628

Sorry, something went wrong.

Uh oh!

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

All reactions
Yury Selivanov
Fix remove_signal_handler to not to crush after PyOS_FiniInterrupts
511fbb5
1st1
1st1 commented Nov 7, 2016
View reviewed changes
Comment thread asyncio/unix_events.py Outdated
if self._interpreter_shutting_down:
# The interpreter is being shutdown. `PyOS_FiniInterrupts`
# was already called and it has restored all signals already.
return

@1st1 1st1 Nov 7, 2016 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

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.

Instead of just return, we can emit a warning that the loop wasn't properly closed. @gvanrossum what do you think about that?

Sorry, something went wrong.

Uh oh!

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

All reactions

@gvanrossum gvanrossum Nov 7, 2016

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.

Only in debug mode. But frankly I don't understand this code.

Sorry, something went wrong.

Uh oh!

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

All reactions
@gvanrossum gvanrossum changed the title Fix remove_signal_handler to not to crush after PyOS_FiniInterrupts Fix remove_signal_handler to not to crash after PyOS_FiniInterrupts Nov 7, 2016
gvanrossum
gvanrossum reviewed Nov 7, 2016
View reviewed changes
Comment thread asyncio/unix_events.py Outdated
if self._interpreter_shutting_down:
# The interpreter is being shutdown. `PyOS_FiniInterrupts`
# was already called and it has restored all signals already.
return

@gvanrossum gvanrossum Nov 7, 2016

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.

This should return a bool.

Sorry, something went wrong.

Uh oh!

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

All reactions
gvanrossum
gvanrossum suggested changes Nov 7, 2016
View reviewed changes

@gvanrossum gvanrossum 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.

I don't understand all this well enough to know whether this is a good idea or not. We really need to find someone else who wants to co-own asyncio -- ATM I feel my role is mostly that of giving you permission to commit what you see fit and I am not actually comfortable with this -- we'll end up with there being only one person who understands asyncio and that's not enough for such an important stdlib module.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread asyncio/unix_events.py Outdated
if self._interpreter_shutting_down:
# The interpreter is being shutdown. `PyOS_FiniInterrupts`
# was already called and it has restored all signals already.
return

@gvanrossum gvanrossum Nov 7, 2016

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.

Only in debug mode. But frankly I don't understand this code.

Sorry, something went wrong.

Uh oh!

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

All reactions
@gvanrossum

gvanrossum commented Nov 7, 2016 via email

Copy link
Copy Markdown
Member
Do we know that the atexit handler will always be called early enough?
All reactions

Sorry, something went wrong.

Uh oh!

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

@1st1

1st1 commented Nov 7, 2016

Copy link
Copy Markdown
Member Author

I don't understand all this well enough to know whether this is a good idea or not. We really need to find someone else who wants to co-own asyncio -- ATM I feel my role is mostly that of giving you permission to commit what you see fit and I am not actually comfortable with this -- we'll end up with there being only one person who understands asyncio and that's not enough for such an important stdlib module.

I agree. I'm myself not feeling comfortable, even two of us doesn't seem to be enough to support asyncio.

@asvetlov, @methane and @Haypo: are you interested to help us with asyncio? At least to offer help with PRs and participate in related discussions.

Do we know that the atexit handler will always be called early enough?

Py_FinalizeEx first calls atexit functions and only after that it calls PyOS_FiniInterrupts(). The latter cleans up the internal state of signals module, making it useless. So yes, we're confident about atexit.

All reactions

Sorry, something went wrong.

Uh oh!

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

Yury Selivanov
Issue a warning in debug mode; return False when interpreter is final…
92f36ae
…izing
@asvetlov

asvetlov commented Nov 9, 2016 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

I think the PR makes sense: we raise ResourceWarning for unclosed transports in debug mode.
I believe we should do the same for unclosed loop too.

All reactions

Sorry, something went wrong.

Uh oh!

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

@gvanrossum

gvanrossum commented Nov 9, 2016 via email

Copy link
Copy Markdown
Member
OK, then LGTM.
All reactions

Sorry, something went wrong.

Uh oh!

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

@vxgmichel

vxgmichel commented Nov 9, 2016

Copy link
Copy Markdown

Shouldn't it be fixed in signalmodule.c instead? (see my comment in PR #396)

All reactions

Sorry, something went wrong.

Uh oh!

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

@gvanrossum

gvanrossum commented Nov 9, 2016 via email

Copy link
Copy Markdown
Member
If something needs to be changed in signalmodule.c please open an issue on bugs.python.org.
All reactions

Sorry, something went wrong.

Uh oh!

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

@1st1

1st1 commented Nov 9, 2016

Copy link
Copy Markdown
Member Author

Yes, the bug in _signal can (and should) be fixed, and I have a working patch for that. My worry is that it might be a bit too late to do that for 3.6. I'll submit the patch and ask Ned's opinion.

All reactions

Sorry, something went wrong.

Uh oh!

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

@1st1

1st1 commented Nov 9, 2016

Copy link
Copy Markdown
Member Author

I've no idea what's going on. I can reproduce this bug with python installed from MacPorts. I can't reproduce it with python I build from source.

All reactions

Sorry, something went wrong.

Uh oh!

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

@1st1

1st1 commented Nov 9, 2016

Copy link
Copy Markdown
Member Author

The bug is the following program:

import signal
import asyncio

def foo():
    pass

async def bar():
    pass

def main():
    loop = asyncio.get_event_loop()
    loop.add_signal_handler(signal.SIGINT, lambda: None)
    loop.add_signal_handler(signal.SIGHUP, lambda: None)
    loop.run_until_complete(bar())

if __name__ == "__main__":
    main()

It should spit out something like this:

Exception ignored in: <bound method BaseEventLoop.__del__ of <_UnixSelectorEventLoop running=False closed=True debug=False>>
Traceback (most recent call last):
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/asyncio/base_events.py", line 431, in __del__
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/asyncio/unix_events.py", line 58, in close
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/asyncio/unix_events.py", line 139, in remove_signal_handler
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/signal.py", line 47, in signal
TypeError: signal handler must be signal.SIG_IGN, signal.SIG_DFL, or a callable object
All reactions

Sorry, something went wrong.

Uh oh!

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

@gvanrossum

gvanrossum commented Nov 9, 2016 via email

Copy link
Copy Markdown
Member
Maybe they ran configure with different parameters or in a different context or with a different version of Xcode?
All reactions

Sorry, something went wrong.

Uh oh!

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

@1st1

1st1 commented Nov 9, 2016

Copy link
Copy Markdown
Member Author

Maybe. It should be something that changes how & when objects are GCed. The above traceback happens when objects are GCed after the signals module is finalized (and that's what I've seen in reports from asyncio and uvloop users). I'll try to get to the bottom of this.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vxgmichel

vxgmichel commented Nov 9, 2016

Copy link
Copy Markdown

@1st1
I tried to reproduce the bug with python 3.5 and python 3.6 (built from the latest sources) with your test for asyncio and this one for signal:

import _signal

class A:
    def __del__(self):
        _signal.signal(_signal.SIGTERM, _signal.SIG_DFL)

a = A()

My results:

asyncio test signal test
python 3.5 TypeError TypeError
python 3.6 No Error TypeError
All reactions

Sorry, something went wrong.

Uh oh!

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

@brettcannon brettcannon added CLA signed and removed CLA signed labels Nov 18, 2016
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Nov 18, 2016
@andrey-git andrey-git mentioned this pull request Feb 10, 2017
Fix exception that asyncio throws on GC home-assistant/core#5851
Closed
@lesteve lesteve mentioned this pull request Mar 21, 2020
SLURMCluster jobs not running when using parameters from dask.yaml dask/dask-jobqueue#394
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@gvanrossum gvanrossum gvanrossum requested changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

CLA signed

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.

6 participants

@1st1 @gvanrossum @asvetlov @vxgmichel @brettcannon @the-knights-who-say-ni
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.