Various assorted improvements to spawnveg#881
Closed
Conversation
Alterations:
- Bugfix: Don't set the terminal signals to their defaults
in the parent prior to calling spawnveg. This is the primary
cause of a lockup in the pty tests which can be reproduced
as follows:
$ exec ksh
$ bin/shtests pty 2>/dev/null
^C ^C ^C (SIGINT doesn't work anymore and may segfault)
The correct way to go about dealing with SIGT* is to set
those to SIG_DFL in the child process; see _sh_fork()
for a working example that doesn't lock up or produce segfaults.
In this commit I duplicate _sh_fork()'s behavior in spawnveg
for the fork fallback and with POSIX_SPAWN_SETSIGDEF for the
posix_spawn version.
- Added support for tcsetpgrp to the fork fallback in spawnveg.
Some form of this appears to have already been attempted in
AT&T olden times, but that old version was broken and needed
bugfixes desperately.
- If the child needs tcsetpgrp, block the terminal signals
in the parent process via sigcritical(). The posix_spawn
version doesn't need this because posix_spawn will block
signals automatically and therefore doesn't need sigcritical.
- Now that the fork fallback for spawnveg works correctly in
interactive terminals, prefer that to the sh_fork() codepath
on operating systems without posix_spawn tcsetpgrp support.
Even though the underlying system call is still ultimately fork,
the sh_ntfork() codepath is faster than the traditional sh_fork
codepath. Benchmark:
$ time /tmp/ksh-devbranch -ic "for i in {1..10000}; do $(whence -p true); done"
real 0m03.302s
user 0m00.988s
sys 0m02.320s
$ time /tmp/ksh-newspawnveg -ic "for i in {1..10000}; do $(whence -p true); done"
real 0m03.160s
user 0m01.187s
sys 0m01.977s
- To that end, split up the spawnveg versions into spawnveg_fast
and spawnveg_slow. Choose the appropriate one when spawnveg is
called; this removes the need for the xec.c ifdef hackery.
- Removed the ntfork_tcpgrp ifdefs from xec.c; spawnveg can
handle it by itself now.
- Bugfix: With the spawnveg_fast and spawnveg_slow innovation,
spawnveg now always has support for setsid. It'll fallback to
fork if POSIX_SPAWN_SETSID isn't available.
- Bugfix: For the posix_spawn version of spawnveg, the flags should
be of the short type pursuant to the POSIX specification.
- Optimization: Use pipe2 in the fork fallback for spawnveg when
it's available to avoid two fcntl syscalls.
- Updated the spawnveg documentation to reflect the new changes.
Author
|
This pull request has been superseded by #888. |
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.
Alterations:
spawnveg. This is the primary cause of a lockup in the pty tests occurring on Linux distributions using glibc 2.35+. The correct way to go about dealing withSIGT*is to set those toSIG_DFLin the child process; see_sh_fork()for a working example that doesn't lock up or produce segfaults. In this commit I duplicate_sh_fork's behavior inspawnvegfor theforkfallback and withPOSIX_SPAWN_SETSIGDEFfor theposix_spawnversion. The lockup can be reproduced as follows:tcsetpgrpto theforkfallback inspawnveg. Some form of this appears to have already been attempted in AT&T olden times, but that old version was broken and needed bugfixes desperately.tcsetpgrp, block the terminal signals in the parent process viasigcritical(). Theposix_spawnversion doesn't need this becauseposix_spawnwill usually block signals automatically and therefore doesn't needsigcritical.forkfallback forspawnvegworks correctly in interactive terminals, prefer that to thesh_fork()codepath on operating systems withoutposix_spawn_file_actions_addtcsetpgrp_np. Even though the underlying system call is still ultimatelyfork, thesh_ntfork()codepath is faster than the traditionalsh_fork()codepath. Benchmark (FreeBSD 14.2):spawnvegversions intospawnveg_fastandspawnveg_slow. Choose the appropriate one whenspawnvegis called; this removes the need for the xec.c ifdef hackery.ntfork_tcpgrpifdefs from xec.c;spawnvegcan handle it by itself now.spawnveg_fastandspawnveg_slowinnovation,spawnvegnow always has support forsetsid. It'll fallback to fork ifPOSIX_SPAWN_SETSIDisn't available.posix_spawnversion ofspawnveg, the flags should be of theshorttype pursuant to the POSIX specification.pipe2in theforkfallback forspawnvegwhen it's available to avoid twofcntlsyscalls.spawnvegdocumentation to reflect the new changes.