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

internal/singleflight: delete, use golang.org/x/sync/singleflight #31697

Open
bradfitz opened this issue Apr 26, 2019 · 12 comments
Open

internal/singleflight: delete, use golang.org/x/sync/singleflight #31697

bradfitz opened this issue Apr 26, 2019 · 12 comments
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@bradfitz
Copy link
Contributor

The singleflight package started life in internal/singleflight and then moved to golang.org/x/sync/singleflight, and the two have had slightly divergent development histories since. It's getting kinda messy. Their APIs have even somewhat diverged.

I just tried to re-unify them and delete internal/singleflight from std, but we can't use mod/vendor packages during bootstrap.

/cc @ianlancetaylor

@bradfitz bradfitz added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Apr 26, 2019
@bradfitz bradfitz added this to the Go1.13 milestone Apr 26, 2019
@gopherbot
Copy link

Change https://golang.org/cl/174080 mentions this issue: src/vendor: add golang.org/x/sync/singleflight, delete internal/singleflight

@cuonglm
Copy link
Member

cuonglm commented Apr 29, 2019

Another step to re-unify them is:

I vote for 1st point, 2nd point will break current user of golang.org/x/sync/singleflight out there. How do you think @bradfitz ?

@bradfitz
Copy link
Contributor Author

We can't break (change) the x/sync/singleflight public API.

We can add a new method, though.

@tarndt
Copy link

tarndt commented Apr 30, 2019

If github.com/golang/sync another fork or is that just a clone of golang.org/x/sync? I've been unsure which should be imported.

@cuonglm
Copy link
Member

cuonglm commented Apr 30, 2019

@tarndt Use golang.org/x/sync, it's custom import path for github.com/golang/sync https://github.com/golang/sync/blob/master/singleflight/singleflight.go#L7

@gopherbot
Copy link

Change https://go.dev/cl/423654 mentions this issue: net: avoid relying on singleflight.Group.DoChan to detect hook called

@gopherbot
Copy link

Change https://go.dev/cl/423655 mentions this issue: internal/singleflight: make DoChan only return Result channel

@bradfitz
Copy link
Contributor Author

bradfitz commented Aug 16, 2022

@cuonglm, rather than delete internal/singleflight, how about taking advantage of its private nature and give it generics like we did in our https://pkg.go.dev/tailscale.com/util/singleflight fork?

The std can use it with types instead of the any boxing.

(Unless you want to wait for #53427; but that could be a long time)

@cuonglm
Copy link
Member

cuonglm commented Aug 16, 2022

@cuonglm, rather than delete internal/singleflight, how about taking advantage of its private nature and give it generics like we did in our https://pkg.go.dev/tailscale.com/util/singleflight fork?

The std can use it with types instead of the any boxing.

(Unless you want to wait for #53427; but that could be a long time)

That sounds interesting, but I think we still need to wait until we bump the bootstrap version to 1.18 at least, for supporting generic?

The main motivation is that we should unify them, so we need to maintain one version only.

@cuonglm
Copy link
Member

cuonglm commented Aug 16, 2022

That sounds interesting, but I think we still need to wait until we bump the bootstrap version to 1.18 at least, for supporting generic?

Ah never mind, we don't have to, since when singleflight is only used in std, not during bootstrap tools.

gopherbot pushed a commit that referenced this issue Aug 24, 2022
So next CLs can revert changes to "internal/singleflight" in CL #82795,
then replace it with "golang.org/x/sync/singleflight" instead.

For #31697

Change-Id: I873ce30d7e051539aa6dc5d4f21e558869a6d132
Reviewed-on: https://go-review.googlesource.com/c/go/+/423654
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
gopherbot pushed a commit that referenced this issue Aug 24, 2022
So next CL can delete "internal/singleflight" and vendor
"golang.org/x/sync/singleflight" instead.

For #31697

Change-Id: I020da1e5a48d484637b538c010029218f5a4a744
Reviewed-on: https://go-review.googlesource.com/c/go/+/423655
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link

Change https://go.dev/cl/425187 mentions this issue: all: switch singleflight to use generic

@cuonglm
Copy link
Member

cuonglm commented Aug 24, 2022

@bradfitz See CL 425187 for adding generic to singleflight. We can keep it then, until #53427 accepted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants