Skip to content
This repository has been archived by the owner on Dec 23, 2024. It is now read-only.

A possible concurrent access to "idToReplicaset" map #36

Closed
nurzhan-saktaganov opened this issue Aug 9, 2024 · 0 comments · Fixed by #40
Closed

A possible concurrent access to "idToReplicaset" map #36

nurzhan-saktaganov opened this issue Aug 9, 2024 · 0 comments · Fixed by #40
Labels
good first issue Good for newcomers

Comments

@nurzhan-saktaganov
Copy link
Collaborator

nurzhan-saktaganov commented Aug 9, 2024

Case 1
We expose an original map idToReplicaset via the RouterRouteAll method. Nothing prevents a caller from modifying this map.

Case 2
An external topology provider has indirect access to this map via TopologyController. No sync primitive is used to restrict concurrent access (rw).

As a possible solution, we should not modify idToReplicaset at all (make maps immutable by convention). Just create an entirely new map whenever we need to change the current topology. Additionally, we have to guard the idToReplicaset variable using some RWMutex.

@KaymeKaydex I will come up with a Merge Request later if you don't mind.

The basic idea:

type Router struct {
        // ...
    	idToReplicasetMutex sync.RWMutex // guards not the map itself, but the variable idToReplicaset
	idToReplicaset      map[uuid.UUID]*Replicaset
	// ...
}

func (c *controller) AddReplicaset(ctx context.Context, rsInfo ReplicasetInfo, instances []InstanceInfo) error {
	var newIdToReplicaset  map[uuid.UUID]*Replicaset
	var oldIDtoReplicaset map[uuid.UUID]*Replicaset
	
	c.r.idToReplicasetMutex.RLock()
	oldIDtoReplicaset = c.r.idToReplicaset // We guarantee atomic assignment using RW mutex
	c.r.idToReplicasetMutex.RUnlock()
	
	// Here we make a deep copy of oldIDtoReplicaset into newIdToReplicaset
	// TODO
	
	// Here we can modify newIdToReplicaset safely because it was not exposed yet
	// TODO
	
	// Expose new topology, the only place where we use write-lock.
	c.r.idToReplicasetMutex.Lock()
	c.r.idToReplicaset  = newIdToReplicaset
	c.r.idToReplicasetMutex.Unlock()
	
	// Don't do anything with oldIDtoReplicaset (since it was exposed before), the garbage collector will handle it
	
	return nil
}

// RouterRouteAll return map of all replicasets.
func (r *Router) RouterRouteAll() map[uuid.UUID]*Replicaset {
        var idToReplicasetDeepCopy map[uuid.UUID]*Replicaset
        var idToReplicsetRef map[uuid.UUID]*Replicaset
        
        c.r.idToReplicasetMutex.RLock()
	idToReplicsetRef = r.idToReplicaset // We guarantee atomic assignment using RW mutex
	c.r.idToReplicasetMutex.RUnlock()
        
        // We can safely make a deep copy of idToReplicsetRef into idToReplicasetDeepCopy
        // TODO
        
	return idToReplicasetDeepCopy
}

func (r *Router) RouteMapClean() {
 	var idToReplicsetRef map[uuid.UUID]*Replicaset
 	
 	c.r.idToReplicasetMutex.RLock()
	idToReplicsetRef = r.idToReplicaset // We guarantee atomic assignment using RW mutex
	c.r.idToReplicasetMutex.RUnlock()

	r.routeMap = make([]*Replicaset, r.cfg.TotalBucketCount+1)
	r.knownBucketCount.Store(0)

	// We can safely iterate over idToReplicsetRef because it's immutable regarding our convention
	for _, rs := range idToReplicsetRef {
		rs.bucketCount.Store(0)
	}
}
@KaymeKaydex KaymeKaydex added the good first issue Good for newcomers label Aug 9, 2024
nurzhan-saktaganov added a commit that referenced this issue Aug 16, 2024
* make the idToReplicaset map immutable by convention
* instead, we create an entirely new object instead of updating the old one
nurzhan-saktaganov added a commit that referenced this issue Aug 26, 2024
* make the idToReplicaset map immutable by convention
* instead, we create an entirely new object instead of updating the old one
nurzhan-saktaganov added a commit that referenced this issue Aug 26, 2024
* make the idToReplicaset map immutable by convention
* instead, we create an entirely new object instead of updating the old one
* update CHANGELOG.md
nurzhan-saktaganov added a commit that referenced this issue Aug 26, 2024
* make the idToReplicaset map immutable by convention
* instead, we create an entirely new object instead of updating the old one
* update CHANGELOG.md
nurzhan-saktaganov added a commit that referenced this issue Aug 26, 2024
* make the idToReplicaset map immutable by convention
* instead, we create an entirely new object instead of updating the old one
* update CHANGELOG.md
nurzhan-saktaganov added a commit that referenced this issue Aug 26, 2024
* make the idToReplicaset map immutable by convention
	we create an entirely new object instead of updating the old one
* remove proxy object 'struct controller'
* update CHANGELOG.md
nurzhan-saktaganov added a commit that referenced this issue Aug 26, 2024
* make the idToReplicaset map immutable by convention
	we create an entirely new object instead of updating the old one
* remove proxy object 'struct controller'
* update CHANGELOG.md
@nurzhan-saktaganov nurzhan-saktaganov linked a pull request Aug 27, 2024 that will close this issue
KaymeKaydex pushed a commit that referenced this issue Aug 30, 2024
* make the idToReplicaset map immutable by convention
	we create an entirely new object instead of updating the old one
* remove proxy object 'struct controller'
* update CHANGELOG.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants