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 CloserFunc and WriterAndCloser #56373

Open
mei-rune opened this issue Oct 21, 2022 · 4 comments
Open

proposal: io: add CloserFunc and WriterAndCloser #56373

mei-rune opened this issue Oct 21, 2022 · 4 comments
Labels
Milestone

Comments

@mei-rune
Copy link

can add CloserFunc and WriterAndCloser

CloserFunc is:

   type CloserFunc  func() error
   
   func (fn CloserFunc) Close() error {
      return fn()
   }

 var _ io.Closer = CloserFunc(func() error {})

WriterAndCloser is:

type writeCloser struct {
	io.Writer
	io.Closer	
}

func WriterAndCloser(w io.Writer, c io.Closer) io.WriteCloser {
	return writeCloser{
		Writer: w,
		Closer: c,
	}
}

see: https://github.com/search?q=CloserFunc+language%3Ago&type=code

@gopherbot gopherbot added this to the Proposal milestone Oct 21, 2022
@seankhliao seankhliao changed the title proposal: io: can add CloserFunc and WriterAndCloser proposal: io: add CloserFunc and WriterAndCloser Oct 21, 2022
@ianlancetaylor
Copy link
Contributor

Your GitHub search seems to find a lot of cases of functions named CloserFunc, but that don't, and shouldn't, have a Close method. Can you point us to a few existing examples of code that would benefit from these additions? Thanks.

@mei-rune
Copy link
Author

mei-rune commented Oct 22, 2022

@ianlancetaylor
I found the close method in the 12 files in the first 3 pages of search results.

https://github.com/angry-tony/cortex-Prometheus-as-a-Service/blob/6d4678127598d65fe6405d4120f9911839fb00b6/pkg/ring/kv/etcd/mock.go#L70
https://github.com/go-kata/kdone/blob/edbd113fc705bc1d735d39f01b6429820bf5341a/closer.go
https://github.com/tgulacsi/go/blob/60a9e8c66e45d6e5c6571574b55bbd8f6c43d73e/iohlp/multicloser.go#L52
https://github.com/cloudradar-monitoring/tacoscript/blob/5bd90febe845b8b1f688f7f8bef2eb154f1fd101/utils/closer.go#L28
https://github.com/goph/fxt/blob/eb299b7150bc0e7c19cbabe277643526e4c37e17/lifecycle.go#L28
https://github.com/HewlettPackard/k8s-sigstore-attestor/blob/5458d8b4a3e2622180959edc9e762bbd8badc56a/pkg/common/catalog/closers.go#L30
https://github.com/ranchercus/rancher/blob/42c0368ca1b8fc1510017c2bf7575634d35014d2/pkg/clusterprovisioninglogger/log.go#L81
https://github.com/chatrale/khan/blob/3ed340c0e005cbbd4e3f42e0ff93bce9cf3d4123/iplist/packed.go#L117
https://github.com/MrE-Fog/acr-cli11/blob/5df1cc9da4197e67e770585d7d863bac75279f86/vendor/oras.land/oras-go/v2/internal/ioutil/io.go#L31
https://github.com/goph/stdlib/blob/426371efcfd69ba2b2bde885e973aeede7b78e10/ext/close.go#L20
https://github.com/djbanodkar/RancherTesting/blob/88bec4200cfe5738495c4c8291ad6b1c521dc1d8/pkg/clusterprovisioninglogger/log.go#L81
https://github.com/manasmishra/spire/blob/355ab564b8d73ce8d1f17b5db7f8b9edbc2472a2/pkg/common/catalog/closers.go#L32

@earthboundkid
Copy link
Contributor

I think this should wait until there's a negative resolution of #21670 / #25860 / #47487.

@apparentlymart
Copy link

apparentlymart commented Oct 27, 2022

This seems like two separate proposals which happen to compose together but which could be discussed independently.

I agree that the first part -- CloserFunc -- seems to be subsumable into all of the various proposals to allow more convenient use of isolated functions to implement interfaces, and so a decision on that should consider the outcome of a decision on those other proposals.

The WriterAndCloser part seems interesting in its own right though, and is similar to a situation I've encountered a small number of times: wrapping another function which returns an io.Reader and hooking an additional Close side-effect onto it, to encapsulate some additional cleanup that needs to happen when passing the result to a function which accepts any arbitrary io.ReadCloser.

When I most recently had that need I optimistically looked into the io package for something with a similar but not quite identical design as this proposal:

func ReaderWithClose(r io.Reader, close func () error) io.ReadCloser

...which is a similar idea as the existing io.NopCloser but with an additional hook function to call in Close, rather than just immediately return nil.

I ended up just implementing this myself, since of course the implementation was pretty trivial for my specific case:

type readerWithClose struct {
	io.Reader
	close func() error
}

func ReaderWithClose(r io.Reader, close func() error) io.ReadCloser {
	return readerWithClose{
		Reader: r,
		close:  close,
	}
}

func (r readerWithClose) Close() error {
	return r.close()
}

I considered generalizing it into a reusable library but found some interesting questions when considering use-cases beyond my immediate need:

  • What should happen if the given io.Reader is also already an io.Closer? Does the new Close method call both the close callback and the underlying reader's Close? If so, what happens if one of them fails and the other succeeds, or if both of them fail?

  • What about all of the other optional interfaces the given reader could possibly implement? I see that io.NopCloser itself already contains a special case for when the given reader is an io.WriterTo:

    func NopCloser(r Reader) ReadCloser {
    	if _, ok := r.(WriterTo); ok {
    		return nopCloserWriterTo{r}
    	}
    	return nopCloser{r}
    }

    I assume that this hypothetical ReaderWithClose would need to do something similar in order to preserve the io.Copy optimization that io.WriterTo implies.

Of course I realize that I'm talking about readers while the original proposal was talking about writers, and so my details here are off-topic. But I'm sharing this because I think there are equivalent questions for io.WriterAndCloser (as proposed here) or io.WriterWithClose (a write variant of my ReaderWithClose), and because it seems to me that if this proposal were accepted for writers then it would make sense to accept it for readers too, for consistency/symmetry.

(The question of whether the second argument to this function should be just a function or an io.Closer of course amounts to re-asking the question about the io.CloserFunc part of this proposal; I don't feel strongly either way about this and am intending to focus only on the questions that seem to arise when hooking an extra Close behavior onto an existing object, regardless of the exact interface used to do so.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

5 participants