classification
Title: Lib/pty.py major revision
Type: Stage: patch review
Components: FreeBSD, Library (Lib), macOS, Tests, Windows Versions: Python 3.10, Python 3.9, Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: aeros, asvetlov, koobs, kulikjak, ned.deily, paul.moore, petr.viktorin, ronaldoussoren, soumendra, steve.dower, tim.golden, zach.ware
Priority: normal Keywords: patch

Created on 2020-09-19 23:19 by soumendra, last changed 2021-01-19 13:03 by petr.viktorin.

Pull Requests
URL Status Linked Edit
PR 22962 merged soumendra, 2020-10-25 06:47
PR 23514 merged asvetlov, 2020-11-25 15:06
PR 23526 merged soumendra, 2020-11-26 23:44
PR 23533 closed soumendra, 2020-11-27 21:45
PR 23536 merged soumendra, 2020-11-28 06:34
PR 23546 open soumendra, 2020-11-29 00:14
PR 23686 open soumendra, 2020-12-08 01:05
PR 23740 open soumendra, 2020-12-11 04:55
PR 24119 merged petr.viktorin, 2021-01-05 15:32
Messages (20)
msg377195 - (view) Author: Soumendra Ganguly (soumendra) * Date: 2020-09-19 23:19
The current pty library has the following issues:

  1. Does not set slave termios. Documented in the source.

  2. Does not set initial slave window size. Documented in the source. Does not handle SIGWINCH. See bpo-41494, bpo-41541. This is essential in the following practical scenarios: i. creating split windows/panes while using a terminal multiplexer; ii. when resizing GUI terminal emulator window, especially relevant when using tiling window managers; iii. resizing an ansi-term window created inside a GNU Emacs frame.

  3. Does not perform signal handling. Signals must be blocked during sensitive portions of code.

  4. Hangs on FreeBSD. See bpo-26228.

  5. Includes deprecated functions pty.master_open(), pty.slave_open().

  6. In pty.fork(), the fallback code should try using TIOCSCTTY first. It is still using the old method of opening a tty to make it the controlling tty. Currently even SysV based systems provide TIOCSCTTY. See https://stackoverflow.com/questions/51593530/code-explanation-for-glibc-login-tty-function-openttyname-immediately-f

The current version of pty.spawn() uses pty.fork() internally. However, pty.fork() closes slave and only returns (pid, master_fd). To update winsize, access to slave is necessary. Further, slave termios must be properly set. The proposed modifications do this by implementing a login_tty(3) based function ( tty.login() ), and using that in pty.spawn() instead of pty.fork(). tty.login() tries TIOCSCTTY before falling back to the old SysV method because Python currently does not provide an interface to the native login_tty(3).

  7. tty.setraw() is called right after tty.tcgetattr(). This increases redundancy of code because tty.setraw() itself makes an identical tty.tcgetattr() call.

  8. Requires testing/porting to more platforms. Solaris, Illumos, macOS, Cygwin, etc. Windows ConPTY?

  9. There should be an option in pty.spawn() to turn off slave's ECHO flag. For example, when it is being used in a pipe. See https://github.com/karelzak/util-linux/commit/1eee1acb245a8b724e441778dfa9b858465bf7e5 and https://github.com/karelzak/util-linux/commit/75ccd75a2fa1194c6415c47b0024a438e26f1ad7#diff-3834a3d25eeaf20d9d0dcb05a46995f6

  10. Tests are incomplete. Tests consider OSes such as Tru64 but not {Free/Net/Open/...}BSD.

Find ongoing work here: https://github.com/8vasu/pypty2
msg377198 - (view) Author: Kyle Stanley (aeros) * (Python committer) Date: 2020-09-20 00:51
In addition to the above, if a major revision is made to pty, I'd suggest also addressing the issue of "master/slave" terminology, and replace it with something comparable like "parent/child". There's an open devguide issue (https://github.com/python/devguide/issues/605) to more explicitly state terms to avoid, and support for avoiding usage of "slave/master" seems uncontroversial (especially in any new code).
msg377202 - (view) Author: Soumendra Ganguly (soumendra) * Date: 2020-09-20 02:51
Makes sense. I will happily make a change of terminology in the pypty2 repository after the most desirable alternative is determined based on the choice of the majority. I think 'mother/son' sounds cute while still retaining the same initials as before; people used to the older
terminology will find this easy to remember. Terminology such as parent/child and server/client might make it a little confusing.
msg381830 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2020-11-25 13:41
New changeset c13d89955d9a2942c6355d6839d7096323244136 by Soumendra Ganguly in branch 'master':
bpo-41818: Updated tests for the standard pty library (GH-22962)
https://github.com/python/cpython/commit/c13d89955d9a2942c6355d6839d7096323244136
msg381839 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-11-25 15:29
This change broke x86 Gentoo buildbots: bpo-42463.
msg381843 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2020-11-25 16:05
https://github.com/python/cpython/pull/23514 has the fix, waiting for all buildbots finish before pressing "Merge" button.
Gentoo bots are green.
msg381845 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-11-25 16:24
> https://github.com/python/cpython/pull/23514 has the fix, waiting for all buildbots finish before pressing "Merge" button.

Thanks for working on a fix :-)
msg381847 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-11-25 16:32
> In addition to the above, if a major revision is made to pty, I'd suggest also addressing the issue of "master/slave" terminology

In bpo-34605, I chose to leave the pty module unchange since it *seems* like "master_fd" and "slave_fd" is part of the API. See my rejected PR 9100.

More recently, I discussed with a glibc maintainer who is open to change the openpty() manual page to avoid "master" and "slave" terms.

In fact, these terms are not part of the API. They are just variable names in a manual page.

"parent" and "child" terms would work here. I'm not sure of the exact relationship between the two file descriptors.
msg381852 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2020-11-25 17:06
New changeset 87f7ab5359bc12eeb858272b7bd58e132cb9c176 by Andrew Svetlov in branch 'master':
bpo-41818: test_openpty succeed on Gentoo, don't expect to fail on this platform (GH-23514)
https://github.com/python/cpython/commit/87f7ab5359bc12eeb858272b7bd58e132cb9c176
msg381870 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2020-11-25 23:45
Moving my notes from PR23514 to here, the issue that that PR addressed is not Gentoo-specific; here's a simple reproducer on Ubuntu:

./python -c "import pty;pty.spawn('./python -m test -v test_pty'.split())"
msg381922 - (view) Author: Soumendra Ganguly (soumendra) * Date: 2020-11-26 23:49
The reproducer was helpful. https://github.com/python/cpython/pull/23526 should fix this issue.
msg381930 - (view) Author: Jakub Kulik (kulikjak) * Date: 2020-11-27 08:36
This change also broke Solaris (SunOS), where (similarly to BSDs and Darwin) OSError is not raised in the `new test_master_read()` test.

Adding `or PLATFORM == "SunOS"` into the `expectedFailureOnBSD` function fixes this issue, but it's no longer just BSDs and it's derivates.
msg381941 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2020-11-27 10:16
New changeset f5a19ead4ba8c81cc27d5a530f830f4709ce240e by Soumendra Ganguly in branch 'master':
bpo-41818: Make test_openpty() avoid unexpected success due to number of rows and/or number of columns being == 0. (GH-23526)
https://github.com/python/cpython/commit/f5a19ead4ba8c81cc27d5a530f830f4709ce240e
msg381965 - (view) Author: Soumendra Ganguly (soumendra) * Date: 2020-11-27 20:50
This is actually good news. I had not tested the code on Solaris; this confirms my suspicion that Linux is the only platform that "behaves differently". Sadly, the current Lib/pty.py code depends on such "different behavior" to exit from pty.spawn()'s copy loop, which is why it hangs on the BSDs, macOS, and now we know that it (probably) also hangs on Solaris. Adding 'or PLATFORM == "SunOS"' is the correct thing to do. I am working on this now.
msg381968 - (view) Author: Soumendra Ganguly (soumendra) * Date: 2020-11-27 22:01
PR-23533 should fix the test_master_read() issue on Solaris. Actually, instead of adding 'or PLATFORM == "SunOS"', I ended up removing the whole line and replaced it with 'if platform.system() != "Linux"' because I expect that test to fail on every platform that is not Linux.
msg381980 - (view) Author: Soumendra Ganguly (soumendra) * Date: 2020-11-28 06:45
Update: I closed PR 23533. PR 23536 is much better. It will help us detect exact behavior on each platform, which is necessary for making pty.spawn() successfully exit its copy loop.
msg382016 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2020-11-28 21:04
New changeset 74311aeb45b52cc145d27d9fca99f01874d6882d by Soumendra Ganguly in branch 'master':
bpo-41818: Fix test_master_read() so that it succeeds on all platforms that either raise OSError or return b"" upon reading from master (GH-23536)
https://github.com/python/cpython/commit/74311aeb45b52cc145d27d9fca99f01874d6882d
msg384408 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2021-01-05 15:34
I noticed an issue in one of the newly added tests; see GH-24119
msg384413 - (view) Author: Soumendra Ganguly (soumendra) * Date: 2021-01-05 16:28
Thank you for the fix. That test was created by modifying an existing test which already had that issue; it is documented in a comment by user nnorwitz in the file. If your solution resolves the problem for all the tests in "class PtyTest", then can you please also remove that comment?
msg385257 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2021-01-19 13:03
New changeset 65cf1ad6723b6b4489fa7dda04283bb2466be531 by Petr Viktorin in branch 'master':
bpo-41818: Close file descriptors in test_openpty (#GH-24119)
https://github.com/python/cpython/commit/65cf1ad6723b6b4489fa7dda04283bb2466be531
History
Date User Action Args
2021-01-19 13:03:20petr.viktorinsetmessages: + msg385257
2021-01-05 16:28:34soumendrasetmessages: + msg384413
2021-01-05 15:34:38petr.viktorinsetmessages: + msg384408
2021-01-05 15:32:37petr.viktorinsetnosy: + petr.viktorin
pull_requests: + pull_request22949
2020-12-11 04:55:10soumendrasetpull_requests: + pull_request22598
2020-12-08 01:05:24soumendrasetpull_requests: + pull_request22550
2020-11-29 00:14:22soumendrasetpull_requests: + pull_request22425
2020-11-28 21:04:28asvetlovsetmessages: + msg382016
2020-11-28 06:45:10soumendrasetmessages: + msg381980
2020-11-28 06:34:19soumendrasetpull_requests: + pull_request22418
2020-11-27 22:01:03soumendrasetmessages: + msg381968
2020-11-27 21:45:38soumendrasetpull_requests: + pull_request22415
2020-11-27 20:50:47soumendrasetmessages: + msg381965
2020-11-27 10:16:45asvetlovsetmessages: + msg381941
2020-11-27 08:36:20kulikjaksetnosy: + kulikjak
messages: + msg381930
2020-11-26 23:49:47soumendrasetmessages: + msg381922
2020-11-26 23:44:35soumendrasetpull_requests: + pull_request22408
2020-11-26 14:26:40vstinnersetnosy: - vstinner
2020-11-25 23:45:29zach.waresetmessages: + msg381870
2020-11-25 17:06:20asvetlovsetmessages: + msg381852
2020-11-25 16:32:26vstinnersetmessages: + msg381847
2020-11-25 16:24:32vstinnersetmessages: + msg381845
2020-11-25 16:05:33asvetlovsetmessages: + msg381843
2020-11-25 15:29:46vstinnersetnosy: + vstinner
messages: + msg381839
2020-11-25 15:06:34asvetlovsetpull_requests: + pull_request22401
2020-11-25 13:41:29asvetlovsetnosy: + asvetlov
messages: + msg381830
2020-10-25 06:47:22soumendrasetkeywords: + patch
stage: patch review
pull_requests: + pull_request21878
2020-09-20 02:51:38soumendrasetmessages: + msg377202
2020-09-20 00:51:32aerossetnosy: + aeros
messages: + msg377198
2020-09-19 23:19:34soumendracreate