Repository navigation
Spawn through an os.posix_spawn() helper instead of fork() - #89
Open
krassowski wants to merge 5 commits into
Open
krassowski wants to merge 5 commits into
krassowski wants to merge 5 commits into
Conversation
3 tasks done
os.posix_spawn() helper instead of fork()
Author
|
The tests all pass on krassowski#1. @takluyver @Red-M would you be open to considering this PR, or should downstream ( |
Member
|
Personally, I'm not really able to review this properly due to its size and use of LLMs, sorry. |
Author
|
Sure, thank you for the reply! |
Member
|
I'm interested in the concept, but it's not going to be a priority for me any time soon, and I also dislike wading through LLM-generated code. So I think go ahead downstream. A couple of thoughts from a quick glance:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Python 3.12 and later give a
DeprecationWarningwhenPtyProcess.spawn()forks a process with threads (pexpect/pexpect#817, pexpect/pexpect#827, #82), and the forked child can hang beforeexec(pexpect/pexpect#369). This PR starts a small Python helper withos.posix_spawn()in place of the fork, and the helper makes the pty its controlling terminal beforeexec.References
posix_spawn()cannot set a controlling terminal, so the helper does it, as GNOME/vte#219 proposes.The first commit adds two tests that fail on master:
Details
Code changes
_posix_spawn_helper.py, a script that callssetsid(), sets the controlling terminal withTIOCSCTTY, changes the directory, closes inherited file descriptors and callsexec._posix_spawn.py: it starts the script aspython -I -S -X utf8 -c <script>withos.posix_spawn(), and raises theOSErrorof a failedchdir()orexecin the caller.PtyProcess.spawn()uses the helper. It still forks with apreexec_fn, withPTYPROCESS_USE_FORK=1, on Solaris and PyPy, in frozen applications, and when a test start of the helper fails.restore_signals=Falsetospawn(), to reset SIGPIPE, SIGXFZ and SIGXFSZ in the child assubprocess.Popendoes.User-facing changes
Before, the forked child ran the finalizers of thread-local objects of other threads, and the at-fork handlers of libraries. In a process with threads, for example an IPython kernel that runs a
!command, this code could hangspawn()or change files that the parent still uses. After, no code of the caller runs in the child, and Python does not warn. pexpect passes apreexec_fnonly when its caller gives one or setsignore_sighup=True, so pexpect uses the helper by default.A spawn now takes about 14 ms at any process size, and the first spawn in a process also runs a test start of the helper:
Backwards-incompatible changes
None
AI usage
I used Opus 5.5 Max to assist with code writing and self-review.
CI
This PR also includes #88 which drops EOL Python versions and adds new ones. I'm happy to rebase once that gets merged.
I ran the CI on my fork (krassowski#1) and all checks are green. It is also all green on jupyter/terminado#250 (which uses a slightly different matrix).