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

Feature request: singleflight to return refcount vs shared bool #8

Closed
wants to merge 6 commits into from

Conversation

gleonid
Copy link

@gleonid gleonid commented Jan 6, 2020

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 calls only return a shared bool indicator, but they do not surface to caller how many actual caller will be using this 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 slightly change signature of Group.Do/DoChan: in addition to "shared bool" also return the actual reference counter (pointer to call.dups ?)

This PR provides an implementation for above
In short it does 2 things:

  • defines a refShared type and changes Group.Do signature to use in lace of shared bool
  • "refactor Group.Do implement via Group.Dochan call" (commit 4eee079)
    This slightly reduces duplicated code and will prevent a race condition on computation of shared flag. This race condition is introduced by previous item, as calculation of shared bool depends on value call.dups and that value could get changed by caller now.

Note

It might be easier to review this PR on per commit basis

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@gleonid
Copy link
Author

gleonid commented Jan 6, 2020

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Jan 6, 2020
@gleonid gleonid changed the title Feature request: Return refcount vs shared bool Feature request: singleflight to return refcount vs shared bool Jan 6, 2020
@gopherbot
Copy link

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

Please visit https://go-review.googlesource.com/c/sync/+/213478 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
Copy link

Message from Gobot Gobot:

Patch Set 1:

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/213478.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Brad Fitzpatrick:

Patch Set 1:

(6 comments)


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

@gopherbot
Copy link

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

Please visit https://go-review.googlesource.com/c/sync/+/213478 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

@gleonid gleonid force-pushed the return_refcount_vs_shared_bool branch from 91fa807 to 55a4369 Compare January 6, 2020 22:00
@gopherbot
Copy link

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

Please visit https://go-review.googlesource.com/c/sync/+/213478 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

@gleonid
Copy link
Author

gleonid commented Jan 8, 2020

declining this PR in favor of #9

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

Successfully merging this pull request may close these issues.

3 participants