gh-87079: Warn on unintended signal wakeup fd override in `asyncio` #96807

hidmic · 2022-09-13T19:11:10Z

Addresses #87079, with a relatively minor change of heart: this patch warns instead of raising an exception. I figured quite a few users may be inadvertently (or even consciously) relying on signal wakeup file descriptor overrides, and an exception may be inconvenient. It turns out even asyncio tests do this, to some extent.

Warnings fulfill the intent of notifying the user and can be trivially promoted to an exception.

Issue: asyncio.ProactorEventLoop mishandles signal wakeup file descriptor #87079

bedevere-bot · 2022-09-13T19:11:14Z

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

ghost · 2022-09-13T19:11:41Z

All commit authors signed the Contributor License Agreement.

hidmic · 2022-09-13T19:12:01Z

Lib/asyncio/unix_events.py

@@ -77,6 +76,7 @@ def close(self):
                              ResourceWarning,
                              source=self)
                self._signal_handlers.clear()
+        super().close()


Otherwise, an active signal wakeup file descriptor may be scrapped too early.

At the very least this deserves to be a comment in the source code. IIUC super().close() is selector_events.BaseSelectorEventLoop.close(), which closes the socketpair, and that would break the check you've added to remove_signal_handler(). But I'm not sure I like that check, see below.

Comment added in 3b4975a.

gvanrossum · 2022-09-14T04:11:11Z

@hidmic Can you sign the CLA and add a news blurb? CLick on Details for the failing tests for instructions.

bedevere-bot · 2022-09-14T22:10:08Z

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

hidmic · 2022-09-14T22:19:00Z

Can you sign the CLA and add a news blurb?

@gvanrossum done and done.

gvanrossum · 2022-09-14T22:31:46Z

Lib/asyncio/proactor_events.py

+                    'Overriding signal wakeup file '
+                    'descriptor for loop signal handlers',


I no longer like string literals split across lines like this, it makes phrase searching harder. Also I'm trying to clarify the warning.

Suggested change

'Overriding signal wakeup file '

'descriptor for loop signal handlers',

"Signal wakeup fd was already set",

Done in bc7b849.

gvanrossum · 2022-09-14T22:36:16Z

Lib/asyncio/unix_events.py

+                    'Overriding signal wakeup file '
+                    'descriptor for loop signal handlers',


Suggested change

'Overriding signal wakeup file '

'descriptor for loop signal handlers',

"Signal wakeup fd was already set",

Done in bc7b849.

gvanrossum · 2022-09-14T22:45:46Z

Lib/asyncio/unix_events.py

@@ -77,6 +76,7 @@ def close(self):
                              ResourceWarning,
                              source=self)
                self._signal_handlers.clear()
+        super().close()


At the very least this deserves to be a comment in the source code. IIUC super().close() is selector_events.BaseSelectorEventLoop.close(), which closes the socketpair, and that would break the check you've added to remove_signal_handler(). But I'm not sure I like that check, see below.

gvanrossum · 2022-09-14T22:49:10Z

Lib/asyncio/unix_events.py

-                signal.set_wakeup_fd(-1)
+                oldfd = signal.set_wakeup_fd(-1)
+                if oldfd != -1 and oldfd != self._csock.fileno():
+                    warnings.warn(
+                        'Got unexpected signal wakeup file '
+                        'descriptor while removing it',
+                        ResourceWarning,
+                        source=self)


I don't really like warnings being issued when closing things -- they aren't very actionable. I understand this is trying to alert the user that something else might have set the wakeup fd, overriding ours. But if we made it this far there's not much we can do about it, is there?

(OTOH maybe this could be logged in debug mode?)

But if we made it this far there's not much we can do about it, is there?

That's fair, though I don't expect the user to act on the warning programmatically, even if they could. It's a hint.

OTOH maybe this could be logged in debug mode?

I think that reduces the value of the hint. I would like to know that my application is potentially broken ASAP, rather than waiting for it to manifest itself somehow to even consider looking at debug logs.

Fair enough. I'll hold off for a little bit and then merge.

gvanrossum · 2022-09-14T22:52:22Z

Lib/asyncio/proactor_events.py

@@ -684,7 +690,13 @@ def close(self):
            return

        if threading.current_thread() is threading.main_thread():
-            signal.set_wakeup_fd(-1)
+            oldfd = signal.set_wakeup_fd(-1)
+            if oldfd == self._csock.fileno():


Is this maybe backwards? In the unix code you use !=.

Separately, the same concern about warnings during closing as for the unix version.

And maybe the error message doesn't have to be split (it's not the end of the world if it's a little too long :-).

Is this maybe backwards? In the unix code you use !=.

Indeed 🤦‍♂️ Fixed in bc7b849.

gvanrossum · 2022-09-14T22:55:35Z

Lib/test/test_asyncio/test_proactor_events.py

-        self.loop.close()
+        with mock.patch('signal.set_wakeup_fd') as set_wakeup_fd:
+            set_wakeup_fd.return_value = -1000
+            self.loop.close()


I guess this test needs to row an assertWarnsRegex call? (See my comment on the code in the file under test.)

Yeap, that's right too... This is an incredibly sloppy patch. It shows I did it on a rush. Sorry for that. Fixed in bc7b849.

kumaraditya303 · 2022-09-17T10:16:47Z

Misc/NEWS.d/next/Library/2022-09-14-19-15-01.gh-issue-87079.0zYmW5.rst

+Warn the user whenever asyncio event loops override a signal wake up file
+descriptor that was previously set.


Suggested change

Warn the user whenever asyncio event loops override a signal wake up file

descriptor that was previously set.

:mod:`asyncio` now emits :exc:`ResourceWarning` when overriding signal wakeup fd if it was previously set.

Well dang I missed this comment and merged without applying that. Sorry about that (I fail at scrolling). I think this isn't so important that we need to send another PR, but if you insist I'll gladly merge it.

No worries, it was just a minor suggestion.

kumaraditya303

LGTM with minor news suggestion.

gvanrossum · 2022-09-17T15:08:35Z

Thanks @hidmic for the patch!

kumaraditya303 · 2022-09-17T15:55:09Z

Seems like this broke some openssl bots. See https://github.com/python/cpython/actions/runs/3073806792/jobs/4966172052 on main

gvanrossum · 2022-09-17T17:08:46Z

Seems like this broke some openssl bots. See https://github.com/python/cpython/actions/runs/3073806792/jobs/4966172052 on main

Oh, that's unfortunate. Unless either of you has a quick fix, we should probably revert.

gvanrossum · 2022-09-17T20:37:27Z

I can repro the failure by running ./python.exe Lib/test/ssltests.py on my Mac. I'm just going to revert this, so you can come up with a fix without stressing out about it.

…yncio` (#96807)" This reverts commit 0587810.

…yncio` (#96807)" (#96898) This reverts commit 0587810. Reason: This broke buildbots (some warnings added by that commit are turned to errors in the SSL buildbot). Repro: ./python Lib/test/ssltests.py

hidmic requested review from 1st1, asvetlov and gvanrossum as code owners September 13, 2022 19:11

bedevere-bot added the awaiting review label Sep 13, 2022

hidmic commented Sep 13, 2022

View reviewed changes

hidmic mentioned this pull request Sep 13, 2022

asyncio.ProactorEventLoop mishandles signal wakeup file descriptor #87079

Open

Warn on unintended signal wakeup fd override in asyncio

bec6ccd

hidmic force-pushed the cope-with-issue-87079 branch from 3f64700 to bec6ccd Compare September 14, 2022 22:10

hidmic added 2 commits September 14, 2022 19:15

Update NEWS

7badd94

Merge branch 'main' into cope-with-issue-87079

60bd836

gvanrossum reviewed Sep 14, 2022

View reviewed changes

hidmic added 3 commits September 16, 2022 19:52

Address peer review comments

bc7b849

Comment on _UnixSelectorEventLoop.close() logic

3b4975a

Merge branch 'main' into cope-with-issue-87079

428cb7a

gvanrossum approved these changes Sep 17, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Sep 17, 2022

kumaraditya303 added the topic-asyncio label Sep 17, 2022

kumaraditya303 reviewed Sep 17, 2022

View reviewed changes

kumaraditya303 approved these changes Sep 17, 2022

View reviewed changes

gvanrossum merged commit 0587810 into python:main Sep 17, 2022

bedevere-bot removed the awaiting merge label Sep 17, 2022

gvanrossum added a commit that referenced this pull request Sep 17, 2022

Revert "gh-87079: Warn on unintended signal wakeup fd override in `as…

8cf0fc2

…yncio` (#96807)" This reverts commit 0587810.

gvanrossum mentioned this pull request Sep 17, 2022

Revert "gh-87079: Warn on unintended signal wakeup fd override in asyncio" #96898

Merged

hidmic deleted the cope-with-issue-87079 branch September 17, 2022 22:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-87079: Warn on unintended signal wakeup fd override in `asyncio` #96807

gh-87079: Warn on unintended signal wakeup fd override in `asyncio` #96807

hidmic commented Sep 13, 2022 •

edited by bedevere-bot

Loading

bedevere-bot commented Sep 13, 2022

ghost commented Sep 13, 2022 •

edited by ghost

Loading

hidmic Sep 13, 2022

gvanrossum Sep 14, 2022

hidmic Sep 16, 2022

gvanrossum commented Sep 14, 2022

bedevere-bot commented Sep 14, 2022

hidmic commented Sep 14, 2022

gvanrossum Sep 14, 2022

hidmic Sep 16, 2022

gvanrossum Sep 14, 2022

hidmic Sep 16, 2022

gvanrossum Sep 14, 2022

gvanrossum Sep 14, 2022

hidmic Sep 16, 2022

gvanrossum Sep 17, 2022

gvanrossum Sep 14, 2022

hidmic Sep 16, 2022

gvanrossum Sep 14, 2022

hidmic Sep 16, 2022

kumaraditya303 Sep 17, 2022

gvanrossum Sep 17, 2022

kumaraditya303 Sep 17, 2022

kumaraditya303 left a comment

gvanrossum commented Sep 17, 2022

kumaraditya303 commented Sep 17, 2022

gvanrossum commented Sep 17, 2022

gvanrossum commented Sep 17, 2022

		'Overriding signal wakeup file '
		'descriptor for loop signal handlers',

	'Overriding signal wakeup file '
	'descriptor for loop signal handlers',
	"Signal wakeup fd was already set",

		Warn the user whenever asyncio event loops override a signal wake up file
		descriptor that was previously set.

	Warn the user whenever asyncio event loops override a signal wake up file
	descriptor that was previously set.
	:mod:`asyncio` now emits :exc:`ResourceWarning` when overriding signal wakeup fd if it was previously set.

gh-87079: Warn on unintended signal wakeup fd override in asyncio #96807

gh-87079: Warn on unintended signal wakeup fd override in asyncio #96807

Conversation

hidmic commented Sep 13, 2022 • edited by bedevere-bot Loading

bedevere-bot commented Sep 13, 2022

ghost commented Sep 13, 2022 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum commented Sep 14, 2022

bedevere-bot commented Sep 14, 2022

hidmic commented Sep 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumaraditya303 left a comment

Choose a reason for hiding this comment

gvanrossum commented Sep 17, 2022

kumaraditya303 commented Sep 17, 2022

gvanrossum commented Sep 17, 2022

gvanrossum commented Sep 17, 2022

gh-87079: Warn on unintended signal wakeup fd override in `asyncio` #96807

gh-87079: Warn on unintended signal wakeup fd override in `asyncio` #96807

hidmic commented Sep 13, 2022 •

edited by bedevere-bot

Loading

ghost commented Sep 13, 2022 •

edited by ghost

Loading