GeezerVison

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

GeezerVison is showing github.com — gh-109709: Fix asyncio test_stdin_broken_pipe() by vstinner · Pull Request #109710 · python/cpython

Conversation

vstinner commented Sep 22, 2023 •

edited by bedevere-app Bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

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

All reactions

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

vstinner commented Sep 22, 2023

Copy link

Copy Markdown

Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented Sep 22, 2023

Copy link

Copy Markdown

Member Author

cc @kumaraditya303

All reactions

Sorry, something went wrong.

Uh oh!

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


          Port code to Windows

441861c

vstinner commented

Sep 22, 2023

View reviewed changes

Comment thread

Lib/test/test_asyncio/test_subprocess.py

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

vstinner Sep 22, 2023

Copy link

Copy Markdown

Member 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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

sorcio Sep 22, 2023

Copy link

Copy Markdown

Contributor

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner Sep 22, 2023

Copy link

Copy Markdown

Member 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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner merged commit cbbdf2c into python:main

Sep 22, 2023

vstinner deleted the test_stdin_broken_pipe branch

September 22, 2023 13:29

miss-islington commented Sep 22, 2023

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-app Bot removed the awaiting review label

Sep 22, 2023

miss-islington commented Sep 22, 2023

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

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

miss-islington assigned vstinner

Sep 22, 2023

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Sep 22, 2023


          pythongh-109709: Fix asyncio test_stdin_broken_pipe() (pythonGH-109710)

8aa841d

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 Bot commented Sep 22, 2023

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-app Bot removed the needs backport to 3.12 only security fixes label

Sep 22, 2023

bedevere-app Bot commented Sep 22, 2023

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-app Bot removed the needs backport to 3.11 only security fixes label

Sep 22, 2023

vstinner added a commit that referenced this pull request

Sep 22, 2023


          [3.11] gh-109709: Fix asyncio test_stdin_broken_pipe() (#109710) (#10…

1629b1d

…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 reviewed

Sep 22, 2023

View reviewed changes

sorcio left a comment

Copy link

Copy Markdown

Contributor

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.

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?).

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/asyncio/subprocess.py

Comment on lines +151 to +160

		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.

sorcio Sep 22, 2023

Copy link

Copy Markdown

Contributor

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, 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, something went wrong.

Uh oh!

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

All reactions

vstinner Sep 22, 2023

Copy link

Copy Markdown

Member 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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

sorcio Sep 22, 2023

Copy link

Copy Markdown

Contributor

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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner Sep 22, 2023 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member 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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner Sep 22, 2023

Copy link

Copy Markdown

Member 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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

sorcio Sep 22, 2023

Copy link

Copy Markdown

Contributor

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'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, something went wrong.

Uh oh!

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

All reactions

vstinner Sep 22, 2023

Copy link

Copy Markdown

Member 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.

Sorry i was confused between rfd and wfd.

Sorry, something went wrong.

Uh oh!

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

All reactions

sorcio Sep 23, 2023

Copy link

Copy Markdown

Contributor

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 opened a new issue here #109757. I'd be glad to work on it.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/test/test_asyncio/test_subprocess.py

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

sorcio Sep 22, 2023

Copy link

Copy Markdown

Contributor

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

sorcio mentioned this pull request

Sep 22, 2023

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

Open

csm10495 pushed a commit to csm10495/cpython that referenced this pull request

Sep 28, 2023


          pythongh-109709: Fix asyncio test_stdin_broken_pipe() (python#109710)

ba232d5

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().

Yhg1s pushed a commit that referenced this pull request

Oct 2, 2023


          [3.12] gh-109709: Fix asyncio test_stdin_broken_pipe() (GH-109710) (#…

c7cbd82

…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]>

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request

Sep 2, 2024


          pythongh-109709: Fix asyncio test_stdin_broken_pipe() (python#109710)

e4bb702

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().

Reviewers

1st1

Awaiting requested review from 1st1

1st1 is a code owner

asvetlov

Awaiting requested review from asvetlov

asvetlov is a code owner

gvanrossum

Awaiting requested review from gvanrossum

kumaraditya303

Awaiting requested review from kumaraditya303

kumaraditya303 is a code owner

willingc

Awaiting requested review from willingc

willingc is a code owner

1 more reviewer

sorcio

sorcio left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

vstinner

Labels

skip news tests

Tests in the Lib/test dir

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.

3 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

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!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vstinner commented Sep 22, 2023 •

edited by bedevere-app Bot

Loading

Uh oh!

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

vstinner Sep 22, 2023 •

edited

Loading

Uh oh!

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