proposal: io/ioutil: add WriteNopCloser

[robpike]

The type ioutil.NopCloser allows us to add a no-op Close to an existing Reader when a ReadCloser is required.

For symmetry, there should also be a ioutil.WriteNopCloser to do the same for Write.

The name can be argued about. Get out your bikeshed paint brushes.

[smasher164]

The permutations go from viable to esoteric 😏:

WriteNopCloser
NopWriteCloser
NopCloseWriter
WriteCloseNopper
CloseWriteNopper
CloseNopWriter

[tylerchr]

I support this proposal out of practical experience. I just ran across a use case where I needed just this function, and settled for a suboptimal design because it didn't exist.

My application produces a log file whose path is configurable via environment variable. If the variable is provided, it's assumed to be a file path and an *os.File (i.e., an io.WriteCloser) is opened; if the variable is not provided I want to use ioutil.Discard. I wanted to abstract this logic into a function that returns an io.WriteCloser so I could defer x.Close() in the caller:

func main() {
    logWriter := GetLogWriter()
    defer logWriter.Close()
}

func GetLogWriter() io.WriteCloser {
    if path, ok := os.LookupEnv("LOG_PATH"); ok {
        if file, err := os.Create(path); err == nil {
            return file
        }
    }
    return ioutil.WriteNopCloser(ioutil.Discard)
}

But, having no trivial way to get an io.WriteCloser from an ioutil.Discard, GetLogWriter wasn't possible to write¹ and I settled for some rather convoluted logic directly in the caller:

func main() {

	var logWriter io.Writer = ioutil.Discard

	if path, ok := os.LookupEnv("LOG_PATH"); ok {
		if file, err := os.Create(path); err != nil {
			panic("sad day")
		} else {
			defer file.Close()
			logWriter = file
		}
	}

}

This works, but it's not great. Specifically, the conditional defer looks like a guaranteed future bug because it breaks from the familiar idiomatic usage.

Footnotes

1. Yes, I could have implemented a variant of ioutil.WriteNopCloser on my own, but the point is that I didn't. I'm settling for more dangerous code instead. ↩

[gopherbot]

Change https://golang.org/cl/175779 mentions this issue: io/ioutil: add WriteNopCloser

[robarchibald]

Would love to see this PR accepted!

[mlh758]

Just ran across a use case for this as well. I have a function that returns a WriteCloser to be used for caching. If the cache file can't be written to I still want everything to succeed without having to do any major alternative paths in the request handler so instead I made a WriterCloser out of Discard basically how this is and log the cache failure separately.

[rsc]

To restate the argument in favor of adding this, the situation where it comes up is that you need to implement an interface method that is defined to return an io.WriteCloser. And all you have is a io.Writer. The implication is that the caller calls Close to say "all the writes are done", which usually triggers processing of the collected writes. So nopping out Close would be an odd thing to do or to make very easy.

The specific case of ioutil.Discard is unfortunate: we probably should have defined that as its own type instead of an io.Writer, and then it could be changed to have a Close method. We could still do that - add a Close method on the underlying implementation - and document that it is safe to use ioutil.Discard.(io.WriteCloser).

Are there examples other than ioutil.Discard?

The situation is different from Read/ReadCloser because closing a reader is more of a courtesy, compared to "now do something with everything I wrote" for closing a writer.

WriteNopCloser may still be worth adding but we should understand the reasons better first.

[magical]

I saw an interesting use of WriteNopCloser in age. The age.EncryptWithArmor function takes an io.Writer and returns an io.WriteCloser. When closed, it flushes its buffer and writes a message trailer, but does not close the underlying writer.

https://github.com/FiloSottile/age/blob/a070570595a3f0a154f106b1c0ea7bc76ece1d89/internal/age/age.go#L42

func EncryptWithArmor(dst io.Writer, recipients ...Recipient) (io.WriteCloser, error) {

Internally this is implemented by wrapping the user-provided writer in types that handle encryption and base64 encoding, plus, at the bottom of the chain, the equivalent of a WriteNopCloser to stop propagation of the Close call.

https://github.com/FiloSottile/age/blob/a070570595a3f0a154f106b1c0ea7bc76ece1d89/internal/age/age.go#L81-L84

		// stream.Writer takes a WriteCloser, and will propagate Close calls (so
		// that the ArmoredWriter will get closed), but we don't want to expose
		// that behavior to our caller.
		finalDst = format.NopCloser(dst)

[mlh758]

@rsc If you were replying to me, that's essentially what I was getting at. In my case I was making a basic file system cache and I wanted to maintain the illusion to the caller that everything passed even if say the program didn't have write permission or something so I put a no-op close method on discard and return that as a WriterCloser.

If perhaps I wanted to have the option to compress my cache file with archive/zip then the Create method on ZipWriter also doesn't have a Close since the data is actually flushed when the overall zip object is closed.

[bcmills]

@magical, arguably the example in github.com/FiloSotille/age/internal/age indicates an API defect in github.com/FiloSottile/age/internal/stream.

stream.NewWriter could just as easily have accepted an io.Writer and left the decision about whether and when to Close it up to the caller.

Then the implementation in github.com/FiloSotille/age/internal/age would provide a non-trivial Close method for the armor case (to close both the *stream.Writer and the io.WriteCloser returned by format.ArmoredWriter), rather than a trivial Close function for the !armor case.

[rsc]

Two weeks ago I asked about motivating use cases. So far there has only been one, which on closer examination turns out not to be that compelling. The fundamental problem is that Close on a WriteCloser is very often semantically meaningful, whereas Close on a ReadCloser often has very little semantic meaning. So a nop'ed ReadCloser is helpful while a nop'ed WriteCloser is more often a mistake.

Based on the discussion so far, this seems like a likely decline.

Leaving open for a week for final comments.

[rsc]

No final comments, so declining.