Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(257279)

#18923: Use the new selectors module in the subprocess module

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 10 months ago by vstinner
Modified:
5 years, 9 months ago
Reviewers:
CC:
AntoinePitrou, haypo, giampaolo.rodola, Charles-Fran├žois Natali, devnull_psf.upfronthosting.co.za
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Total comments: 4

Patch Set 3 #

Patch Set 4 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Lib/subprocess.py View 1 2 3 4 chunks +71 lines, -168 lines 0 comments Download
Lib/test/test_subprocess.py View 1 2 3 2 chunks +6 lines, -4 lines 0 comments Download

Messages

Total messages: 1
victor.stinner_gmail.com
5 years, 9 months ago #1
http://bugs.python.org/review/18923/diff/9757/Lib/subprocess.py
File Lib/subprocess.py (right):

http://bugs.python.org/review/18923/diff/9757/Lib/subprocess.py#newcode420
Lib/subprocess.py:420: _PopenSelector = selectors.SelectSelector
This code should be factorized, but I opened a new issue for that: #19465.
http://bugs.python.org/issue19465

http://bugs.python.org/review/18923/diff/9757/Lib/subprocess.py#newcode1583
Lib/subprocess.py:1583: self._input_offset + _PIPE_BUF]
(Unrelated to the selector issue) It may be more efficient to use a memory view
instead of a slice here.

http://bugs.python.org/review/18923/diff/9757/Lib/subprocess.py#newcode1597
Lib/subprocess.py:1597: data = os.read(key.fd, 4096)
In the old code, poll() uses 4096 whereas select() uses 1024 bytes... Do you
know the reason? Why 4096 and not 1 byte or 1 MB? I would prefer a global
constant rather than an harded limit.

Use maybe os.stat(fd).st_blksize?

Modules/_io/fileio.c uses SMALLCHUNK to read the whole content of a file:

#if BUFSIZ < (8*1024)
#define SMALLCHUNK (8*1024)
#elif (BUFSIZ >= (2 << 25))
#error "unreasonable BUFSIZ > 64MB defined"
#else
#define SMALLCHUNK BUFSIZ
#endif

The io module uses a default buffer size of 8 KB to read.

http://bugs.python.org/review/18923/diff/9757/Lib/test/test_subprocess.py
File Lib/test/test_subprocess.py (left):

http://bugs.python.org/review/18923/diff/9757/Lib/test/test_subprocess.py#old...
Lib/test/test_subprocess.py:2191: class ProcessTestCaseNoPoll(ProcessTestCase):
Oh oh, removing a test is not a good idea. You should test select and poll
selectors when poll is used by default.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+