proposal: x/sync/singleflight: add generic version

[bradfitz]

The singleflight package's Group type uses string keys and interface{} values:

https://pkg.go.dev/golang.org/x/sync/singleflight#Group

I've locally forked it to add generics [K comparable, V any] but I'd like to get it upstream instead.

Proposal: add singleflight.TypedGroup[K, V] or singleflight.GenericGroup[K, V] or singleflight.GroupOf[K, V] behind a go1.18 build tag.

Do we have a convention yet on naming new parallel generic types alongside others?

Related: #47657 (for PoolOf, MapOf). (There was also talk of default type parameters so old code could instantiate types and get the old behavior (in this case [string, any]) and new callers could specify types?)

[ianlancetaylor]

I don't think we have a convention yet.

[icholy]

What about x/sync/v2/singleflight ?

[catatsuy]

I tried it before. Thank you.

refs: #52520

[gopherbot]

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

[lmittmann]

Is there any update on this proposal?

[ianlancetaylor]

@lmittmann No.

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[rsc]

Maybe it should be x/sync/singleflight/v2.Group[K, V]? Note that even the current v0.2.0 can have a subdirectory package named v2 - it doesn't require tagging the whole repo v2.

[rsc]

This really circles back to #48287, which we didn't resolve.

[lmittmann]

I know this might sound crazy, but how about simply introducing a breaking change in an experimental package, which is v0, and has "looser compatibility requirements"?

[doggedOwl]

