[Bug] sh.R leaves open pipes

[sondavidb]

Description

Our testing suite leaves dangling resources that can potentially leave open readers/writers.

In sh.R, we create an io.Pipe() that returns a reader and a writer for stdout and stderr. Then a goroutine is spun up using those writers.

soci-snapshotter/util/dockershell/shell.go

Lines 346 to 367 in 8a3b361

	func (s *Shell) R(args ...string) (stdout, stderr io.Reader, err error) {
	if s.IsInvalid() {
	return nil, nil, fmt.Errorf("invalid shell")
	}
	if len(args) < 1 {
	return nil, nil, fmt.Errorf("no command to run")
	}
	s.r.Logf(">>> Running(returning reader): %v\n", args)
	cmd := s.Command(args[0], args[1:]...)
	outR, outW := io.Pipe()
	errR, errW := io.Pipe()
	cmd.Stdout, cmd.Stderr = outW, errW
	go func() {
	if err := cmd.Run(); err != nil {
	outW.CloseWithError(err)
	errW.CloseWithError(err)
	return
	}
	outW.Close()
	errW.Close()
	}()
	return outR, errR, nil

If they get stuck on a cmd.Run() due to expecting output from the writer, it will hang until it receives an EOF, but as there's no writer provided as an output, it becomes a lot more cumbersome to ensure the goroutine is not left hanging.

Ideally the shell should handle this and send it an EOF on cleanup, but the stdout and stderr for the initial shell and the stdout and stderr returned from sh.R are different, it does not clean this up properly.

My initial thought was to return the stdout and stderr used in the reporter instead of making new ones, but I imagine that in the case of multiple commands being run, one would not want to reuse the same stdout and stderr for each command.

We probably just need an easier way to return the writers that are used.

Steps to reproduce the bug

No response

Describe the results you expected

sh.R should make it much easier to not orphan goroutines.

Host information

1. OS: AL2
2. Snapshotter Version: main
3. Containerd Version: v1.7.11

Any additional context or information about the bug

This might be a feature request versus a bug, but as we probably just expect the shell to handle this on shutdown, the fact that it does not is surprising behavior.

[Kern--]

Is the concern that if cmd.Run never exits, the write end of the pipes will never be closed? Or is the concern that if we don't drain the read end of the pipe that cmd.Run can't exit?

[sondavidb]

Both. It gets stuck on cmd.Run() which means the pipes never exit, and it's stuck on cmd.Run() because we never get input or close the pipes.

The simple way to fix this would probably be to just close stdout and stderr when cleaning up tests.