-
-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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.
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
gh-121313: Limit the reading size from pipes to their default buffer size on Unix systems #121315
Changes from 12 commits
108e65b
37ca606
df4f307
89936c2
31c65b4
0a4e4a3
7afde51
8d9b16e
e6c64fe
936b601
49d8adb
43e19dd
b0b86e5
2b6ff24
e726f51
59cff4d
da10f8e
94d4c4a
52606d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ | |
import time | ||
import tempfile | ||
import itertools | ||
import stat | ||
|
||
|
||
from . import util | ||
|
@@ -391,8 +392,17 @@ def _recv(self, size, read=_read): | |
buf = io.BytesIO() | ||
handle = self._handle | ||
remaining = size | ||
is_pipe = False | ||
page_size = 0 | ||
if not _winapi: | ||
page_size = os.sysconf(os.sysconf_names['SC_PAGESIZE']) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than do the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've shifted fetching the base page size and calculating the default pipe size to the existing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, looking good |
||
if size > 16 * page_size: | ||
mode = os.fstat(handle).st_mode | ||
is_pipe = stat.S_ISFIFO(mode) | ||
limit = 16 * page_size if is_pipe else remaining | ||
while remaining > 0: | ||
chunk = read(handle, remaining) | ||
to_read = min(limit, remaining) | ||
chunk = read(handle, to_read) | ||
n = len(chunk) | ||
if n == 0: | ||
if remaining == size: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Limit reading size in os.read for pipes to default pipe size in order to avoid memory overallocation | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be updated from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
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.
Personal nitpick, PEP-8 doesn't seem to specify (https://peps.python.org/pep-0008/#imports), but I like imports to be alphabetical.
itertools
,time
, andtempfile
which were already in the code just above this are also out of order (although time and tempfile only slightly). Rest are in order. Not sure if it matters for Python core developer acceptanceThere 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.
Done