10000 gh-129205: Use os.readinto() in subprocess errpipe_read by cmaloney · Pull Request #129498 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-129205: Use os.readinto() in subprocess errpipe_read #129498

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

gh-129205: Use os.readinto() in subprocess errpipe_read #129498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c06287e
gh-129205: Use os.readinto() in subprocess errpipe_read
cmaloney Jan 31, 2025
c4cb414
📜🤖 Added by blurb_it.
blurb-it[bot] Feb 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions Lib/subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1921,12 +1921,13 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,

# Wait for exec to fail or succeed; possibly raising an
# exception (limited in size)
errpipe_data = bytearray()
Copy link
Member

Choose a reason for hiding this comment

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

There was nothing special about the 50_000 in the existing code. I put it in there in order to be fail safe and have some limit on this pipe in case something weird happened as unbounded reads are unwise. In reality the data coming through this error pipe will always be tiny in normally operating processes on normally operating machines. This code is not trying to prevent allocations or limit allocation to a specific size. It's just a failsafe.

This PR would cause it to always allocate and zero fill excessive memory during normal operation.

The norm is that the exception info sent through the pipe, if any, will be smaller than PIPEBUF (512) and show up in its entirety in the first read cal. So this loop isn't expected to do much, the second += if there was an error to report will be a += b'' no-op before it breaks out of the loop.

If the existing bounded os.read calls happen to pre-allocate 50k each, they at least should not be zero filling those. we could change the number to 3500 and be fine if that allocation size actually matters to anyone. I don't think it does.

Copy link
Member

Choose a reason for hiding this comment

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

Basically: I think readinto is a regression in this use case. It is normally 0 data read, and always tiny data when there is something to read. The limit is merely a fail-safe. If you have identified performance issues due to the current values, propose lowering them. But lets not complicate things with a pre-allocated zero-filled bytearray and readinto. the normal pattern of system calls will be either:

read(fd) -> 0  (closed)

or with a very rare error to be reported:

read(fd) -> 20-60ish bytes.
read(fd) -> 0 (closed)

The loop is a fail-safe because that's how we're supposed to use read APIs on pipes rather than relying on the OS mapping a single small <PIPEBUF write to a single small <PIPEBUF read on the other end, even though in practice that is what'll happen.

Copy link
Contributor Author
@cmaloney cmaloney Feb 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Neither bytearray nor os.read forces a zero fill currently, they just do PyMem_Malloc. bytearray does set the byte past the end to zero / guarantee the byte array is null terminated, but it doesn't zero-set the whole array.

bytearray construction: https://github.com/python/cpython/blob/main/Objects/bytearrayobject.c#L136-L152

os.read:
os_read_impl: https://github.com/python/cpython/blob/main/Modules/posixmodule.c#L11436-L11463
PyBytes_FromStringandSize: https://github.com/python/cpython/blob/main/Objects/bytesobject.c#L136-L161
_PyBytes_FromSize: https://github.com/python/cpython/blob/main/Objects/bytesobject.c#L119-L133
_PyObject_InitVar: https://github.com/python/cpython/blob/main/Include/internal/pycore_object.h#L430-L447

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can definitely lower the initial allocation from 50_000, the os.read currently always allocates that much as well (then downsizes to the actual read size). I'll have a look at making and testing a better errpipe upper bound size.

while True:
part = os.read(errpipe_read, 50000)
errpipe_data += part
if not part or len(errpipe_data) > 50000:
break
errpipe_data = bytearray(50000)
unread = memoryview(errpipe_data)
while count := os.readinto(errpipe_read, unread):
unread = unread[count:]
bytes_left = len(unread)
del unread
del errpipe_data[-bytes_left:]
finally:
# be sure the FD is closed no matter what
os.close(errpipe_read)
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2025-02-02-20-55-18.gh-issue-129205.hb1iMy.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
:mod:`subprocess` forkserver now uses a fixed 50,000 byte buffer to read startup errors from the child processes it runs. Previously there could be up to 100,000 bytes read with multiple allocations and copies.
Loading
0