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

asvetlov · 2017-12-21T06:38:14Z

https://bugs.python.org/issue26133

1st1 · 2017-12-21T06:41:42Z

Lib/asyncio/unix_events.py

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


stacklevel arg?

vstinner · 2017-12-21T10:54:03Z

Lib/asyncio/unix_events.py

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


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?

vstinner · 2017-12-21T10:54:36Z

Lib/asyncio/unix_events.py

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


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

vstinner · 2017-12-21T10:55:12Z

Misc/NEWS.d/next/Library/2017-12-21-11-08-42.bpo-26133.mt81QV.rst

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


It should be written "Don't", no?

asvetlov · 2017-12-21T11:37:13Z

Notes are fixed.
Not sure about stacklevel too. With level 2 traceback points on __del__ most likely, with default it shows .close() explicitly.

vstinner · 2017-12-21T11:55:37Z

Lib/asyncio/unix_events.py

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


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.

vstinner

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.

bedevere-bot · 2017-12-21T12:04:58Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

miss-islington · 2017-12-21T15:07:30Z

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!

bedevere-bot · 2017-12-21T15:07:38Z

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

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

1st1 · 2017-12-21T15:42:26Z

Lib/asyncio/unix_events.py

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


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.

1st1 · 2017-12-21T15:45:07Z

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.

1st1 · 2017-12-21T15:45:27Z

I'd appreciate if patches like this are merged with my approval.

asvetlov · 2017-12-21T15:46:22Z

@1st1 sorry

1st1 · 2017-12-21T15:47:48Z

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)

asvetlov · 2017-12-21T17:49:18Z

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

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

Uh oh!

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

asvetlov commented Dec 21, 2017 •

edited by bedevere-bot

Loading

1st1 commented Dec 21, 2017 •

edited

Loading