I agree that in this case and more generally in x/* experimental packages the right thing to do is a breaking change, eitherwise the whole purpose of these packages is mute and might as well put them directly under the standard library.

[rsc]

Holding for #48287.

[rsc]

Placed on hold.
— rsc for the proposal review group

[lmittmann]

@rsc the new discussion around math/rand/v2 (#60751) seems like there is now a consensus on how to update APIs for generics. Might this clear the way for x/sync/singleflight/v2.Group[K, V]?

[lmittmann]

Might this clear the way for x/sync/singleflight/v2.Group[K, V]?

@rsc @bradfitz have you had a chance to look into this yet?

[jmhodges]

I took a crack at the v2 formulation for this change. I've posted it as a draft because it differs enough from the original proposal that I didn't want to send it out for full review unless I heard from @bradfitz or @rsc that this was an okay way to use the original.

Happy to instead create a new proposal ticket for the v2 version of this if those folks would prefer!

[samber]

I made a simple lib for experiments: https://github.com/samber/go-singleflightx

• generics
• nullable output
• batching
• sharded groups

[costela]

@rsc this is marked as "on hold" pending the discussion on #48287, which was closed shortly thereafter, AFAICT without a follow-up.

As @lmittmann mentions, the release of math/rand/v2 seems to indicate this blocker has been removed and the proposal could be put back on the table?

[ianlancetaylor]

Thanks. Moving back to incoming.

[derekperkins]

I know this might sound crazy, but how about simply introducing a breaking change in an experimental package, which is v0, and has "looser compatibility requirements"?

This would be my preference, but either that or a v2 is preferable to not shipping anything

[dfinkel]

@derekperkins
The x/ repos aren't necessarily "experimental" so much as "extended". There's an actual x/exp for experimental stuff.

A lot relies on the x/sync/singleflight package (including many libraries). It really doesn't make sense to make a breaking change here.

A v2 is a much better option. (as was discussed above)

Hopefully this makes it to the head of the proposal review committee's queue in the near-ish future.

[derekperkins]

The x/ repos aren't necessarily "experimental" so much as "extended"

I'm just commenting based on the v0.8 release tag, which allows breaking changes. v2 being a "much better option" didn't receive consensus above, so I was offering my opinion. Either way is fine.

[catatsuy]

As the singleflight API is considering adding generics, this is a good opportunity to simplify and improve its design. I would like to propose three changes that could enhance the API while maintaining its core functionality:

---

Proposed Changes

1. Asynchronous Deletion of Completed Calls

   • Problem: Currently, completed calls are deleted synchronously, which requires holding a lock. This can cause contention under high load.
   • Proposed Solution: Delete completed calls asynchronously to improve throughput and reduce lock contention. While this may temporarily allow access to stale values, this is rare and acceptable for most use cases.

2. Removal of the shared Flag

   • Problem: The shared flag adds complexity to the API and its internal implementation.
   • Proposed Solution: Remove the shared flag and simplify the return value of Do to (V, error). This allows for:
     • Elimination of the dups field and related logic.
     • Reduction of unnecessary locks, improving maintainability and performance.
   • Impact: This is a breaking change, but since generics support will already introduce API changes, this is a good opportunity to make this improvement.

3. Reconsideration of panic Handling

   • Problem: The current implementation handles panic and runtime.Goexit, adding complexity to error handling.
   • Proposed Solution: Reevaluate whether panic handling is necessary. For simpler use cases, it might be better to let users handle panic themselves with recover. For advanced scenarios, an opt-in mechanism for panic handling could be provided.

---

Motivation

These changes aim to make singleflight simpler, more performant, and easier to maintain. Generics support already introduces API changes, making this an ideal time to address these improvements.

---

Reference

I have implemented a simplified version of singleflight with asynchronous deletion. In benchmarking tests, this approach showed approximately 2x faster performance compared to the current implementation. You can view the code here:
https://github.com/catatsuy/cache/blob/main/singleflight.go

[pkieltyka]

awesome work, + how about adding support for “context” for cancellation specifically

[ianlancetaylor]

Why would it be a good idea to get rid of the shared flag? It adds complexity but it's there for a reason.

[catatsuy]

Thank you for your feedback!

1. About context support:
   The context can already be passed and handled within the fn function provided to Do. Since the context can be managed inside fn, I believe there’s no need to pass it directly to the singleflight itself. Could you share any specific benefits or use cases that would require context support at the singleflight level?

2. About the shared flag:
   In many example codes, the shared flag is ignored, and I haven’t seen clear use cases where it is explicitly needed. Removing it allows us to eliminate unnecessary locks, which can result in faster performance. If you have a concrete example of when the shared flag is useful, I’d be happy to reconsider its value.

Additionally, I’d like to suggest another improvement:

• Currently, the map is initialized lazily during the first Do call. Instead, I propose adding a New function to initialize the map explicitly when the variable is created. This change would make the API more consistent and potentially reduce runtime overhead during the first call.

Let me know your thoughts! @pkieltyka @ianlancetaylor

[ianlancetaylor]

There is an example of using the shared flag at https://go.googlesource.com/go/+/refs/heads/master/src/net/lookup.go#386. It's using the internal copy of singleflight in the main Go repo, but it's the same code.

[pkieltyka]

@catatsuy maybe I'm missing something, here https://github.com/catatsuy/cache/blob/main/singleflight.go#L51 there is no context, how would you use context with your library?

[catatsuy]

Thank you for pointing that out! You're right, my current implementation at the link you provided doesn’t explicitly support context. However, my point is that context can already be handled within the fn function passed to Do. For example:

ctx, cancel := context.WithTimeout(context.Background(), time.Second)
defer cancel()

sf := cache.NewSingleflightGroup[string]()

result, err := sf.Do("key", func() (string, error) {
    // Use context here
    select {
    case <-ctx.Done():
        return "", ctx.Err()
    default:
        // Do some work
        return "value", nil
    }
})

In this way, context is managed by the caller inside fn. This avoids the need to integrate context directly into the singleflight library itself.

If you have a specific scenario where context at the singleflight level (e.g., passed directly to Do) would provide a clear advantage, I’d love to hear more about it! @pkieltyka

[pkieltyka]

it's just a less flexible option and depends on function closures. personally I'd like to see a context.Context argument prepended to Do (just my opinion).