Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.10] bpo-26228: Fix pty EOF handling (GH-12049) #27732

Merged
merged 1 commit into from
Aug 12, 2021

Conversation

miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Aug 11, 2021

On non-Linux POSIX platforms, like FreeBSD or macOS,
the FD used to read a forked PTY may signal its exit not
by raising an error but by sending empty data to the read
syscall. This case wasn't handled, leading to hanging
pty.spawn calls.

Co-authored-by: Reilly Tucker Siemens reilly@tuckersiemens.com
Co-authored-by: Łukasz Langa lukasz@langa.pl
(cherry picked from commit 81ab8db)

Co-authored-by: Zephyr Shannon geoffpshannon@gmail.com

https://bugs.python.org/issue26228

On non-Linux POSIX platforms, like FreeBSD or macOS,
the FD used to read a forked PTY may signal its exit not
by raising an error but by sending empty data to the read
syscall. This case wasn't handled, leading to hanging
`pty.spawn` calls.

Co-authored-by: Reilly Tucker Siemens <reilly@tuckersiemens.com>
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
(cherry picked from commit 81ab8db)

Co-authored-by: Zephyr Shannon <geoffpshannon@gmail.com>
@miss-islington
Copy link
Contributor Author

@RadicalZephyr and @ambv: Status check is done, and it's a success ✅ .

@miss-islington
Copy link
Contributor Author

@RadicalZephyr and @ambv: Status check is done, and it's a success ✅ .

@ambv ambv added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 11, 2021
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @ambv for commit 1f7fb15 🤖

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 11, 2021
@miss-islington
Copy link
Contributor Author

@RadicalZephyr and @ambv: Status check is done, and it's a pending ❌ .

15 similar comments
@miss-islington
Copy link
Contributor Author

@RadicalZephyr and @ambv: Status check is done, and it's a pending ❌ .

@miss-islington
Copy link
Contributor Author

@RadicalZephyr and @ambv: Status check is done, and it's a pending ❌ .

@miss-islington
Copy link
Contributor Author

@RadicalZephyr and @ambv: Status check is done, and it's a pending ❌ .

@miss-islington
Copy link
Contributor Author

@RadicalZephyr and @ambv: Status check is done, and it's a pending ❌ .

@miss-islington
Copy link
Contributor Author

@RadicalZephyr and @ambv: Status check is done, and it's a pending ❌ .

@miss-islington
Copy link
Contributor Author

@RadicalZephyr and @ambv: Status check is done, and it's a pending ❌ .

@miss-islington
Copy link
Contributor Author

@RadicalZephyr and @ambv: Status check is done, and it's a pending ❌ .

@miss-islington
Copy link
Contributor Author

@RadicalZephyr and @ambv: Status check is done, and it's a pending ❌ .

@miss-islington
Copy link
Contributor Author

@RadicalZephyr and @ambv: Status check is done, and it's a pending ❌ .

@miss-islington
Copy link
Contributor Author

@RadicalZephyr and @ambv: Status check is done, and it's a pending ❌ .

@miss-islington
Copy link
Contributor Author

@RadicalZephyr and @ambv: Status check is done, and it's a pending ❌ .

@miss-islington
Copy link
Contributor Author

@RadicalZephyr and @ambv: Status check is done, and it's a pending ❌ .

@miss-islington
Copy link
Contributor Author

@RadicalZephyr and @ambv: Status check is done, and it's a pending ❌ .

@miss-islington
Copy link
Contributor Author

@RadicalZephyr and @ambv: Status check is done, and it's a pending ❌ .

@miss-islington
Copy link
Contributor Author

@RadicalZephyr and @ambv: Status check is done, and it's a pending ❌ .

@ambv
Copy link
Contributor

ambv commented Aug 12, 2021

@pablogsal, this is a bugfix we should have done a long time ago. If you feel messing with PTY at RC stage is too risky, we can merge this post-3.10.0. Both here, and on the original PR, I ran with test-with-buildbots and confirmed all's good manually on operating systems I have access to (Win10, Ubuntu, and macOS 10.15).

This backport has an outstanding buildbot/AMD64 FreeBSD Non-Debug PR job where the worker got disconnected. It would have been a valuable buildbot job, given BSD and non-pydebug, but it seems the worker's got trouble relatively often: https://buildbot.python.org/all/#/builders/387

In any case, we have to have this PR in at some point as it's one of the changes that will allow running -R: -uall on macOS.

@ambv ambv requested a review from pablogsal August 12, 2021 08:03
@@ -0,0 +1 @@
pty.spawn no longer hangs on FreeBSD, OS X, and Solaris.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should say "macOS" but I have a follow-up PR for doc updates anyway so I'll fix that there.

Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the change and I think is safe enough. Also we have another RC so we can still test it to some degree

@ambv ambv merged commit 5d44443 into python:3.10 Aug 12, 2021
@miss-islington miss-islington deleted the backport-81ab8db-3.10 branch August 12, 2021 12:36
@miss-islington
Copy link
Contributor Author

@RadicalZephyr and @ambv: Status check is done, and it's a pending ❌ .

@miss-islington
Copy link
Contributor Author

@RadicalZephyr and @ambv: Status check is done, and it's a success ✅ .

@miss-islington
Copy link
Contributor Author

@RadicalZephyr and @ambv: Status check is done, and it's a pending ❌ .

@miss-islington
Copy link
Contributor Author

@RadicalZephyr and @ambv: Status check is done, and it's a success ✅ .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants