diff --git a/internal/container/run.go b/internal/container/run.go index eeeda7d2c..899fb054d 100644 --- a/internal/container/run.go +++ b/internal/container/run.go @@ -9,6 +9,8 @@ import ( dcontainer "github.com/moby/moby/api/types/container" dockerClient "github.com/moby/moby/client" "github.com/pkg/errors" + + "github.com/buildpacks/pack/pkg/logging" ) type Handler func(bodyChan <-chan dcontainer.WaitResponse, errChan <-chan error, reader io.Reader) error @@ -87,9 +89,17 @@ func DefaultHandler(out, errOut io.Writer) Handler { } } +// optionallyCloseWriter flushes writers that pack created for a phase and that +// buffer output, namely *logging.PrefixWriter, whose Close flushes any pending +// partial line without closing the writer it wraps. +// +// Writers owned by the caller must not be closed here. When a builder is +// trusted the logger's writer (often os.Stdout) is passed through unwrapped, and +// closing an arbitrary io.Closer would close the caller's stream. That is +// surprising and breaks consumers using pack as a library. See #1214. func optionallyCloseWriter(writer io.Writer) error { - if closer, ok := writer.(io.Closer); ok { - return closer.Close() + if prefixWriter, ok := writer.(*logging.PrefixWriter); ok { + return prefixWriter.Close() } return nil diff --git a/internal/container/run_test.go b/internal/container/run_test.go new file mode 100644 index 000000000..11b776bda --- /dev/null +++ b/internal/container/run_test.go @@ -0,0 +1,74 @@ +package container + +import ( + "bytes" + "io" + "testing" + + "github.com/heroku/color" + "github.com/sclevine/spec" + "github.com/sclevine/spec/report" + + "github.com/buildpacks/pack/pkg/logging" +) + +func TestContainerRun(t *testing.T) { + color.Disable(true) + defer color.Disable(false) + + spec.Run(t, "container/run", testContainerRun, spec.Report(report.Terminal{}), spec.Parallel()) +} + +// recordingWriteCloser models a writer owned by the caller that also happens to +// implement io.Closer, such as os.Stdout passed to pack via the logger. +type recordingWriteCloser struct { + bytes.Buffer + closed bool +} + +func (w *recordingWriteCloser) Close() error { + w.closed = true + return nil +} + +var _ io.WriteCloser = &recordingWriteCloser{} + +func testContainerRun(t *testing.T, when spec.G, it spec.S) { + when("#optionallyCloseWriter", func() { + it("does not close writers owned by the caller", func() { + writer := &recordingWriteCloser{} + + if err := optionallyCloseWriter(writer); err != nil { + t.Fatalf("expected no error, got: %v", err) + } + + // Closing a caller-owned writer (e.g. os.Stdout) is surprising and + // breaks consumers using pack as a library. See #1214. + if writer.closed { + t.Fatal("expected caller-owned writer to remain open, but it was closed") + } + }) + + it("flushes buffered output from a PrefixWriter", func() { + var buf bytes.Buffer + prefixWriter := logging.NewPrefixWriter(&buf, "some-prefix") + + // A line without a trailing newline stays buffered until the writer + // is flushed. + if _, err := prefixWriter.Write([]byte("buffered output")); err != nil { + t.Fatalf("writing to prefix writer: %v", err) + } + if got := buf.String(); got != "" { + t.Fatalf("expected partial line to stay buffered, got: %q", got) + } + + if err := optionallyCloseWriter(prefixWriter); err != nil { + t.Fatalf("expected no error, got: %v", err) + } + + if got := buf.String(); !bytes.Contains([]byte(got), []byte("buffered output")) { + t.Fatalf("expected buffered output to be flushed, got: %q", got) + } + }) + }) +}