Avoid corrupting internal maps when disco.Disco and auth.CachingCredentialsSource get concurrent calls #35
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
Previously the disco.Disco implementation was not concurrency-safe, but didn't document that as being the case and so some callers tried to use it concurrently. With the previous implementation that would potentially corrupt the internal maps if two concurrent calls try to access them at once. It's relatively straightforward to use a mutex to avoid corrupting the maps, so now we'll do that rather than making safe concurrent use the caller's problem. This intentionally uses the mutex only to protect the integrity of the two maps and _does not_ prevent two concurrent network lookups. This means that it's possible in theory to concurrently run discovery for the same hostname more than once concurrently if both see the cache unpopulated when they first check, in which case it's unspecified which of the two gets commemorated in the cache for future use, but for reasonable server implementations this shouldn't actually matter in practice.
Previously CachingCredentialsSource was not safe for concurrent use, but was not documented as such so callers tried to use it concurrently anyway. It's realtively straightfoward to use a mutex to guard the internal cache map, so we'll do that here to avoid pushing the synchronization responsibility downstream into callers. This mutex is here only to prevent corrupting our map and it intentionally does not prevent concurrent calls to the underlying credentials source. This means that two concurrent callers might sometimes race to populate the same cache entry, in which case it's unspecified which one will "win" and have its result retained for future calls, but one of them definitely will.
brandonc
approved these changes
Jun 13, 2023
|
Thanks for the review, @brandonc! I'm not sure of the process for publishing new releases of this codebase. Is there an automated release process that your team would run here, or is it okay for me to just add a |
|
@apparentlymart There is no automation present, so prepping the changelog and creating a release with a v0.1.1 tag is what I would recommend! |
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.
Both
disco.Discoandauth.CachingCredentialsSourceinternally use maps to represent caches of earlier results, but previously neither of them was making any effort to be concurrency-safe in updating those maps, which meant that concurrent calls could potentially corrupt those maps, or panic as seen in hashicorp/terraform#33333.To avoid pushing synchronization complexity down into callers, we'll guard all accesses of these maps using a
sync.Mutex. These mutexes are used only to ensure integrity of the maps themselves, and intentionally do not prevent concurrent calls to the real operations whose results are being cached. This means that it's possible in principle for two goroutines to race to populate the cache and it's unspecified which one will "win", but regardless of which one wins the maps should still be left in a consistent state for future reads and we should avoid any more concurrent access panics.