-
Notifications
You must be signed in to change notification settings - Fork 203
lru: use read/write lock for lru get #216
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Welcome @stbenjam! |
@@ -104,11 +104,12 @@ func (c *Cache) RemoveOldest() { | |||
} | |||
|
|||
func (c *Cache) removeElement(e *list.Element) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the fix needs to be in lru... Get is mutating since it moves things to the front, so I think we need this:
// Get looks up a key's value from the cache.
func (c *Cache) Get(key Key) (value interface{}, ok bool) {
- c.lock.RLock()
- defer c.lock.RUnlock()
+ c.lock.Lock()
+ defer c.lock.Unlock()
return c.cache.Get(key)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦
I didn't catch that... thanks so much! This was driving me crazy.
/lgtm |
`Get` is mutating since it moves an element to the front when it's accessed, using a read-only lock can result in a panic like the below: ``` 2021-08-18T19:57:56.080933580Z E0818 19:57:56.080795 17 runtime.go:78] Observed a panic: &runtime.TypeAssertionError{_interface:(*runtime._type)(0x4ad36a0), concrete:(*runtime._type)(nil), asserted:(*runtime._ type)(0x4769e60), missingMethod:""} (interface conversion: interface {} is nil, not *golang_lru.entry) 2021-08-18T19:57:56.080933580Z goroutine 5333 [running]: 2021-08-18T19:57:56.080933580Z k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/runtime.logPanic(0x4c78f00, 0xc0398de210) 2021-08-18T19:57:56.080933580Z /go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:74 +0x95 2021-08-18T19:57:56.080933580Z k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/runtime.HandleCrash(0x0, 0x0, 0x0) 2021-08-18T19:57:56.080933580Z /go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:48 +0x86 2021-08-18T19:57:56.080933580Z panic(0x4c78f00, 0xc0398de210) 2021-08-18T19:57:56.080933580Z /usr/lib/golang/src/runtime/panic.go:965 +0x1b9 2021-08-18T19:57:56.080933580Z k8s.io/kubernetes/vendor/k8s.io/utils/internal/third_party/forked/golang/golang-lru.(*Cache).removeElement(0xc00012b280, 0xc0013afef0) ``` Co-authored-by: Jordan Liggitt <liggitt@google.com>
@liggitt Sorry fixed a typo in the commit, needs a lgtm again. |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, liggitt, stbenjam The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Get
is mutating since it moves an element to the front when it's accessed, using a read-only lock can result in a panic like the below:Which issue(s) this PR fixes:
Fixes kubernetes/kubernetes#104452