buildpacks / pack

CLI for building apps using Cloud Native Buildpacks
https://buildpacks.io
Apache License 2.0
2.58k stars 289 forks source link

pack.Client::Build() closes logger's `io.Writer` #1214

Open matejvasek opened 3 years ago

matejvasek commented 3 years ago

Summary

I am using pack code as a library in my own Go project. I am not sure how much is this supported and whether this is really a bug.

Still:

        // logWriter is os.Stdout in verbose mode of this app
    packClient, err := pack.NewClient(pack.WithLogger(logging.New(logWriter)))
    if err != nil {
        return
    }

    // the builder must be trusted to reproduce the issue
    if err = packClient.Build(ctx, packOpts); err != nil {
        }
        // os.Stdout is closed here

The issue is that after invocation of packClient.Build() the logWriter is closed.

There is a code that check whether logWriter instance also implements io.Closer and if it does it will call close on it deliberately. In case that the builder is trusted the logWriter (os.Stdout) is unaltered passed down and closed (since it is *os.File). This is not good, especially for stdout.

In case the builder is not trusted the logWriter is wrapped by logging.PrefixWriter which is not io.Closer so everything works all right.

In standalone pack CLI it's not an issue because the os.Stdout is closed just before exiting, but if used a an library this is not convenient.

For now I workarounded this by wrapping os.Stdout into a struct that implements only io.Writer not io.Closer.

IMHO the io.Writer passed to the API shouldn't be closed, it's really surprising when that happens.

It might make sense if the parameter had to be io.WriteCloser, then user could expect that the stream will be closed.

matejvasek commented 3 years ago

logging.PrefixWriter which is not io.Closer so everything works all right.

correction: logging.PrefixWriter actually is io.Closer but it doesn't really close the underlying io.Writer that it is wrapping

matejvasek commented 3 years ago

this was probably introduced in https://github.com/buildpacks/pack/pull/867

matejvasek commented 3 years ago

cc @jromero

natalieparellano commented 2 years ago

@matejvasek are you still seeing this issue?