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: add OnceCloser helper #25408

Open
bradfitz opened this issue May 15, 2018 · 17 comments
Open

proposal: io: add OnceCloser helper #25408

bradfitz opened this issue May 15, 2018 · 17 comments

Comments

@bradfitz
Copy link
Contributor

bradfitz commented May 15, 2018

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

@gopherbot gopherbot added this to the Proposal milestone May 15, 2018
@bcmills
Copy link
Contributor

bcmills commented May 15, 2018

This is a possible alternative to #25390.

@jimmyfrasche
Copy link
Member

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
Copy link
Contributor

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

@bradfitz
Copy link
Contributor Author

Yeah, ioutil could be a better place for this.

@jimmyfrasche
Copy link
Member

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
Copy link
Contributor

bcmills commented May 16, 2018

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
Copy link
Contributor Author

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
Copy link
Member

dsnet commented May 16, 2018

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
Copy link
Contributor

cznic commented May 16, 2018

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
Copy link
Contributor

bcmills commented May 16, 2018

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
Copy link
Contributor Author

@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
Copy link
Contributor

as commented May 17, 2018

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
Copy link
Contributor

bcmills commented May 21, 2018

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
Copy link
Contributor

rsc commented May 21, 2018

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
Copy link
Contributor

seebs commented Jun 1, 2018

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
Copy link
Contributor

bcmills commented Jun 4, 2018

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
Copy link
Contributor

seebs commented Jun 12, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants