bpo-26133: Dont unsubscribe signals in UNIX even loop on interpreter shutdown by asvetlov · Pull Request #4956 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation21 Commits5 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
| self.remove_signal_handler(sig) |
|---|
| else: |
| warinigs.warn(f"Closing the loop {self} on interpreter shutdown " |
| f"stage, signal unsubsription is disabled") |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stacklevel arg?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! :)
| else: |
|---|
| warinigs.warn(f"Closing the loop {self} on interpreter shutdown " |
| f"stage, signal unsubsription is disabled", |
| stacklevel=2) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that stacklevel=2 is correct. Maybe remove it?
For the warning category, I hesitate to use ResourceWarning and pass source=self. ResourceWarning are quiet by default. It's maybe better to not pollute stdout with such warning?
| for sig in list(self._signal_handlers): |
|---|
| self.remove_signal_handler(sig) |
| else: |
| warinigs.warn(f"Closing the loop {self} on interpreter shutdown " |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace {self} with {self!r}. See for example BaseEventLoop.close() resource warning.
| @@ -0,0 +1 @@ |
|---|
| Dont unsubscribe signals in asyncio UNIX event loop on interpreter shutdown. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be written "Don't", no?
Notes are fixed.
Not sure about stacklevel too. With level 2 traceback points on __del__ most likely, with default it shows .close() explicitly.
| else: |
|---|
| warinigs.warn(f"Closing the loop {self!r} on interpreter shutdown " |
| f"stage, signal unsubsription is disabled", |
| ResourceWarning) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For ResourceWarning, you can add source=self, so the warning will be logged with the traceback were the event loop was created, if traceback is enabled.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
Thanks @asvetlov for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request
| @@ -51,8 +51,14 @@ def __init__(self, selector=None): |
|---|
| def close(self): |
| super().close() |
| for sig in list(self._signal_handlers): |
| self.remove_signal_handler(sig) |
| if not sys.is_finalizing(): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if someone calls loop.close() twice? I guess self._signal_handlers.clear() is needed.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double warning will be shown obviously.
But it is very rare situation: most likely the loop will be closed by __del__.
Even atexit callbacks are called when sys.is_finalizing() is False.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, in this case I'd like to clear the list of signal handlers.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, will make a postfix PR soon.
Not sure about stacklevel too. With level 2 traceback points on del most likely, with default it shows .close() explicitly.
And what's the point of showing 'close()' explicitly? __del__ gives you an idea that the warning is triggered by the GC.
I'd appreciate if patches like this are merged with my approval.
NP, Andrew, it's not a big deal. I trust you and Victor completely. It's just that I have a few follow-up questions that I wish we discussed before the merge just in case.
asvetlov pushed a commit that referenced this pull request
And what's the point of showing 'close()' explicitly?
__del__gives you an idea that the warning is triggered by the GC.
Stack trace points in __del__ too.
Any loop.close() at interpreter finalizing stage is dangerous and should be fixed. Well, not dangerous but very non-accurate, I'd like to discourage this antipattern.