bpo-26133: Dont unsubscribe signals in UNIX even loop on interpreter shutdown by asvetlov · Pull Request #4956 · python/cpython · GitHub

https://bugs.python.org/issue26133

stacklevel arg?

Sure! :)

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?

Please replace {self} with {self!r}. See for example BaseEventLoop.close() resource warning.

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.

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.

Done

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: 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!

GH-4962 is a backport of this pull request to the 3.6 branch.

…shutdown (pythonGH-4956) (cherry picked from commit 4a02543)

What happens if someone calls loop.close() twice? I guess self._signal_handlers.clear() is needed.

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.

Well, in this case I'd like to clear the list of signal handlers.

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.

@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.

…shutdown (GH-4956) (#4962) (cherry picked from commit 4a02543)

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.

Dont unsubscribe signals in UNIX even loop on interpreter shutdown

756b93d

asvetlov requested a review from 1st1 as a code owner December 21, 2017 06:38

the-knights-who-say-ni added the CLA signed label Dec 21, 2017

bedevere-bot added the awaiting merge label Dec 21, 2017

asvetlov requested a review from vstinner December 21, 2017 06:38

1st1 reviewed Dec 21, 2017

View reviewed changes

asvetlov added 2 commits December 21, 2017 09:16

Add stacklevel 2 for raised message

b2b4a7e

Add NEWS entry

0a0b8c9

vstinner reviewed Dec 21, 2017

View reviewed changes

Fix notes

41a9f22

vstinner reviewed Dec 21, 2017

View reviewed changes

Add source-self

628d68e

vstinner requested changes Dec 21, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Dec 21, 2017

vstinner approved these changes Dec 21, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Dec 21, 2017

asvetlov merged commit 4a02543 into python:master Dec 21, 2017

bedevere-bot removed the awaiting merge label Dec 21, 2017

asvetlov deleted the bpo-26133 branch December 21, 2017 15:06

asvetlov added topic-asyncio needs backport to 3.6 labels Dec 21, 2017

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 21, 2017

bpo-26133: Dont unsubscribe signals in UNIX even loop on interpreter …

aeed967

…shutdown (pythonGH-4956) (cherry picked from commit 4a02543)

bedevere-bot removed the needs backport to 3.6 label Dec 21, 2017

1st1 reviewed Dec 21, 2017

View reviewed changes

asvetlov pushed a commit that referenced this pull request Dec 21, 2017

bpo-26133 : Dont unsubscribe signals in UNIX even loop on interpreter …

3bc68cf

…shutdown (GH-4956) (#4962) (cherry picked from commit 4a02543)

asvetlov mentioned this pull request Dec 24, 2017

bpo-26133: Clear signals list on interpreter finalizing #5002

Merged

+                              self.remove_signal_handler(sig)
+                      else:
+                          warinigs.warn(f"Closing the loop {self} on interpreter shutdown "
+                                        f"stage, signal unsubsription is disabled")

+                      else:
+                          warinigs.warn(f"Closing the loop {self} on interpreter shutdown "
+                                        f"stage, signal unsubsription is disabled",
+                                        stacklevel=2)

+                          for sig in list(self._signal_handlers):
+                              self.remove_signal_handler(sig)
+                      else:
+                          warinigs.warn(f"Closing the loop {self} on interpreter shutdown "

+                      else:
+                          warinigs.warn(f"Closing the loop {self!r} on interpreter shutdown "
+                                        f"stage, signal unsubsription is disabled",
+                                        ResourceWarning)

                       super().close()
-                      for sig in list(self._signal_handlers):
-                          self.remove_signal_handler(sig)
+                      if not sys.is_finalizing():

		@@ -0,0 +1 @@
		Dont unsubscribe signals in asyncio UNIX event loop on interpreter shutdown.

bpo-26133: Dont unsubscribe signals in UNIX even loop on interpreter shutdown by asvetlov · Pull Request #4956 · python/cpython · GitHub

Repository navigation

bpo-26133: Dont unsubscribe signals in UNIX even loop on interpreter shutdown - #4956

bpo-26133: Dont unsubscribe signals in UNIX even loop on interpreter shutdown#4956asvetlov merged 5 commits intopython:masterpython/cpython:masterfrom asvetlov:bpo-26133asvetlov/cpython:bpo-26133Copy head branch name to clipboard

Conversation

asvetlov commented Dec 21, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asvetlov commented Dec 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Dec 21, 2017

Uh oh!

miss-islington commented Dec 21, 2017

Uh oh!

bedevere-bot commented Dec 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 commented Dec 21, 2017

Uh oh!

1st1 commented Dec 21, 2017

Uh oh!

asvetlov commented Dec 21, 2017

Uh oh!

1st1 commented Dec 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asvetlov commented Dec 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bpo-26133: Dont unsubscribe signals in UNIX even loop on interpreter shutdown#4956
asvetlov merged 5 commits into
python:masterfrom
asvetlov:bpo-26133

asvetlov commented Dec 21, 2017 •
edited by bedevere-bot
Loading

1st1 commented Dec 21, 2017 •
edited
Loading