Preserve context values #6
Conversation
| // The context passed to the fn function is a context that preserves all values | ||
| // from the passed context but is cancelled by the singleflight only when all | ||
| // awaiting caller's contexts are cancelled (no caller is awaiting the result). | ||
| // If there are multiple callers, context passed to one caller does not affect | ||
| // the execution and returned values of others except if the function result is | ||
| // dependent on the context values. |
There was a problem hiding this comment.
This might be regarded as a breaking/behavior change. Wondering if this behavior is desired in the general case, or should this be a separate function (something like DoWithValues)?
There was a problem hiding this comment.
I think that this behaviour is desired in general case, even expected. I would not pollute the package api, just relay on the 0 major version to bump the minor one to reflect the behaviour change.
withoutcancel.go
Outdated
| @@ -0,0 +1,12 @@ | |||
| //go:build go1.21 | |||
There was a problem hiding this comment.
I think it's also common to have either the old (or the new) version to have a _go1xx suffix. I don't recall which one's more common (use the suffix for the old or new), but it's worth considering.
There was a problem hiding this comment.
In this case it looks like needed to avoid double declaration when using different go versions to build.
There was a problem hiding this comment.
We still need the explicit go build directive as the file suffixes only work for the GOOS/GOARCH, so it's mostly a matter of convention.
I can rename the withoutcancel.go to withoutcancel_go121.go (for Go 1.21+) and withoutcancel_backport.go to withoutcancel.go if you consider it more idiomatic :)
There was a problem hiding this comment.
The rename is more explicit/idiomatic, even cosmetic, I am for it.
|
Hey, thank you for the PR. On the first glance, it looks great and complete. I would have to review it a bit more before the merge. I must say that I was looking for a way to nicely preserve values since the 0.1 version. Go 1.21 helps a lot there and the backport is clean. |
|
Also; thank you for writing this module! ❤️ We started using this module in Moby (Docker Engine) 24.0 and up; I must admit that I don't recall how we arrived on your module, but @vvoland may remember; it could've been that we started implementing, and discovered "someone already did this!" 😅 In either case; it's been running on many systems, and as far as I'm aware, no issues so far 🎉 That said, feel free to call out if you need help maintaining the project (e.g. help review pull requests / changes); I'm sure Moby maintainers are happy to help out (feel free to ping any of us in that case 👍) |
|
Ah, it was actually @corhere who found this package and let me know about it in one of my context plumbing PRs: moby/moby#44365 (comment) 😄 |
|
I am so glad to read that you are using this package in Moby. :) There is no big pressure on the maintenance, so far, but a help from the community is always welcome. :) |
Instead of passing a fresh `context.Background()` wrap context passed by the user with a context that ignores cancellation. This allows to drop the original cancellation while preserving context values. If built with Go 1.21+ the stdlib `context.WithoutCancel` is used. Otherwise it fallbacks to an in-tree copy of the withoutCancel. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
vvoland
force-pushed
the
ctx-without-cancel
branch
from
6f0f39d
to
c3e6384
Compare
|
Thank you for contributing, resenje.org/singleflight v0.4.0 is released with this improvement. |
|
No problem, it's nice to contribute back! Thanks for the quick release! |
Instead of passing a fresh
context.Background()wrap context passed by the user with a context that ignores cancellation.This allows to drop the original cancellation while preserving context values.
If built with Go 1.21+ the stdlib
context.WithoutCancelis used.Otherwise it fallbacks to an in-tree copy of the withoutCancel.