Skip to content

$&here leaks child processes #150

@jpco

Description

@jpco

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	} } 

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions