-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add EvictOldest to public API, add OnEviction and OnExpiration for setting callbacks outside of the Config #3
Conversation
cache.go
Outdated
return false | ||
} | ||
|
||
cache.evictions++ |
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.
would this operation need to take place within a mutex?
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.
Good catch, it was previously wrapped with one cause of set
05c7790
to
a6043d8
Compare
a6043d8
to
e5fd14e
Compare
cache.go
Outdated
@@ -259,6 +269,16 @@ func (cache *Cache) SetMaxAge(maxAge time.Duration) error { | |||
return nil | |||
} | |||
|
|||
// OnEviction sets the eviction callback | |||
func (cache *Cache) OnEviction(callback func(key, value interface{})) { | |||
cache.onEviction = callback |
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 don't think this is thread safe.
cache.go
Outdated
|
||
// OnExpiration sets the expiration callback | ||
func (cache *Cache) OnExpiration(callback func(key, value interface{})) { | ||
cache.onExpiration = callback |
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 don't think this is thread safe.
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.
It should be ok based if you set it once and forget, but otherwise I think changing the callbacks isn't thread safe (since they might be set while another goroutine is invoking the old callbacks, and potentially could cause the old callback to be invoked even after OnEviction). Though maybe it's not a big deal that the behaviour isn't 100% correct here though.
@f2prateek I can highlight in comments that it's not threadsafe. I'd imagine it's only to be used during cache setup. |
Actually, the only operations that use those callbacks are already guarded by the mutex so this is a quick change :) |
…tting callbacks outside of the Config
e5fd14e
to
677bf32
Compare
Minimal changes, not breaking the api this time 😁
cc @segmentio/gateway