util/singleflight: add DoChanContext #12003
Conversation
andrew-d
force-pushed
the
andrew/singleflight-context
branch
from
bde2a05
to
36e0841
Compare
andrew-d
force-pushed
the
andrew/singleflight-context
branch
from
36e0841
to
ef3fca7
Compare
util/singleflight/singleflight.go
Outdated
| } | ||
| // Create a context that is not canceled when the parent context is, | ||
| // but otherwise propagates all values. | ||
| callCtx, callCancel := context.WithCancel(context.WithoutCancel(ctx)) |
There was a problem hiding this comment.
This may or may not be the right semantic, but using context.WithoutCancel(ctx) here instead of context.Background() means that the context passed to fn is the context given to first call to DoChanContext. If there are multiple concurrent calls to DoChanContext with different context values, then that could be a surprising semantic.
I don't have a right answer here, but I feel like I would expect the result of fn to be idempotent regardless of what context values is used. Thus, the root of the context given to fn is context.Background and not ctx.
There was a problem hiding this comment.
Whatever semantic we choose, this needs to be documented.
There was a problem hiding this comment.
I changed to using context.Background and documented it; we might want to change at some point, but IMO it's easier to require explicit value propagation.
| // finished executing after the last caller has returned. | ||
| if c.ctxWaiters.Add(-1) == 0 { | ||
| c.cancel() | ||
| c.wg.Wait() |
There was a problem hiding this comment.
If we have 10 concurrent waiters, I find it odd that 9 return immediately, but then the 10th must wait, but maybe that's the right semantic?
This is a variant of DoChan that supports context propagation, such that the context provided to the inner function will only be canceled when there are no more waiters for a given key. This can be used to deduplicate expensive and cancelable calls among multiple callers safely. Updates #11935 Signed-off-by: Andrew Dunham <[email protected]> Change-Id: Ibe1fb67442a854babbc6924fd8437b02cc9e7bcf
andrew-d
force-pushed
the
andrew/singleflight-context
branch
from
ef3fca7
to
a93cc9e
Compare
|
@dsnet because I'm always a bit scared of heavily-concurrent code... mind taking another look at this before I merge? I think I addressed all your comments and the tests still pass with |
As an alterative to #11935 using #12003. Updates #11935 Signed-off-by: Andrew Dunham <[email protected]> Change-Id: I05f643fe812ceeaec5f266e78e3e529cab3a1ac3
This is a variant of DoChan that supports context propagation, such that the context provided to the inner function will only be canceled when there are no more waiters for a given key. This can be used to deduplicate expensive and cancelable calls among multiple callers safely.
Updates #11935
Change-Id: Ibe1fb67442a854babbc6924fd8437b02cc9e7bcf