Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: io/ioutil: add WriteNopCloser #22823

Closed
robpike opened this issue Nov 20, 2017 · 12 comments · May be fixed by #31892
Closed

proposal: io/ioutil: add WriteNopCloser #22823

robpike opened this issue Nov 20, 2017 · 12 comments · May be fixed by #31892

Comments

@robpike
Copy link
Contributor

@robpike robpike commented Nov 20, 2017

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.

@robpike robpike changed the title ioutil: add NopWriterCloser ioutil: add ioutil.WriteNopCloser Nov 20, 2017
@odeke-em odeke-em changed the title ioutil: add ioutil.WriteNopCloser io/ioutil: add WriteNopCloser Nov 20, 2017
@karalabe

This comment was marked as disruptive content.

Copy link
Contributor

@karalabe karalabe commented Nov 20, 2017

🔴 :trollface:

@smasher164

This comment has been minimized.

Copy link
Member

@smasher164 smasher164 commented Nov 21, 2017

The permutations go from viable to esoteric 😏:

WriteNopCloser
NopWriteCloser
NopCloseWriter
WriteCloseNopper
CloseWriteNopper
CloseNopWriter
@titanous titanous added this to the Proposal milestone Nov 21, 2017
@titanous titanous added the Proposal label Nov 21, 2017
@tylerchr

This comment has been minimized.

Copy link

@tylerchr tylerchr commented Sep 18, 2018

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[^1] 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.

[^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.

sj14 added a commit to sj14/go that referenced this issue May 7, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 7, 2019

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

@robarchibald

This comment has been minimized.

Copy link

@robarchibald robarchibald commented Oct 7, 2019

Would love to see this PR accepted!

@mlh758

This comment has been minimized.

Copy link

@mlh758 mlh758 commented Nov 26, 2019

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.

@mvdan mvdan changed the title io/ioutil: add WriteNopCloser proposal: io/ioutil: add WriteNopCloser Nov 26, 2019
@rsc rsc added this to Incoming in Proposals Nov 27, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 27, 2019

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

This comment has been minimized.

Copy link
Contributor

@magical magical commented Nov 28, 2019

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

This comment has been minimized.

Copy link

@mlh758 mlh758 commented Nov 29, 2019

@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.

@rsc rsc moved this from Incoming to Active in Proposals Dec 4, 2019
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Dec 4, 2019

@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

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Dec 11, 2019

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 rsc moved this from Active to Likely Decline in Proposals Dec 11, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jan 8, 2020

No final comments, so declining.

@rsc rsc closed this Jan 8, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

You can’t perform that action at this time.