api: use singleflight for /info endpoint #45847
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thaJeztah
changed the title
thaJeztah
commented
Jun 30, 2023
Comment on lines
69
to
70
| version := httputils.VersionFromContext(ctx) | ||
| if versions.LessThan(version, "1.25") { |
There was a problem hiding this comment.
Hm.. so thinking about this; if info (as returned above) is gonna be shared, and it's a pointer, then I guess the steps below wouldn't really work (as they would now modify all requests?
There was a problem hiding this comment.
Fixed this by using the API version as key for the single-flight
thaJeztah
force-pushed
the
sysinfo_singleflight
branch
2 times, most recently
from
af0f982
to
ececb80
Compare
vvoland
approved these changes
Jul 3, 2023
Prevent potential suggestion when many concurrent requests happen on the /info endpoint. It's worth noting that with this change, requests to the endpoint while another request is still in flight will share the results, hence might be slightly incorrect (for example, the output includes SystemTime, which may now be incorrect). Assuming that under normal circumstances, requests will still happen fast enough to not be shared, this may not be a problem, but we could decide to update specific fields to not be shared. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
sysinfo_singleflight
branch
from
ececb80
to
a1c9a68
Compare
|
Rebased; this now only contains the singleflight changes |
|
For reviewers; this one is easiest to review with "whitespace" hidden in the diff; use this link to hide whitespace in the diff; https://github.com/moby/moby/pull/45847/files?w=1 |
rumpl
approved these changes
Jul 4, 2023
|
Let me bring this one in; we discussed this PR in a maintainers call;
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prevent potential suggestion when many concurrent requests happen on the /info endpoint. It's worth noting that with this change, requests to the endpoint while another request is still in flight will share the results, hence might be slightly incorrect (for example, the output includes SystemTime, which may now be incorrect).
Assuming that under normal circumstances, requests will still happen fast enough to not be shared, this may not be a problem, but we could decide to update specific fields to not be shared.
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)