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

Add non locking versions of Get, Set and Has #104

Open
Lochlanna opened this issue Dec 11, 2021 · 5 comments
Open

Add non locking versions of Get, Set and Has #104

Lochlanna opened this issue Dec 11, 2021 · 5 comments

Comments

@Lochlanna
Copy link

I have personally had a requirement to do multiple consecutive operations using the same key with the requirement that nothing can change from start to end. To enable this, I propose explicitly named versions of the Get, Set and Has functions which do not take out locks on the shard. Instead, it is up to the user to use the GetShard method and lock/unlock it correctly.

The three functions would look like

func (m ConcurrentMap) UnlockedSet(key string, value interface{})
func (m ConcurrentMap) UnlockedGet(key string) (interface{}, bool)
func (m ConcurrentMap) UnlockedHas(key string) bool

I'm not sure about the naming. Maybe UnsafeGet, UnsafeSet and UnsafeHas is better?

Here's an example of how I think this could be useful

shard := conMap.GetShard(key)
shard.RLock()
pointerToMyStruct, ok := conMap.UnlockedGet(key)
// Some other thread might be trying to write to this same key but, we still have a lock!
if ok {
    pointerToMyStruct.ReadSomeCoolInfo()
}
shard.RUnlock()
// Now the write on the other thread will go-ahead

In my case, I'm developing a cache where entries expire. I want to ensure that if the value has expired when the Get call is made, it's still expired when when ReadSomeCoolInfo call is made. If another thread were allowed to refresh the value between the two functions, it would cause me to return the wrong information.

The alternative to these functions would be to expose items on the shards to other packages and let the developer dig in and do it all manually.

@orcaman
Copy link
Owner

orcaman commented Dec 15, 2021

Hi! First of all, I like your username. It's like a hardcore scotch distillery ❤️

Your proposal makes a lot of sense. My only concern is that I feel that 99% of users that end up using this repo are just looking for a way to set and get stuff. So any additional public APIs will make the library less user-friendly.

This is why I think exposing items on the shards to other packages makes more sense.

WDYT?

@Lochlanna
Copy link
Author

Thank you!

Keeping the public API as simple as possible does make sense. Exposing the underlying members can sometimes be just as confusing as a complicated API to inexperienced developers.

In this case, however, the items member is protected to some degree by the fact that users have to go out of their way to get the shard first. This is outside of the get/set basic usage enough that the average user probably won't go down this path and access items mistakenly.

Just exposing items makes it a very small PR too 😁

@orcaman
Copy link
Owner

orcaman commented Dec 15, 2021

Maybe if we called it InternalItems // here be dragons or something like that?

@Lochlanna
Copy link
Author

Yeah, I think that's good!
Shall I make the change and open a PR?

@biryukovmaxim
Copy link

biryukovmaxim commented Sep 14, 2022

@orcaman @Lochlanna
Guys, I have another simple idea, we can add Get Callback to Get method like that:

// GetCb is a callback executed in a map.Get() call, while RLock is held
type GetCb[V any] func(v V, exists bool)

// Get retrieves an element from map under given key.
func (m ConcurrentMap[V]) Get(key string, cbs ...GetCb[V]) (V, bool) {
	// Get shard
	shard := m.GetShard(key)
	shard.RLock()
	// Get item from shard.
	val, ok := shard.items[key]
	for _, cb := range cbs {
		cb(val, ok)
	}
	shard.RUnlock()
	return val, ok
}

The most important thing is No Extra public methods, signature compatibility

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

No branches or pull requests

3 participants