bpo-30050: Allow disabling full buffer warnings in signal.set_wakeup_fd by njsmith · Pull Request #4792 · python/cpython

njsmith · 2017-12-11T07:49:37Z

Previously, if the wakeup fd's buffer overflowed, Python would
unconditionally print a warning on stderr. But for many of
set_wakeup_fd's users (e.g. Twisted, Tornado, and Trio), this is
incorrect: the way they use set_wakeup_fd, a full buffer is a totally
normal and unexceptional condition. On the other hand, for asyncio and
curio, a full buffer causes signals to be lost, so they really want to
issue a warning (at the least!).

This change lets the caller of set_wakeup_fd choose whether they would
like this warning to be printed or not.

https://bugs.python.org/issue30050

No semantic changes.

This variable is already set to this value directly above. No semantic change.

This commit has no behavioral changes. When a send() on Windows failed in signal handler context, we used to do a lot of hoop-jumping to pass both errno and the GetLastError() back out so they could be reported. But... send() on Windows does not set errno: "[If an error occurs], a value of SOCKET_ERROR is returned, and a specific error code can be retrieved by calling WSAGetLastError." -- https://msdn.microsoft.com/en-us/library/windows/desktop/ms740149(v=vs.85).aspx This allows the code to be substantially simplified. Also, there is no concept of EINTR on Windows, and even if there was then it wouldn't be reported through errno, so there's no need to loop checking for errno == EINTR.

njsmith · 2017-12-11T07:53:58Z

This PR will be easiest to review one commit at a time: each commit is self-contained, and the first few are brush-clearing refactorings.

(I think there's potential to simplify signalmodule.c even more by bringing the Windows and POSIX code closer together, and I also noticed that our SOCKET_T handling is rather dubious – we spend a lot of energy on using SOCKET_T on Windows and int on POSIX, but then at key points we cast SOCKET_T to int anyway. But that will have to wait for another time.)

vstinner

I like the overall change, but I have a few comments.

Would you mind to document the new optional parameter in the signal section of Doc/whatsnew/3.7.rst please?

vstinner · 2017-12-11T08:30:02Z

Modules/signalmodule.c

-    int send_errno;
-    int send_win_error;
-} wakeup = {INVALID_FD, 0, 0};
+} wakeup = {INVALID_FD, 1, 0};


I suggest to use C99 initializers:

wakeup = {.fd = INVALID_FD, .warn_on_full_buffer = 1, use_end=0};

vstinner · 2017-12-11T08:30:24Z

Modules/signalmodule.c

+static volatile struct {
+    sig_atomic_t fd;
+    int warn_on_full_buffer;
+} wakeup = {INVALID_FD, 1};


wakeup = {.fd = INVALID_FD, .warn_on_full_buffer = 1};

vstinner · 2017-12-11T08:42:24Z

Doc/library/signal.rst

+   the byte values give you the signal numbers. This is simple, but it
+   can run into a problem: generally the fd will have a limited amount
+   of buffer space, and if too many signals arrive too quickly, the
+   buffer may become full, and some signals may be lost. If you use


I propose to add something like "if many signals are received quickly, faster that what the application can read".

I never saw this warning, there is need to scare users ;-)

Tweaked the text to emphasize that this is a rare condition.

vstinner · 2017-12-11T08:42:49Z

Modules/signalmodule.c

-                Py_AddPendingCall(report_wakeup_write_error,
-                                  (void *)(intptr_t)errno);
+                if (wakeup.warn_on_full_buffer ||
+                    (errno != EWOULDBLOCK && errno != EAGAIN)) {


PEP 7: for multiline condition, please put { on a new line.

vstinner · 2017-12-11T08:45:59Z

Lib/test/test_signal.py

+        signum = signal.SIGINT
+
+        def handler(signum, frame):
+            pass


Please add a comment explaining that the signal handler is called but don't read pending signals from the socket.

vstinner · 2017-12-11T08:47:07Z

Lib/test/test_signal.py

+        # Fill the send buffer
+        try:
+            while True:
+                write.send(b"x" * 1024)


What is send(1024 bytes) fails with BlockingIOError, but send(1 bytes) works?

I suggest to loop on write.send(b"x") instead.

I don't think this can happen in any real system (you'd get a partial send instead), but I checked and sending 1 byte at a time is only ~2x slower, so why not. Done.

vstinner · 2017-12-11T08:49:00Z

Lib/test/test_signal.py

        assert_python_ok('-c', code)

+    @unittest.skipIf(_testcapi is None, 'need _testcapi')
+    def test_warn_on_full_buffer(self):


Please try to split this test into 3 tests, and handle stderr in the parent process. Running 3 tests in the same process may have side effects between tests.

I'd rather not? Part of what I want to test is that there are no side-effects between tests :-). (As there could be e.g. if warn_on_full_buffer values leaked between different calls to set_wakeup_fd.) And the handling of stderr within the child process is copied straight from the other wakeup fd warning tests in the same file.

vstinner · 2017-12-11T08:49:33Z

Modules/signalmodule.c

 {
    struct _Py_stat_struct status;
+    static char *kwlist[] = {
+        "", "warn_on_full_buffer", NULL,


Please write an unit test to make sure that it's not possible to pass the signal number as a keyword argument.

Dummy test like: with self.assertRaises(TypeError): signal.set_wakeup_fd(signum=signal.SIGINT)

vstinner · 2017-12-11T08:52:15Z

@pitrou, @serhiy-storchaka: I would feel more confortable if this change would be double checked, since signals are complex and is a critical component.

IHMO it's still ok to add the new parameter in Python 3.7.

vstinner

LGTM, just a few remaining nitpicking.

But as I wrote, I would prefer to get review of at least a second core developer.

vstinner · 2017-12-11T11:38:17Z

Doc/library/signal.rst

      On Windows, the function now also supports socket handles.

+   .. versionchanged:: 3.7
+      Added ``warn_on_full_buffer``.


Added warn_on_full_buffer parameter.

vstinner · 2017-12-11T11:39:53Z

Lib/test/test_signal.py


+    def test_invalid_call(self):
+        with self.assertRaises(TypeError):
+            signal.set_wakeup_fd(signum=signal.SIGINT)


I suggest a short comment: "# first parameter is positional-only"

vstinner · 2017-12-11T11:40:34Z

Lib/test/test_signal.py

+            signal.set_wakeup_fd(signum=signal.SIGINT)
+
+        with self.assertRaises(TypeError):
+            signal.set_wakeup_fd(signal.SIGINT, False)


I suggest a short comment: "# warn_on_full_buffer is a keyword-only parameter"

vstinner · 2017-12-11T11:41:38Z

Lib/test/test_signal.py

        assert_python_ok('-c', code)

+    @unittest.skipIf(_testcapi is None, 'need _testcapi')
+    def test_warn_on_full_buffer(self):


vstinner · 2017-12-11T11:43:01Z

Ah, there are some issues on Windows. AppVeyor failed to compile:

..\Modules\signalmodule.c(219): error C2065: 'res': undeclared identifier [C:\projects\cpython\PCbuild\pythoncore.vcxproj]
..\Modules\signalmodule.c(219): warning C4047: '=': 'int' differs in levels of indirection from 'PyObject *' [C:\projects\cpython\PCbuild\pythoncore.vcxproj]

njsmith · 2017-12-12T05:51:15Z

Huh, weird that appveyor didn't highlight that the first time! Anyway, fixed the appeyor build (I hope) and addressed all comments.

vstinner · 2017-12-12T08:44:32Z

Huh, weird that appveyor didn't highlight that the first time! Anyway, fixed the appeyor build (I hope) and addressed all comments.

AppVeyor didn't run the first time. There is an heuristic to not run AppVeyor on changes which are only documentation or specific to Linux. The heuristic isn't perfect :-)

vstinner

LGTM.

njsmith added 6 commits December 10, 2017 21:57

signalmodule.c refactor: share more code between Windows and POSIX

4e6297b

No semantic changes.

refactor: remove redundant variable assignment

f7cdf02

This variable is already set to this value directly above. No semantic change.

signalmodule.c: Add warn_on_full_buffer support

3a9ebe3

Add test for set_wakeup_fd(..., warn_on_full_buffer=True/False)

73a5391

Document set_wakeup_fd(..., warn_on_full_buffer=True/False)

1eca2f3

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

bedevere-bot added the awaiting review label Dec 11, 2017

vstinner reviewed Dec 11, 2017

View reviewed changes

Address @vstinner's review comments

3c2cd18

vstinner approved these changes Dec 11, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Dec 11, 2017

njsmith added 2 commits December 11, 2017 21:48

Fix windows build

f8ac672

Address @vstinner's second round of review comments

99b34ee

vstinner approved these changes Dec 12, 2017

View reviewed changes

1st1 approved these changes Dec 18, 2017

View reviewed changes

1st1 merged commit 902ab80 into python:master Dec 18, 2017

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

njsmith mentioned this pull request Jan 18, 2018

Maybe use signal.set_wakeup_fd on unix-likes python-trio/trio#109

Closed

Uh oh!

Comments

Conversation

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

Uh oh!

Uh oh!

njsmith commented Dec 11, 2017

Uh oh!

vstinner left a comment

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!

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!

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!

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!

vstinner commented Dec 11, 2017

Uh oh!

vstinner left a comment

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!

vstinner commented Dec 11, 2017

Uh oh!

njsmith commented Dec 12, 2017

Uh oh!

vstinner commented Dec 12, 2017

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

njsmith commented Dec 11, 2017 •

edited by bedevere-bot

Loading