proposal: io: add OnceCloser helper

[bradfitz]

Proposal, to add to the io package:

// OnceCloser returns a Closer wrapping c that guarantees it only calls c.Close
// once and is safe for use by multiple goroutines. Each call to the returned Closer
// will return the same value, as returned by c.Close.
func OnceCloser(c Closer) Closer {
	return &onceCloser{c: c}
}

With implementation like:

type onceCloser struct {
	c    Closer
	once sync.Once
	err  error
}

func (c *onceCloser) Close() error {
	c.once.Do(c.close)
	return c.err
}

func (c *onceCloser) close() { c.err = c.c.Close() }

This would let people safely write code like:

       oc := io.OnceCloser(file)
       defer oc.Close()

       ....
       if err != nil {
             return nil, err
       }
       if err := oc.Close(); err != nil {
             return nil, err
      }

... without worrying about double calls to file.Close, which is undefined since https://golang.org/cl/8575043

https://golang.org/pkg/io/#Closer

// The behavior of Close after the first call is undefined.
// Specific implementations may document their own behavior.

/cc @bcmills @mpvl @ianlancetaylor

[bcmills]

This is a possible alternative to #25390.

[jimmyfrasche]

On one hand this is a little simple for the stdlib but on the other it's a nasty thing everyone has to workaround at some point (and a cleaner solution than any I've come up with in the past).

If it goes in, it would make more sense to put this in io/ioutil next to NopCloser.

Since the wrapped Closer this returns hides all the methods except Close, perhaps it should just return func() error.

[davecheney]

I like everything about this proposal save the name of the type. Then again, CloseOncer is worse 😞

[bradfitz]

Yeah, ioutil could be a better place for this.

[jimmyfrasche]

IdempotentCloser because that word is super fun to say. But, seriously, I like OnceCloser because it's simple and evocative of its sync.Once + io.Closer nature.

[bcmills]

It would make me sad to put anything more in ioutil (see #19660). OnceCloser certainly parallels NopCloser, but it also parallels some other things in the io package (like io.MultiWriter).

That said, we could always clean it up if/when we clean up NopCloser for Go 2.

[bradfitz]

I see arguments for both io and io/ioutil. I don't care strongly either way, but I also agree we should clean up util packages for any Go 2.

[dsnet]

This approach has the advantage that it requires no changes to the io.Closer interface. However, I'm concerned that this helper primarily helps those that actually realize io.Closer is not idempotent and I fear that improper usages will still continue for those that don't realize this.

(not saying we shouldn't add this helper for Go1 at least)

[cznic]

I think that those 10 SLOC could and should be written outside the stdlib in user code when it makes sense, which I'm not sure can happen in proper code. The helper reminds me of something similar I think I saw somewhere for preventing closing of a closed channel. If it's not sound code in the case of channel, why it is in the case of an io.Closer?

[bcmills]

If it's not sound code in the case of channel, why it is in the case of an io.Closer?

Closing a channel does not return an error that needs to be checked, so a channel-close is much easier to defer correctly.

But perhaps that is an argument for not synchronizing OnceCloser: the two calls would usually be within the same function (one deferred and one with an error check), not in separate goroutines.

[bradfitz]

@cznic, the difference between a channel and an io.Closer is that a channel is an implementation, and io.Closer is an interface. The channel behavior is documented. The io.Closer expected behavior on double-Close was never documented (on whether callers should not double Close, or how/whether implementations should handle double Close), so now we have tons of implementations of io.Closer with different behaviors. They roughly fall into three classes of behavior with respect to double closes:

• return nil
• return the original Close value
• deadlock or crash

The final case is often because they were implemented using a channel underneath.

But in general, as a user of an io.Closer, it's not safe to double Close one because you don't know what it's going to do, and you have to assume it might deadlock or crash. The OnceCloser sanitizes its behavior.

I think that those 10 SLOC could and should be written outside the stdlib

We've had it in https://godoc.org/go4.org/types#NewOnceCloser for over 2 years, and it was in the @perkeep codebase for years before that. The point of putting in the standard library is so it's easily accessible for people who want to sanitize an io.Closer.

[as]

Why should there be a helper to synchronize an io.Closer and not an io.Reader or io.Writer? It seems like this is just a helper to step around the concurrency model of the language.

The final case is often because they were implemented using a channel underneath.

There's an app for that
https://play.golang.org/p/KFadFvLCEMj

[bcmills]

It seems like this is just a helper to step around the concurrency model of the language.

As I noted above: the main use-case for this proposal is for error-handling, not concurrency.

Why should there be a helper to synchronize an io.Closer and not an io.Reader or io.Writer?

Failing to call Read on an io.Reader or Write on an io.Writer rarely leaks resources. On the other hand, failing to Close an io.Closer often does: for example, a goroutine reading from a socket or pipe.

[rsc]

This depends on what the suggested patterns / best practices are for using closers, which is tied up with error handling. I don't think we know that yet.

[seebs]

Suggested patterns/best-practices appear to be:

• Always call defer f.Close() after opening something which needs closing and won't be surviving this function, so it gets cleaned up.
• When writing to a thing which can have errors, always call f.Close() directly, to check for I/O errors.
• Definitely don't call f.Close() twice.

Unfortunately, you can follow at most two of these.

As a user, I think my preferred answer would be "define multiple close calls to be safe after all". Of course, that breaks at least some existing code, but I don't know how many cases there are where it's unsafe and would still be unsafe even if it became safe for channels. (Defining previously-undefined behavior is usually a lot safer than the other way around.)

[bcmills]

Defining previously-undefined behavior is usually a lot safer than the other way around.

That's true of implementations, but not of interfaces. Adding new behavior to an interface potentially changes a correct implementation into an incorrect one, often with no visible error at compile-time.

As Russ notes in https://research.swtch.com/vgo-import, “when you change a function’s behavior, you also change its name.” That's especially vital when you don't control the implementation of that function.

[seebs]

... Yes, in retrospect that's pretty obvious and I'm not sure why I missed it.

The current situation is sort of ugly; in practice, people tend not to actually do the full work to implement OnceCloser, thus there's mutexes and checks for nil and so on. And if two things execute err = oc.Close() concurrently, I think the write to err in the one which gets to execute the once.Do() code can race with the read in the other one.

So, there's a definite actual difficulty, and the workarounds for it are difficult to do quite correctly.