Skip to content
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

make GetOrSet and GetAndDelete atomic #107

Merged
merged 1 commit into from
Aug 20, 2023
Merged

Conversation

DoubleDi
Copy link
Contributor

@DoubleDi DoubleDi commented Jun 28, 2023

as it is stated in #97 GetOrSet and GetAndDelete are not atomic.

This PR tries to resolve this problem

I am not sure about the reasons (maybe performance), that the lock was initially made in getWithOpts. Also a good followup question is - do we need the metricsMu at all if this PR will be merged?

Copy link
Contributor

@swithek swithek left a comment

Choose a reason for hiding this comment

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

Thanks for the PR (and sorry for the delay on my part). A couple of comments:

cache.go Outdated Show resolved Hide resolved
cache.go Show resolved Hide resolved
@DoubleDi
Copy link
Contributor Author

DoubleDi commented Jul 17, 2023

@swithek @davseby Hi! Please check the latest version.

Please note, that the GetOrSet func has changed.
The current version, when the loader is defined returns a loaded item from the loader, and only if the loaded item is nil will set the new value.
The new version, when the loader is defined does not search the loader at all if the item is not present in the cache and directly sets the new value

I assume the new logic is better, because when you call GetOrSet you already have a preloaded value, that you pass and you do not want to preload it again internally in the cache

cache.go Outdated Show resolved Hide resolved
@DoubleDi
Copy link
Contributor Author

DoubleDi commented Aug 1, 2023

@davseby @swithek Please check when you have the time

@swithek
Copy link
Contributor

swithek commented Aug 2, 2023

@DoubleDi will do, later today/tomorrow.

Copy link
Contributor

@swithek swithek left a comment

Choose a reason for hiding this comment

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

One comment that needs to be addressed, other than that, everything looks good. @DoubleDi

cache.go Outdated Show resolved Hide resolved
cache.go Outdated Show resolved Hide resolved
@DoubleDi
Copy link
Contributor Author

@swithek when you have time please check

cache.go Outdated Show resolved Hide resolved
@swithek
Copy link
Contributor

swithek commented Aug 20, 2023

Thanks for your contribution @DoubleDi. Merging :)

@swithek swithek merged commit 0a03e09 into jellydator:master Aug 20, 2023
2 checks passed
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.

None yet

3 participants