pr comments

sreya · sreya · commit e11e2d216fe2 · 2024-10-16T18:35:57.000Z
diff --git a/coderd/cryptokeys/cache.go b/coderd/cryptokeys/cache.go
@@ -101,16 +101,18 @@ func WithCacheClock(clock quartz.Clock) CacheOption {
 	}
 }
 
-// NewSigningCache instantiates a cache. Close should be called to
-// release resources associated with its internal timer.
-func NewSigningCache(ctx context.Context, logger slog.Logger, fetcher Fetcher, feature codersdk.CryptoKeyFeature, opts ...func(*cache)) (SigningKeycache, error) {
+// NewSigningCache instantiates a cache. Close should be called to release resources
+// associated with its internal timer.
+func NewSigningCache(ctx context.Context, logger slog.Logger, fetcher Fetcher,
+	feature codersdk.CryptoKeyFeature, opts ...func(*cache)) (SigningKeycache, error) {
 	if !isSigningKeyFeature(feature) {
 		return nil, xerrors.Errorf("invalid feature: %s", feature)
 	}
 	return newCache(ctx, logger, fetcher, feature, opts...)
 }
 
-func NewEncryptionCache(ctx context.Context, logger slog.Logger, fetcher Fetcher, feature codersdk.CryptoKeyFeature, opts ...func(*cache)) (EncryptionKeycache, error) {
+func NewEncryptionCache(ctx context.Context, logger slog.Logger, fetcher Fetcher,
+	feature codersdk.CryptoKeyFeature, opts ...func(*cache)) (EncryptionKeycache, error) {
 	if !isEncryptionKeyFeature(feature) {
 		return nil, xerrors.Errorf("invalid feature: %s", feature)
 	}
@@ -288,23 +290,21 @@ func checkKey(key codersdk.CryptoKey, sequence int32, now time.Time) (string, []
 func (c *cache) refresh() {
 	now := c.clock.Now("CryptoKeyCache", "refresh")
 	c.mu.Lock()
+	defer c.mu.Unlock()
 
 	if c.closed {
-		c.mu.Unlock()
 		return
 	}
 
 	// If something's already fetching, we don't need to do anything.
 	if c.fetching {
-		c.mu.Unlock()
 		return
 	}
 
 	// There's a window we must account for where the timer fires while a fetch
 	// is ongoing but prior to the timer getting reset. In this case we want to
 	// avoid double fetching.
 	if now.Sub(c.lastFetch) < refreshInterval {
-		c.mu.Unlock()
 		return
 	}
 
@@ -317,8 +317,8 @@ func (c *cache) refresh() {
 		return
 	}
 
+	// We don't defer an unlock here due to the deferred unlock at the top of the function.
 	c.mu.Lock()
-	defer c.mu.Unlock()
 
 	c.lastFetch = c.clock.Now()
 	c.refresher.Reset(refreshInterval)

Original file line number	Diff line number	Diff line change
`@@ -101,16 +101,18 @@ func WithCacheClock(clock quartz.Clock) CacheOption {`
`101`	`101`	`}`
`102`	`102`	`}`
`103`	`103`
`104`		`-// NewSigningCache instantiates a cache. Close should be called to`
`105`		`-// release resources associated with its internal timer.`
`106`		`-func NewSigningCache(ctx context.Context, logger slog.Logger, fetcher Fetcher, feature codersdk.CryptoKeyFeature, opts ...func(*cache)) (SigningKeycache, error) {`
	`104`	`+// NewSigningCache instantiates a cache. Close should be called to release resources`
	`105`	`+// associated with its internal timer.`
	`106`	`+func NewSigningCache(ctx context.Context, logger slog.Logger, fetcher Fetcher,`
	`107`	`+ feature codersdk.CryptoKeyFeature, opts ...func(*cache)) (SigningKeycache, error) {`
`107`	`108`	`if !isSigningKeyFeature(feature) {`
`108`	`109`	`return nil, xerrors.Errorf("invalid feature: %s", feature)`
`109`	`110`	`}`
`110`	`111`	`return newCache(ctx, logger, fetcher, feature, opts...)`
`111`	`112`	`}`
`112`	`113`
`113`		`-func NewEncryptionCache(ctx context.Context, logger slog.Logger, fetcher Fetcher, feature codersdk.CryptoKeyFeature, opts ...func(*cache)) (EncryptionKeycache, error) {`
	`114`	`+func NewEncryptionCache(ctx context.Context, logger slog.Logger, fetcher Fetcher,`
	`115`	`+ feature codersdk.CryptoKeyFeature, opts ...func(*cache)) (EncryptionKeycache, error) {`
`114`	`116`	`if !isEncryptionKeyFeature(feature) {`
`115`	`117`	`return nil, xerrors.Errorf("invalid feature: %s", feature)`
`116`	`118`	`}`
`@@ -288,23 +290,21 @@ func checkKey(key codersdk.CryptoKey, sequence int32, now time.Time) (string, []`
`288`	`290`	`func (c *cache) refresh() {`
`289`	`291`	`now := c.clock.Now("CryptoKeyCache", "refresh")`
`290`	`292`	`c.mu.Lock()`
	`293`	`+ defer c.mu.Unlock()`
`291`	`294`
`292`	`295`	`if c.closed {`
`293`		`- c.mu.Unlock()`
`294`	`296`	`return`
`295`	`297`	`}`
`296`	`298`
`297`	`299`	`// If something's already fetching, we don't need to do anything.`
`298`	`300`	`if c.fetching {`
`299`		`- c.mu.Unlock()`
`300`	`301`	`return`
`301`	`302`	`}`
`302`	`303`
`303`	`304`	`// There's a window we must account for where the timer fires while a fetch`
`304`	`305`	`// is ongoing but prior to the timer getting reset. In this case we want to`
`305`	`306`	`// avoid double fetching.`
`306`	`307`	`if now.Sub(c.lastFetch) < refreshInterval {`
`307`		`- c.mu.Unlock()`
`308`	`308`	`return`
`309`	`309`	`}`
`310`	`310`
`@@ -317,8 +317,8 @@ func (c *cache) refresh() {`
`317`	`317`	`return`
`318`	`318`	`}`
`319`	`319`
	`320`	`+ // We don't defer an unlock here due to the deferred unlock at the top of the function.`
`320`	`321`	`c.mu.Lock()`
`321`		`- defer c.mu.Unlock()`
`322`	`322`
`323`	`323`	`c.lastFetch = c.clock.Now()`
`324`	`324`	`c.refresher.Reset(refreshInterval)`