classification
Title: Patch for bugs in pty.py
Type: behavior Stage:
Components: Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: ajaksu2, fergushenderson, gpolo, gregory.p.smith, pitrou, psss, python-dev, r.david.murray
Priority: normal Keywords: patch

Created on 2008-03-26 01:15 by fergushenderson, last changed 2012-09-29 20:03 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
pty.py.patch fergushenderson, 2008-03-26 01:15 Patch to Lib/pty.py
pty.py.patch2 fergushenderson, 2009-06-01 22:53 Patch for (1)+(2): waitpid and return status
pty.py.patch3 r.david.murray, 2010-08-27 19:43 Patch for (3): _copy loop bug
Messages (12)
msg64533 - (view) Author: Fergus Henderson (fergushenderson) Date: 2008-03-26 01:15
The attached patch fixes some bugs in the pty.py module:

  - spawn() would not wait for the invoked process to finish.
    Also, it did not return a meaningful value, so there was no way
    to tell if the invoked process failed.    After this patch,
    spawn() now waits for the invoked process, and returns the value
    returned from os.waitpid().

  - There was a bug in the _copy() loop that caused it to spin
    using 100% CPU rather than blocking after EOF was reached on
    one of the two file descriptors that it was waiting for.
msg64534 - (view) Author: Guilherme Polo (gpolo) * (Python committer) Date: 2008-03-26 01:22
Hi Fergus,

I would suggest using "if not data" to check for EOF
msg64535 - (view) Author: Fergus Henderson (fergushenderson) Date: 2008-03-26 01:46
On Tue, Mar 25, 2008 at 9:22 PM, Guilherme Polo <report@bugs.python.org> wrote:
>
>  I would suggest using "if not data" to check for EOF

Good idea.
msg87914 - (view) Author: Daniel Diniz (ajaksu2) Date: 2009-05-16 19:38
Fergus,
Can you provide a test for the _copy loop bug? IIUC, the spawn change is
an RFE and shouldn't land on the maintenance branches (or 3.1).
msg88683 - (view) Author: Fergus Henderson (fergushenderson) Date: 2009-06-01 22:51
The spawn change (the last hunk of the original patch) is a bug fix, not
an RFE.
It has two parts that fix two bugs:
  (1) a coding bug: spawn() would not wait for the invoked process to
finish.  This is fixed by the line that adds the call to os.waitpid().
  (2) a design bug: because previously spawn() didn't return a value,
there is no way to tell if the invoked process failed.  This is fixed by
the "return status" line.

Now I guess you can argue that (2) is an RFE.
But (1) is just a bug fix, not an RFE, IMHO.

Those are both separate from the other bug fixed in the patch:
  (3) Another coding bug: the bug in the _copy() loop that caused
    it to spin using 100% CPU rather than blocking

It's a little tricky to write a test of the _copy() loop bug, for
several reasons.
(a) There currently isn't any test for pty.spawn, apparently since
"Cannot do extensive 'do or fail' testing because pty code is not too
portable."
(b) Also, for this bug the symptom is just that the code spins (using
100% CPU, if available) rather than blocking.  It's difficult to detect
that situation using portable code.

I can maybe figure out how deal with (a), but I'm not sure how to
address (b), especially since I don't know the intended portability goals.

I will split the patch up into two patches, one of which addresses
(1)+(2), and the other of which addresses (3).

I have addressed Guilherme Polo's suggestion about using "if not data".
msg115120 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-08-27 19:35
Woops, didn't mean to delete that file.  Reattaching.
msg115121 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-08-27 19:38
That didn't work so well :(
msg115123 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-08-27 19:48
OK, file restored.

Design bugs are usually fixed by "feature requests" :)

See issue 967171 for the feature request.
msg153466 - (view) Author: Roundup Robot (python-dev) Date: 2012-02-16 08:40
New changeset 994659efa292 by Gregory P. Smith in branch '3.2':
Issue #2489: Fix bug in _copy loop that could consume 100% cpu on EOF.
http://hg.python.org/cpython/rev/994659efa292

New changeset c7338f62f956 by Gregory P. Smith in branch 'default':
Issue #2489: Fix bug in _copy loop that could consume 100% cpu on EOF.
http://hg.python.org/cpython/rev/c7338f62f956

New changeset f889458c65cc by Gregory P. Smith in branch '2.7':
Issue #2489: Fix bug in _copy loop that could consume 100% cpu on EOF.
http://hg.python.org/cpython/rev/f889458c65cc
msg153467 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2012-02-16 08:43
I'm keeping this open to address the added behavior for spawn in 3.3.
msg171593 - (view) Author: Roundup Robot (python-dev) Date: 2012-09-29 19:41
New changeset ec2921d4de37 by Gregory P. Smith in branch 'default':
pty.spawn() now returns the child process status as returned by os.waitpid().
http://hg.python.org/cpython/rev/ec2921d4de37
msg171594 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-09-29 20:03
The test breaks on OpenIndiana (and possibly elsewhere):
http://buildbot.python.org/all/builders/x86%20OpenIndiana%203.x/builds/4640

======================================================================
FAIL: test_spawn_returns_status (test.test_pty.PtyTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/test/test_pty.py", line 200, in test_spawn_returns_status
    status = pty.spawn([sys.executable, '-c', 'import sys; sys.exit(0)'])
  File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/pty.py", line 175, in spawn
    _copy(master_fd, master_read, stdin_read)
  File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/pty.py", line 147, in _copy
    rfds, wfds, xfds = select(fds, [], [])
  File "/export/home/buildbot/32bits/3.x.cea-indiana-x86/build/Lib/test/test_pty.py", line 65, in handle_sig
    self.fail("isatty hung")
AssertionError: isatty hung
History
Date User Action Args
2012-09-29 20:03:48pitrousetnosy: + pitrou
messages: + msg171594
2012-09-29 19:41:30gregory.p.smithsetstatus: open -> closed
resolution: fixed
2012-09-29 19:41:10python-devsetmessages: + msg171593
2012-02-16 08:43:07gregory.p.smithsetversions: + Python 3.3, - Python 3.1, Python 2.7, Python 3.2
nosy: + gregory.p.smith

messages: + msg153467

assignee: gregory.p.smith
stage: test needed ->
2012-02-16 08:40:38python-devsetnosy: + python-dev
messages: + msg153466
2010-08-27 19:48:34r.david.murraysetmessages: + msg115123
versions: + Python 2.7, Python 3.2, - Python 2.6, Python 2.4
2010-08-27 19:43:37r.david.murraysetfiles: + pty.py.patch3
2010-08-27 19:38:15r.david.murraysetmessages: + msg115121
2010-08-27 19:37:56r.david.murraysetfiles: - pty.py.patch3
2010-08-27 19:35:17r.david.murraysetfiles: + pty.py.patch3
nosy: + r.david.murray
messages: + msg115120

2010-08-27 19:32:10r.david.murraysetfiles: - pty.py.patch3
2009-06-01 22:53:42fergushendersonsetfiles: + pty.py.patch2
2009-06-01 22:52:00fergushendersonsetfiles: + pty.py.patch3

messages: + msg88683
2009-06-01 20:12:16pssssetnosy: + psss
2009-05-16 19:38:42ajaksu2setpriority: normal
versions: + Python 3.1, - Python 2.5, Python 3.0
nosy: + ajaksu2

messages: + msg87914

stage: test needed
2008-03-26 01:46:31fergushendersonsetmessages: + msg64535
2008-03-26 01:22:32gpolosetnosy: + gpolo
messages: + msg64534
2008-03-26 01:15:48fergushendersoncreate