Simpler code to cache value#2808
Conversation
Signed-off-by: David Gageot <david.gageot@docker.com>
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
One confirmed medium-severity data race introduced by the refactor.
| if cached != "" { | ||
| return cached | ||
| } | ||
| func ResetForTests() { |
There was a problem hiding this comment.
[MEDIUM] Data race: unsynchronised write to Get in ResetForTests() vs concurrent reads
ResetForTests() assigns a fresh sync.OnceValue to the package-level variable Get without any synchronisation. Meanwhile, parallel tests (e.g. TestHeaders uses t.Parallel()) or the HTTP client may be reading Get to call it. Writing to a shared variable from one goroutine while another goroutine reads it is a data race under the Go memory model — go test -race will flag it.
The old implementation avoided this by holding mu inside ResetForTests() before clearing cached. The new implementation has no equivalent protection on the variable itself.
Suggested fix: protect the variable with a sync.RWMutex, use sync/atomic (atomic.Value or a pointer), or — simplest for a test-only reset — guard the write and read with a mutex:
var getMu sync.RWMutex
var getOnce = sync.OnceValue(get)
func Get() string {
getMu.RLock()
f := getOnce
getMu.RUnlock()
return f()
}
func ResetForTests() {
getMu.Lock()
getOnce = sync.OnceValue(get)
getMu.Unlock()
}
No description provided.