[narInfoCache] Use sync.Map and simplify IsInBinaryCache

[mikeland73]

Summary

This change simplifies code, eliminates manual locks. It:

• uses sync.Map
• Changes fillNarInfoCache to be fillNarInfoCacheIfNeeded
• Simplifies IsInBinaryCache
• Simplifies goroutine in FillNarInfoCache because we no longer have to check for values that are already set and we don't have to handle error.

How was it tested?

devbox run build

[gcurtis]

If the cache is just a global variable, using a map of sync.Onces might simplify things.

(I haven't tested any of this)

var cache sync.Map

func isInBinaryCache(key string) (bool, error) {
    type inCacheFunc func() (bool, error)
    f, ok := cache.Load(key)
    if !ok {
        f = inCacheFunc(sync.OnceValues(func() (bool, error) { /* check cache */ }))
        f, _ = cache.LoadOrStore(key, f)
    }
    return f.(inCacheFunc)()

[gcurtis]

There could be a race here where multiple goroutines see alreadySet == false before reaching line 93. Or one goroutine might reach line 93 and then another sees status.(bool) == false on line 91.

[mikeland73]

Good call. I think our calling patterns make this unlikely but better to avoid if possible.

I think using defer might solve most race conditions. It's not immune to a success following a failure (but neither is the current code so it would not be a regression)

[mikeland73]

@gcurtis @savil using defer here is functionally identical to what was previously handled in line 119 except now it is handled 100% of the time.

This in conjunction with LoadOrStore in line 125 guarantee that the first result is preserved forever (so a race condition cannot replace an existing value). This fixes an existing race condition where two calls to fillNarInfoCacheIfNeeded could override each other if they had different results.

This fix also removes the race condition where an early false causes another goroutine to return false early.

It partially* fixes multiple goroutines trying to fetch the same result at the same time. In that case only the first result is saved and other goroutines return the same result. Previously, each call to fillNarInfoCacheIfNeeded could return a different result.

• I say "partially" because ideally we don't make 2 http requests, but I think that's not a big deal so unless the fix is easy, I would avoid complicating the code.

[savil]

This fix also removes the race condition where an early false causes another goroutine to return false early.

I don't quite understand this scenario. Care to elaborate?

I say "partially" because ideally we don't make 2 http requests, but I think that's not a big deal so unless the fix is easy, I would avoid complicating the code.

Did you try @gcurtis 's suggestion of using sync.Onces? I think that would address this.

[savil]

@mikeland73 thanks for simplifying and hardening this code!

[savil]

This fix also removes the race condition where an early false causes another goroutine to return false early.

I don't quite understand this scenario. Care to elaborate?

I say "partially" because ideally we don't make 2 http requests, but I think that's not a big deal so unless the fix is easy, I would avoid complicating the code.

Did you try @gcurtis 's suggestion of using sync.Onces? I think that would address this.

[mikeland73]

This fix also removes the race condition where an early false causes another goroutine to return false early.

I don't quite understand this scenario. Care to elaborate?

This was in reference to the first version of this PR (and @gcurtis's comment). In the first version if fillNarInfoCacheIfNeeded got called twice simultaneously, it's possbie the first one would initialize the status to false, and the second one would return false right away, even thought he ultimate value would be true. Using defer which does the cleanup at the end eliminates this case.

Did you try @gcurtis 's suggestion of using sync.Onces? I think that would address this.

I think his solution is better because it guarantees only one call per package. I'll hold off on merging and see if I can get it to work.

[mikeland73]

We could replace this with a single LoadOrStore but that would create throwaway OnceValues function every time.

[mikeland73]

@gcurtis implemented your suggestion. PTAL

[gcurtis]

Suggested change

	f = inCacheFunc(sync.OnceValues(func() (bool, error) {
	return p.fetchNarInfoStatus()
	}))
	f = inCacheFunc(sync.OnceValues(p.fetchNarInfoStatus))