Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upFix remove_signal_handler to not to crash after PyOS_FiniInterrupts #456
Conversation
if self._interpreter_shutting_down: | ||
# The interpreter is being shutdown. `PyOS_FiniInterrupts` | ||
# was already called and it has restored all signals already. | ||
return |
This comment has been minimized.
This comment has been minimized.
1st1
Nov 7, 2016
•
Author
Member
Instead of just return
, we can emit a warning that the loop wasn't properly closed. @gvanrossum what do you think about that?
This comment has been minimized.
This comment has been minimized.
if self._interpreter_shutting_down: | ||
# The interpreter is being shutdown. `PyOS_FiniInterrupts` | ||
# was already called and it has restored all signals already. | ||
return |
This comment has been minimized.
This comment has been minimized.
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. |
if self._interpreter_shutting_down: | ||
# The interpreter is being shutdown. `PyOS_FiniInterrupts` | ||
# was already called and it has restored all signals already. | ||
return |
This comment has been minimized.
This comment has been minimized.
Do we know that the atexit handler will always be called early enough?
|
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.
|
…izing
asvetlov
commented
Nov 9, 2016
•
I think the PR makes sense: we raise |
OK, then LGTM.
|
vxgmichel
commented
Nov 9, 2016
If something needs to be changed in signalmodule.c please open an issue on
bugs.python.org.
|
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. |
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. |
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:
|
Maybe they ran configure with different parameters or in a different
context or with a different version of Xcode?
|
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:
|
1st1 commentedNov 7, 2016
I think I've figured out what's going on with
remove_signal_handler
.This PR fixes: