bpo-26228: Fix pty EOF handling

[RadicalZephyr]

I took a shot at fixing this in a smaller more targeted patch. I don't have a BSD system to test this on, but I think this should still solve the major issue of pty.spawn hanging on platforms that don't raise an error. In addition, pty.spawn should now ALWAYS return the terminal to the previous settings.

Co-authored with @reillysiemens.

https://bugs.python.org/issue26228

[RadicalZephyr]

@eamanu I would love to address your comments and move this closer to being merged, but I don't really understand what you were getting at. If you could provide some clarifications to the questions I asked that would be really helpful. Thanks!

[reillysiemens]

@RadicalZephyr: Looks like there are some conflicts with this, would you be able to rebase or otherwise resolve those?

@csabella, @gpshead, @vadmium, @eamanu: Do any of y'all see further blockers to this being merged? 🤔

[RadicalZephyr]

@reillysiemens rebased against master 👍

[8vasu]

Hi.

From 3.10 docs:

In order to force spawn to return before the child process exits an OSError should be thrown

This is a result of ALL OSErrors in _copy() being caught in spawn(). However, this patch removes that except clause. I actually like this change :D It is already a part of some code that I have been writing. This way, unwanted OSErrors will not be ignored. However, docs need to be updated. Also, this needs to be approved by reviewers.

[RadicalZephyr]

Good catch @8vasu. I've updated the docs a bit to not advertise that throwing OSError from the master_read function will still work to cause spawn to return, and instead suggest returning an empty byte array.

Perhaps that whole clause should just be removed instead, and make the usage of spawn returning before the child process ends not officially supported? I think that particular functionality is a very niche use-case that would be abusing pty.spawn to fork a daemon process but first to do some I/O with it via a pty. I think that use-case is advanced enough that it's outside of the scope of what pty.spawn should be used for.

[8vasu]

Agreed. In fact, if the child process does not change state, even if you successfully get out of _copy(), you'll still be stuck at the final waitpid() in spawn() [ this is off the top of my head; I have not tested this claim, so please correct me if I am wrong. ]

Anyway, there are several other problems with cpython's pty. A lot of its fallback code is outdated; I contacted the glibc people about this. I am currently using a tiling window manager; every time I resize xterm's X window, the underlying terminal size also gets modified, and the output of commands are laid out incorrectly, making them very hard to read [ no SIGWINCH handling ].

There are much better pty libraries for python on PyPI, but when I want to perform a quick test, I should not have to download and learn how to use third party libraries. I have fixed most of these issues in cpython's pty while keeping it backward compatible: I decided to keep the "except OSError" in spawn() for now. I tested my version on Linux, FreeBSD, OpenBSD, and NetBSD. I have not tested it on Solaris, Illumos, macOS, Cygwin, etc. However, I am afraid that cpython will probably take a decade to merge such changes, given that its primary objective is not systems programming :D After a little more testing, I will make the file available in a separate repository; it will serve as an independent drop-in replacement for cpython's pty.

[RadicalZephyr]

I think one of the big concerns with making changes to the existing pty module is breaking backwards compatibility. Perhaps what really needs to happen is to create a new pty2 module, several other modules in the Python standard library that needed substantial reworks. Perhaps your replacement library could be submitted as such! Then pty could be deprecated and pty2 recommended for new code.

[8vasu]

Fantastic idea. I will admit that I have never done this before. If I was to submit a pull request for a new library, I think I would also be responsible for creating docs, tests, etc. Also, if the new library does not modify certain parts of pty, then I believe that I should import pty in the new library to re-use that code. What is your opinion?

Anyway, I might still make the new library available as a separate download.

[RadicalZephyr]

I'm not sure about reusing code from the pty module. I'm not very experienced in engaging with the cpython contribution process. I think maybe the best place to start with getting pty2 going is to chat with the devs on the #python-dev IRC channel on freenode. https://www.python.org/community/irc/

[8vasu]

Thank you for the suggestion @RadicalZephyr I will try that first.

[8vasu]

Here you go: https://github.com/8vasu/pypty2 :)

I welcome suggestions and criticisms.

[8vasu]

I realized that @reillysiemens is a co-author of this. Since https://github.com/8vasu/pypty2 includes a solution of this issue as well, I should include his name in the Acknowledgements file. Does anyone have any objection?

[8vasu]

Done.

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @ambv for commit 9f62b82 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

[miss-islington]

Thanks @RadicalZephyr for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

[miss-islington]

Thanks @RadicalZephyr for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

[miss-islington]

Sorry, @RadicalZephyr and @ambv, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 81ab8db235580317edcb0e559cd4c983f70883f5 3.9

[bedevere-bot]

GH-27732 is a backport of this pull request to the 3.10 branch.

[ambv]

3.9 differs too much for backporting, leaving this behind then.

[vstinner]

Thanks for the fix! I had headaches fighting with https://bugs.python.org/issue38547

See also my commit 7a51a7e for Solaris/AIX: https://bugs.python.org/issue40140