bpo-26133: Dont unsubscribe signals in UNIX even loop on interpreter shutdown #4956
Conversation
Lib/asyncio/unix_events.py
Outdated
| self.remove_signal_handler(sig) | ||
| else: | ||
| warinigs.warn(f"Closing the loop {self} on interpreter shutdown " | ||
| f"stage, signal unsubsription is disabled") |
Lib/asyncio/unix_events.py
Outdated
| else: | ||
| warinigs.warn(f"Closing the loop {self} on interpreter shutdown " | ||
| f"stage, signal unsubsription is disabled", | ||
| stacklevel=2) |
There was a problem hiding this comment.
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?
Lib/asyncio/unix_events.py
Outdated
| for sig in list(self._signal_handlers): | ||
| self.remove_signal_handler(sig) | ||
| else: | ||
| warinigs.warn(f"Closing the loop {self} on interpreter shutdown " |
There was a problem hiding this comment.
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. | |||
There was a problem hiding this comment.
It should be written "Don't", no?
|
Notes are fixed. |
Lib/asyncio/unix_events.py
Outdated
| else: | ||
| warinigs.warn(f"Closing the loop {self!r} on interpreter shutdown " | ||
| f"stage, signal unsubsription is disabled", | ||
| ResourceWarning) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
LGTM.
It's hackish, but as I explained at https://bugs.python.org/issue26133#msg258429 it's hard to fix the root issue.
The hack is only used when the code doesn't explicitly close the event loop, which is a bug on itself. So it's ok to workaround https://bugs.python.org/issue26133 side effect.
|
When you're done making the requested changes, leave the comment: |
|
Thanks @asvetlov for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6. |
|
GH-4962 is a backport of this pull request to the 3.6 branch. |
…shutdown (pythonGH-4956) (cherry picked from commit 4a02543)
| @@ -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(): | |||
There was a problem hiding this comment.
What happens if someone calls loop.close() twice? I guess self._signal_handlers.clear() is needed.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Well, in this case I'd like to clear the list of signal handlers.
There was a problem hiding this comment.
Ok, will make a postfix PR soon.
And what's the point of showing 'close()' explicitly? |
|
I'd appreciate if patches like this are merged with my approval. |
|
@1st1 sorry |
|
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. |
Stack trace points in |
https://bugs.python.org/issue26133