-
-
Notifications
You must be signed in to change notification settings - Fork 185
Fix remove_signal_handler to not to crash after PyOS_FiniInterrupts #456
Conversation
asyncio/unix_events.py
Outdated
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?
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.
asyncio/unix_events.py
Outdated
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.
@gvanrossum
gvanrossum
left a comment
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.
asyncio/unix_events.py
Outdated
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.
gvanrossum
commented
Nov 7, 2016
via email
1st1
commented
Nov 7, 2016
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.
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.
gvanrossum
commented
Nov 9, 2016
via email
vxgmichel
commented
Nov 9, 2016
gvanrossum
commented
Nov 9, 2016
via email
1st1
commented
Nov 9, 2016
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.
1st1
commented
Nov 9, 2016
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.
1st1
commented
Nov 9, 2016
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
gvanrossum
commented
Nov 9, 2016
via email
1st1
commented
Nov 9, 2016
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.
vxgmichel
commented
Nov 9, 2016
|
@1st1 My results:
|
I think I've figured out what's going on with
remove_signal_handler.This PR fixes: