gh-109709: Fix asyncio test_stdin_broken_pipe() #109710

vstinner · 2023-09-22T02:19:46Z

Replace harcoded sleep of 500 ms with synchronization using a pipe.

Issue: test_asyncio.test_subprocess: test_stdin_broken_pipe() failed on GHA Windows x64 CI #109709

Replace harcoded sleep of 500 ms with synchronization using a pipe. Fix also Process._feed_stdin(): catch also BrokenPipeError on stdin.write(input), not only on stdin.drain().

vstinner · 2023-09-22T02:41:17Z

On FreeBSD, without this fix, I reproduce the bug in less than 30 seconds with the command:

./python -m test test_asyncio.test_subprocess -m test_stdin_broken_pipe -m test_communicate_ignore_broken_pipe -j25 -F

With this fix, I can no longer reproduce the issue. I interrupted the test after 6 min 36 sec:

0:06:36 load avg: 27.14 [1341] test_asyncio.test_subprocess passed
^C

Well, while running this stress test I found a second issue, a a real bug in Process._feed_stdin(): it doesn't catch BrokenPipeError as promised when the error occurs on stdin.write(). I already updated my PR to include a fix.

vstinner · 2023-09-22T02:41:42Z

cc @kumaraditya303

vstinner · 2023-09-22T13:28:16Z

Lib/test/test_asyncio/test_subprocess.py

+            from subprocess import STARTUPINFO
+            startupinfo = STARTUPINFO()
+            startupinfo.lpAttributeList = {"handle_list": [handle]}
+            kwargs = dict(startupinfo=startupinfo)


On Windows, passing pipes as stdin, stdout and stderr is well supported. Passing an additional pipe is not supported by msvcrt (CreateProcess). Passing a handle is possible, but it requires to convert FD to handle and then handle to FD.

Could the process be synchronized in a way that requires less boilerplate for Windows? Is just calling terminate() not enough?

I like using a pipe as a sync primitive, but I'm unhappy by the quantity of code needed for that :-(

In practice, what is needed is that the child process hangs until the parent decides to terminate it in a clean fashion.

miss-islington · 2023-09-22T13:29:46Z

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

miss-islington · 2023-09-22T13:29:51Z

Sorry, @vstinner, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker cbbdf2c1440c804adcfc32ea0470865b3b3b8eb2 3.11

Replace harcoded sleep of 500 ms with synchronization using a pipe. Fix also Process._feed_stdin(): catch also BrokenPipeError on stdin.write(input), not only on stdin.drain(). (cherry picked from commit cbbdf2c) Co-authored-by: Victor Stinner <[email protected]>

bedevere-app · 2023-09-22T13:29:57Z

GH-109731 is a backport of this pull request to the 3.12 branch.

bedevere-app · 2023-09-22T13:33:40Z

GH-109735 is a backport of this pull request to the 3.11 branch.

…9735) gh-109709: Fix asyncio test_stdin_broken_pipe() (#109710) Replace harcoded sleep of 500 ms with synchronization using a pipe. Fix also Process._feed_stdin(): catch also BrokenPipeError on stdin.write(input), not only on stdin.drain(). (cherry picked from commit cbbdf2c)

sorcio

Yay for less magic sleeps and more actual synchronization!

I added a review comment on a bit that worries me (is transport.write() actually supposed to raise on broken pipe?).

sorcio · 2023-09-22T14:36:57Z

Lib/asyncio/subprocess.py

+            if input is not None:
+                self.stdin.write(input)
+                if debug:
+                    logger.debug(
+                        '%r communicate: feed stdin (%s bytes)', self, len(input))
+
            await self.stdin.drain()
        except (BrokenPipeError, ConnectionResetError) as exc:
-            # communicate() ignores BrokenPipeError and ConnectionResetError
+            # communicate() ignores BrokenPipeError and ConnectionResetError.
+            # write() and drain() can raise these exceptions.


Ok, if I understand correctly this change is here because write() could raise BrokenPipeError when registering a writer. Is this expected behavior? _UnixWritePipeTransport.write() tries hard not to raise if the os.write() call fails.

Could it be that this was not noticed on Linux because the error is raised only by the kqueue selector?

Sorry, I didn't keep the traceback. I got a BrokenPipeError on FreeBSD, yeah, it was somewhere in the kqueue selector. You can revert my change, and stress-test the test as I described in the issue / PR, to easily trigger the bug.

Could it be that this was not noticed on Linux because the error is raised only by the kqueue selector?

Maybe on Windows and Linux, write() cannot trigger these exceptions, but write() does on FreeBSD.

Simple snippet to trigger the exception:

import os, selectors sel = selectors.DefaultSelector() rfd, wfd = os.pipe() os.close(rfd) sel.register(wfd, selectors.EVENT_WRITE)

Tested on modern releases of Linux, macOS, FreeBSD, and I only see it raise on FreeBSD. See discussion here for some details: tokio-rs/mio#582 (older versions of macOS used to fail in the same case, and NetBSD/OpenBSD report different errors).

I think this should be considered a bug. _UnixWritePipeTransport.write() should not be expected to raise, otherwise exception handling code would have to be sprinkled all over user code.

In the linked issues (both Tokio and libevent) it's solved at an abstraction level similar to the selectors module in Python. If we agree it could be considered a bug, it's either a selectors bug, or an asyncio bug.

wait, registering a closed FD is bad: don't do that. It wasn't the issue that I got (I hope).

I think this should be considered a bug. _UnixWritePipeTransport.write() should not be expected to raise, otherwise exception handling code would have to be sprinkled all over user code.

If you think that there is a bug, please open a new issue.

It's not a closed FD! Only the other end of the pipe is closed. The write fd is still valid. On Linux/Mac you will always get an event, so you get the error when you try to write. On other BSDs you get the error preemptively, when registering.

Sorry i was confused between rfd and wfd.

I opened a new issue here #109757. I'd be glad to work on it.

sorcio · 2023-09-22T14:39:07Z

Lib/test/test_asyncio/test_subprocess.py

+            from subprocess import STARTUPINFO
+            startupinfo = STARTUPINFO()
+            startupinfo.lpAttributeList = {"handle_list": [handle]}
+            kwargs = dict(startupinfo=startupinfo)


Could the process be synchronized in a way that requires less boilerplate for Windows? Is just calling terminate() not enough?

Replace harcoded sleep of 500 ms with synchronization using a pipe. Fix also Process._feed_stdin(): catch also BrokenPipeError on stdin.write(input), not only on stdin.drain().

…109731) gh-109709: Fix asyncio test_stdin_broken_pipe() (GH-109710) Replace harcoded sleep of 500 ms with synchronization using a pipe. Fix also Process._feed_stdin(): catch also BrokenPipeError on stdin.write(input), not only on stdin.drain(). (cherry picked from commit cbbdf2c) Co-authored-by: Victor Stinner <[email protected]>

Replace harcoded sleep of 500 ms with synchronization using a pipe. Fix also Process._feed_stdin(): catch also BrokenPipeError on stdin.write(input), not only on stdin.drain().

vstinner requested review from 1st1, asvetlov, gvanrossum, kumaraditya303 and willingc as code owners September 22, 2023 02:19

bedevere-app bot added awaiting review tests Tests in the Lib/test dir labels Sep 22, 2023

vstinner added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes and removed tests Tests in the Lib/test dir labels Sep 22, 2023

bedevere-app bot mentioned this pull request Sep 22, 2023

test_asyncio.test_subprocess: test_stdin_broken_pipe() failed on GHA Windows x64 CI #109709

Closed

vstinner added tests Tests in the Lib/test dir topic-asyncio skip news labels Sep 22, 2023

pythongh-109709: Fix asyncio test_stdin_broken_pipe()

24b415d

Replace harcoded sleep of 500 ms with synchronization using a pipe. Fix also Process._feed_stdin(): catch also BrokenPipeError on stdin.write(input), not only on stdin.drain().

vstinner force-pushed the test_stdin_broken_pipe branch from 561d588 to 24b415d Compare September 22, 2023 02:32

Port code to Windows

441861c

vstinner commented Sep 22, 2023

View reviewed changes

vstinner merged commit cbbdf2c into python:main Sep 22, 2023

vstinner deleted the test_stdin_broken_pipe branch September 22, 2023 13:29

bedevere-app bot removed the awaiting review label Sep 22, 2023

miss-islington assigned vstinner Sep 22, 2023

bedevere-app bot removed the needs backport to 3.12 only security fixes label Sep 22, 2023

bedevere-app bot removed the needs backport to 3.11 only security fixes label Sep 22, 2023

sorcio reviewed Sep 22, 2023

View reviewed changes

sorcio mentioned this pull request Sep 22, 2023

[asyncio][FreeBSD] _UnixWritePipeTransport.write() may raise BrokenPipeError on FreeBSD #109757

Open

Uh oh!

gh-109709: Fix asyncio test_stdin_broken_pipe() #109710

gh-109709: Fix asyncio test_stdin_broken_pipe() #109710

Uh oh!

Conversation

vstinner commented Sep 22, 2023 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Sep 22, 2023

Uh oh!

vstinner commented Sep 22, 2023

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!

miss-islington commented Sep 22, 2023

Uh oh!

miss-islington commented Sep 22, 2023

Uh oh!

bedevere-app bot commented Sep 22, 2023

Uh oh!

bedevere-app bot commented Sep 22, 2023

Uh oh!

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

vstinner Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

vstinner commented Sep 22, 2023 •

edited by bedevere-app bot

Loading

vstinner Sep 22, 2023 •

edited

Loading