GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-26133: Dont unsubscribe signals in UNIX even loop on interpreter shutdown by asvetlov · Pull Request #4956 · python/cpython

Conversation

asvetlov commented Dec 21, 2017 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

https://bugs.python.org/issue26133

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          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

Comment thread

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")

1st1 Dec 21, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stacklevel arg?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

srinivasreddy reacted with thumbs up emoji

All reactions

👍 1 reaction

asvetlov Dec 21, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

Comment thread

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)

vstinner Dec 21, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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 "

vstinner Dec 21, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

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

vstinner Dec 21, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Fix notes

41a9f22

asvetlov commented Dec 21, 2017

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner reviewed

Dec 21, 2017

View reviewed changes

Comment thread

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)

vstinner Dec 21, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

asvetlov Dec 21, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Add source-self

628d68e

vstinner requested changes

Dec 21, 2017

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot added awaiting changes and removed awaiting merge labels

Dec 21, 2017

bedevere-bot commented Dec 21, 2017

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 commented Dec 21, 2017

Copy link

Copy Markdown

Contributor

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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot commented Dec 21, 2017

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Comment thread

Lib/asyncio/unix_events.py

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

1st1 Dec 21, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

asvetlov Dec 21, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

1st1 Dec 21, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

asvetlov Dec 21, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

1st1 commented Dec 21, 2017

Copy link

Copy Markdown

Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

1st1 commented Dec 21, 2017

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

asvetlov commented Dec 21, 2017

Copy link

Copy Markdown

Contributor Author

@1st1 sorry

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

1st1 commented Dec 21, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 commented Dec 21, 2017

Copy link

Copy Markdown

Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

asvetlov mentioned this pull request

Dec 24, 2017

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

Merged

Reviewers

1st1

1st1 left review comments

vstinner

vstinner approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

topic-asyncio

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

asvetlov commented Dec 21, 2017 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

1st1 commented Dec 21, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.