golang.org/x/sync/singleflight: add Use method to perform cleanup of temp resource after last use

[gleonid]

Background:

We are using singleflight to run expensive procedure that creates a temporary resource (temp file). Our challenge - we need to determine when it is safe to cleanup this temp resource. Currently Group.Do/DoChan methods only return a shared bool indicator, but they do not surface to caller how many actual callers will be using the result.

Proposal to consider:

Currently implementation maintains "dups int" property for each call, semantically it is almost exactly a "reference counter" except in case of single caller it holds value 0 instead of 1. I propose, to introduce a new Method DoRefCount with signature virtually identical to one of Do method, except instead of shared bool it will be returning a refCount RefCounter(pointer to call.dups), which will hold a pointer to a reference counter (pointer to call.dups ?)

This PR provides an implementation for above

this PR replaces https://github.com/golang/sync/pull/8

[gopherbot]

This PR (HEAD: be10acbae9b64aa34e18b408ce17408dcb4f4d53) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/sync/+/213797 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off) See the Wiki page for more info

[gopherbot]

Message from Leonid Gershanovich:

Patch Set 2:

this PR is a replacement for https://go-review.googlesource.com/c/sync/+/213478

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/213797. After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Brad Fitzpatrick:

Patch Set 2:

(4 comments)

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/213797. After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Brad Fitzpatrick:

Patch Set 2:

(1 comment)

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/213797. After addressing review feedback, remember to publish your drafts!

[gopherbot]

This PR (HEAD: 5e2433882d00cb5bf3748bac1fb10de51963d87b) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/sync/+/213797 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off) See the Wiki page for more info

[gopherbot]

Message from Dave Cheney:

Patch Set 3:

(1 comment)

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/213797. After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Brad Fitzpatrick:

Patch Set 7:

I thought of a nicer (IMO) way to implement this:

Instead define an interface like:

type SharedValuer interface{ SharedValue(refCount int) (interface{}, error) }

And document that if the fn given to Do (or DoChan) returns an interface{} implementing SharedValuer, then its SharedValue method is called (with the initial refcount) to return the value to each dup caller.

Then it's the return value's responsibility to do its own cleanup (e.g. have a Close method and on the final 1->0 refcount transition, close the real underlying resource)

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/213797. After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Leonid Gershanovich:

Patch Set 7:

(6 comments)

Patch Set 7:

I thought of a nicer (IMO) way to implement this:

Instead define an interface like:

type SharedValuer interface{ SharedValue(refCount int) (interface{}, error) }

And document that if the fn given to Do (or DoChan) returns an interface{} implementing SharedValuer, then its SharedValue method is called (with the initial refcount) to return the value to each dup caller.

Then it's the return value's responsibility to do its own cleanup (e.g. have a Close method and on the final 1->0 refcount transition, close the real underlying resource)

I do not particularly like this approach as it creates a requirement for functor used with singleflight. In other words, if I want to "singleflight" a particular function - it has to be written in a special way. Such requirement seems to be non intuitive, as "number of callers" is a notion introduced and maintained by singleflight package itself, thus IMO it is (and should remain) completely orthogonal/independent from functor itself.

Also this approach means that same chunk of code (implementing SharedValuer interface and Close/decrementing) will have to be repeated by everyone who want to use it, which is more error prone that doing it once.

Having said that, your approach will definitely work. Although I am not 100% clear of what interface of SharedValuer.SharedValue return value is expected to be and what kind of error might be occurring on SharedValuer.SharedValue.

I have created this PR with 2 purposes:

1. explain what feature I believe will be useful in singleflight package
2. to naively illustrate how it can be implemented

Bottom line: if you believe that feature I am requesting will benefit package and community and have a preference how it should be done - I'd be very happy with any implementation.

Thanks for considering it Leonid

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/213797. After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Bryan C. Mills:

Patch Set 7:

Patch Set 7:

(6 comments)

Patch Set 7:

I thought of a nicer (IMO) way to implement this:

Instead define an interface like:

type SharedValuer interface{ SharedValue(refCount int) (interface{}, error) }

And document that if the fn given to Do (or DoChan) returns an interface{} implementing SharedValuer, then its SharedValue method is called (with the initial refcount) to return the value to each dup caller.

Then it's the return value's responsibility to do its own cleanup (e.g. have a Close method and on the final 1->0 refcount transition, close the real underlying resource)

I do not particularly like this approach as it creates a requirement for functor used with singleflight. In other words, if I want to "singleflight" a particular function - it has to be written in a special way. Such requirement seems to be non intuitive, as "number of callers" is a notion introduced and maintained by singleflight package itself, thus IMO it is (and should remain) completely orthogonal/independent from functor itself.

Also this approach means that same chunk of code (implementing SharedValuer interface and Close/decrementing) will have to be repeated by everyone who want to use it, which is more error prone that doing it once.

I'm not keen on exposing reference-counts at all. What about something more lifetime-oriented?

Consider, as an alternative:

// Use calls new at most once at a time, then invokes fn with the resulting value. // // If the dispose argument is non-nil, Use invokes it after the last call to fn has returned. func (g *Group) Use(key string, new func() (interface{}, error), dispose func(interface{}), fn func(interface{}) error) error

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/213797. After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Leonid Gershanovich:

Patch Set 7:

Patch Set 7:

Patch Set 7:

(6 comments)

Patch Set 7:

I thought of a nicer (IMO) way to implement this:

Instead define an interface like:

type SharedValuer interface{ SharedValue(refCount int) (interface{}, error) }

And document that if the fn given to Do (or DoChan) returns an interface{} implementing SharedValuer, then its SharedValue method is called (with the initial refcount) to return the value to each dup caller.

Then it's the return value's responsibility to do its own cleanup (e.g. have a Close method and on the final 1->0 refcount transition, close the real underlying resource)

I do not particularly like this approach as it creates a requirement for functor used with singleflight. In other words, if I want to "singleflight" a particular function - it has to be written in a special way. Such requirement seems to be non intuitive, as "number of callers" is a notion introduced and maintained by singleflight package itself, thus IMO it is (and should remain) completely orthogonal/independent from functor itself.

Also this approach means that same chunk of code (implementing SharedValuer interface and Close/decrementing) will have to be repeated by everyone who want to use it, which is more error prone that doing it once.

I'm not keen on exposing reference-counts at all. What about something more lifetime-oriented?

Consider, as an alternative:

// Use calls new at most once at a time, then invokes fn with the resulting value. // // If the dispose argument is non-nil, Use invokes it after the last call to fn has returned. func (g *Group) Use(key string, new func() (interface{}, error), dispose func(interface{}), fn func(interface{}) error) error

Couple of small concerns:

1. Should 'fn'/'dispose' be called, when 'new' returns an error? I am guessing no, but right now this is completely up to caller.
2. Should 'dispose' be called, when at least one 'fn' returns an error?
3. Code that to use Group.Use might be a bit harder to follow, for example:

var sg singleflight.Group
rc, err, refCount := sg.DoRefCount(key, longfunctor)
// use rc
filename := rc.(string)

defer func () {
  if refCount.Decrement() {
    doCleanup(filename)
  }
}
f, err := Openfile(filename)
defer f.Close()
...

will become

var sg singleflight.Group
err := sg.Use(key, 
          longfunctor,
          func (o interface{}) error {
             filename := o.(string)
             doCleanup(filename)
          },
          func (o interface{}) error {
             filename := o.(string)
             f, err := Openfile(filename)
             defer f.Close()
             ...
          },
       )

Other than that - I do like this approach

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/213797. After addressing review feedback, remember to publish your drafts!

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

[gopherbot]

This PR (HEAD: b39aa21ee1627f2838960c527ef1355031ef13f2) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/sync/+/213797 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off) See the Wiki page for more info

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[gopherbot]

Message from Gobot Gobot:

Patch Set 8:

Congratulations on opening your first change. Thank you for your contribution!

Next steps: Within the next week or so, a maintainer will review your change and provide feedback. See https://golang.org/doc/contribute.html#review for more info and tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be surprising to people new to the project. The careful, iterative review process is our way of helping mentor contributors and ensuring that their contributions have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which little code gets reviewed or merged. If a reviewer responds with a comment like R=go1.11, it means that this CL will be reviewed as part of the next development cycle. See https://golang.org/s/release for more details.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/213797. After addressing review feedback, remember to publish your drafts!

[gopherbot]

This PR (HEAD: 2baeae8687f4d47c88799a42e72d419dafc195ec) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/sync/+/213797 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off) See the Wiki page for more info

[gopherbot]

Message from Bryan C. Mills:

Patch Set 9:

(1 comment)

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/213797. After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Go Bot:

Patch Set 8:

Congratulations on opening your first change. Thank you for your contribution!

Next steps: Within the next week or so, a maintainer will review your change and provide feedback. See https://golang.org/doc/contribute.html#review for more info and tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be surprising to people new to the project. The careful, iterative review process is our way of helping mentor contributors and ensuring that their contributions have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which little code gets reviewed or merged. If a reviewer responds with a comment like R=go1.11, it means that this CL will be reviewed as part of the next development cycle. See https://golang.org/s/release for more details.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/213797. After addressing review feedback, remember to publish your drafts!