search

awslabs / soci-snapshotter

A containerd snapshotter plugin which enables standard OCI images to be lazily loaded without requiring a build-time conversion step.
Apache License 2.0
517 stars 54 forks source link

[Bug] sh.R leaves open pipes #1187

Open sondavidb opened 5 months ago

sondavidb commented 5 months ago

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.

https://github.com/awslabs/soci-snapshotter/blob/8a3b3617a0dff68c7728b99369e458e360d48c53/util/dockershell/shell.go#L346-L367

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-- commented 4 months ago

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 commented 4 months ago

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.