Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

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

Fix remove_signal_handler to not to crash after PyOS_FiniInterrupts #456

Open
1st1 wants to merge 2 commits into python:master
base: master
Choose a base branch
Loading
from 1st1:signals

Conversation

@1st1
Copy link
Member

@1st1 1st1 commented Nov 7, 2016

if self._interpreter_shutting_down:
# The interpreter is being shutdown. `PyOS_FiniInterrupts`
# was already called and it has restored all signals already.
return
Copy link
Member Author

@1st1 1st1 Nov 7, 2016
edited
Loading

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?

Copy link
Member

@gvanrossum gvanrossum Nov 7, 2016

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 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
if self._interpreter_shutting_down:
# The interpreter is being shutdown. `PyOS_FiniInterrupts`
# was already called and it has restored all signals already.
return
Copy link
Member

@gvanrossum gvanrossum Nov 7, 2016

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.

Copy link
Member

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

if self._interpreter_shutting_down:
# The interpreter is being shutdown. `PyOS_FiniInterrupts`
# was already called and it has restored all signals already.
return
Copy link
Member

@gvanrossum gvanrossum Nov 7, 2016

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.

Copy link
Member

gvanrossum commented Nov 7, 2016 via email

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

Copy link
Member Author

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.

Copy link

asvetlov commented Nov 9, 2016
edited
Loading

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.

Copy link
Member

gvanrossum commented Nov 9, 2016 via email

OK, then LGTM.

Copy link

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

Copy link
Member

gvanrossum commented Nov 9, 2016 via email

If something needs to be changed in signalmodule.c please open an issue on bugs.python.org.

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member Author

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

Copy link
Member

gvanrossum commented Nov 9, 2016 via email

Maybe they ran configure with different parameters or in a different context or with a different version of Xcode?

Copy link
Member Author

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.

Copy link

@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

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

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /