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

$&here leaks child processes #150

Open
jpco opened this issue Dec 2, 2024 · 1 comment · May be fixed by #160
Open

$&here leaks child processes #150

jpco opened this issue Dec 2, 2024 · 1 comment · May be fixed by #160
Labels

Comments

@jpco
Copy link
Collaborator

jpco commented Dec 2, 2024

This was probably not much of a problem previously because wait3() would clean up errant processes like this, but after migrating to waitpid() in #128 it's more obvious. See the following:

; %read << EOF
hello
EOF
; echo <=wait  # surprising
0
; echo <=wait  # not surprising
wait: No child process

My expectation would be to get a No child process exception on the first wait.

There's nothing obvious in the blame layer to suggest this is intentional. However, I think it's interesting that $&here is written like the other "normal" redirections (using a REDIR(here)) but it also performs a pipefork() like the (not-REDIR()-using) primitives $&pipe, $&readfrom/$&writeto, and $&backquote, all of which do wait for their child processes. Maybe the authors back in the day hemmed and hawed about implementing $&here via a fork, since it's not strictly necessary?

The simple/narrow fix is to, in PRIM(here) save the return value from redir(), ewait() for the writing process to finish, and then return it (plus the appropriate exception-handling-wrapping).

However, if we wanted, we could also get rid of the fork sometimes or always. Other shells seem to use a few different strategies for heredocs:

  • bash directly tests pipe buffer sizes at build time, and uses a pipe for heredocs smaller than the tested buffer size, and a temporary file for larger ones.
  • zsh (based a quick strace) seems to always use a temporary file for heredocs.
  • dash uses a simple-to-guess minimum pipe size (PIPE_BUF), uses a pipe for smaller heredocs, and forks for larger ones.
  • rakitzis rc, still, always forks off a pipe writer.

I'm somewhat in favor of following dash's example here. It seems relatively simple while still making the common short-heredoc case (heredoc length < the system's PIPE_BUF, minimum 512 per POSIX but usually 4096) fast. I don't actually have a strong intuition on whether a fork or creating/deleting a temporary file is faster on most systems, so I don't have a strong urge to change the "longer" case. Plus, if users really want a tmpfile-based %here, they can define their own version something like this:

fn %here fd input cmd {
	let (f = /tmp/es.here.$pid)
	unwind-protect {
		echo -n $input > $f
		%open $fd $f $cmd
	} {
		rm -f $f
	}
}
@jpco
Copy link
Collaborator Author

jpco commented Dec 2, 2024

Alternatively, if we don't mind keeping the unconditional fork, we could get rid of $&here and redefine %here pretty simply in terms of %readfrom :)

fn %here fd input cmd {
	%readfrom _here^$fd {echo -n $input} {%open $fd $(_here^$fd) $cmd}
}

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

Successfully merging a pull request may close this issue.

1 participant