Skip to content

pkg/cdi: stop embedding Cache mutex - #335

Open
thaJeztah wants to merge 1 commit into
cncf-tags:mainfrom
thaJeztah:internalize_locking
Open

pkg/cdi: stop embedding Cache mutex#335
thaJeztah wants to merge 1 commit into
cncf-tags:mainfrom
thaJeztah:internalize_locking

Conversation

@thaJeztah

Copy link
Copy Markdown
Contributor

The cache mutex has been embedded since the initial cache implementation in b730453, unintentionally promoting Lock and Unlock as part of the public Cache API.

Move the mutex to an unexported mu field and add deprecated forwarding methods for compatibility. This keeps existing callers working while making cache locking an implementation detail and allows the synchronization primitive to change independently in the future.

The cache mutex has been embedded since the initial cache implementation in
b730453, unintentionally promoting
`Lock` and `Unlock` as part of the public `Cache` API.

Move the mutex to an unexported `mu` field and add deprecated forwarding
methods for compatibility. This keeps existing callers working while making
cache locking an implementation detail and allows the synchronization
primitive to change independently in the future.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Comment thread pkg/cdi/cache.go
Comment on lines +51 to +63
// Lock locks the cache.
//
// Deprecated: Cache locking is an implementation detail and should not be managed by callers.
func (c *Cache) Lock() {
c.mu.Lock()
}

// Unlock unlocks the cache.
//
// Deprecated: Cache locking is an implementation detail and should not be managed by callers.
func (c *Cache) Unlock() {
c.mu.Unlock()
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how likely it is someone is messing with the mutex; added the deprecated wrappers to keep SemVer compatibility, but perhaps it's something we can still wiggle out of.

cc @klihub @elezar

(was dusting off a branch I had with various changes in this package, and splitting it up into sizeable commits).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant