gh-96522: Fix deadlock in pty.spawn #96639
Conversation
zhangyoufu
commented
Sep 7, 2022
•
zhangyoufu
commented
- Issue: pty.spawn deadlock #96522
|
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
|
All commit authors signed the Contributor License Agreement. |
| while data: | ||
| n = os.write(fd, data) | ||
| data = data[n:] | ||
|
|
||
| def _read(fd): | ||
| """Default read function.""" | ||
| return os.read(fd, 1024) |
There was a problem hiding this comment.
Would it be okay to replace high_waterlevel and this 1024 above here with a PIPE_BUF global set to 4096 so it's clear where it comes from? This also makes the deadlock in line 164 (master_read) more apparent.
There was a problem hiding this comment.
_copy accept caller provided master_read (may be something other than pty._read). It does not support "read at most n bytes" unfortunately. The actual buffer size may exceed 4096 here. high_waterlevel serves as a threshold for whether we should continue reading into our buffer, it can not limit maximum buffer size. A name like PIPE_BUF is not appropriate per my understanding.
I do want to increase the read size from 1024 to 4096, but I'm afraid of unexpected changes to application behavior. As long as we didn't pass lots of data via pty, 1024 should serve us well. So I didn't change it.
Lib/pty.py
Outdated
| fds = [master_fd, STDIN_FILENO] | ||
| while fds: | ||
| rfds, _wfds, _xfds = select(fds, [], []) | ||
| os.set_blocking(master_fd, False) |
There was a problem hiding this comment.
Would you mind commenting why this is necessary?
And also, even though _copy is a private function here, it is for sure used by end users in their programs (per Hyrum's Law) so it would be good to unset non-blocking when exiting _copy so that backward's compatibility is preserved.
There was a problem hiding this comment.
Changed as requested. PTAL
|
@ambv PTAL, thanks. |
|
Thanks @zhangyoufu for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11. |
|
Sorry @zhangyoufu and @ambv, I had trouble checking out the |
ambv
added
needs backport to 3.11
|
Thanks @zhangyoufu for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11. |
|
Sorry, @zhangyoufu and @ambv, I could not cleanly backport this to |
(cherry picked from commit 9c5aa89) Co-authored-by: Youfu Zhang <1315097+zhangyoufu@users.noreply.github.com>
|
GH-104655 is a backport of this pull request to the 3.11 branch. |
* main: (30 commits) pythongh-103987: fix several crashes in mmap module (python#103990) docs: fix wrong indentation causing rendering error in dis page (python#104661) pythongh-94906: Support multiple steps in math.nextafter (python#103881) pythongh-104472: Skip `test_subprocess.ProcessTestCase.test_empty_env` if ASAN is enabled (python#104667) pythongh-103839: Allow building Tkinter against Tcl 8.7 without external libtommath (pythonGH-103842) pythongh-85984: New additions and improvements to the tty library. (python#101832) pythongh-104659: Consolidate python examples in enum documentation (python#104665) pythongh-92248: Deprecate `type`, `choices`, `metavar` parameters of `argparse.BooleanOptionalAction` (python#103678) pythongh-104645: fix error handling in marshal tests (python#104646) pythongh-104600: Make type.__type_params__ writable (python#104634) pythongh-104602: Add additional test for listcomp with lambda (python#104639) pythongh-104640: Disallow walrus in comprehension within type scopes (python#104641) pythongh-103921: Rename "type" header in argparse docs (python#104654) Improve readability of `typing._ProtocolMeta.__instancecheck__` (python#104649) pythongh-96522: Fix deadlock in pty.spawn (python#96639) pythonGH-102818: Do not call `PyTraceBack_Here` in sys.settrace trampoline. (pythonGH-104579) pythonGH-103545: Add macOS specific constants for ``os.setpriority`` to ``os`` (python#104606) pythongh-104623: Update macOS installer to SQLite 3.42.0 (pythonGH-104624) pythongh-104619: never leak comprehension locals to outer locals() (python#104637) pythongh-104602: ensure all cellvars are known up front (python#104603) ...
