Merge pull request #3123 from WulixuanS/fix/genericmanager

Avoid multiple executions after rlock in concurrent scenarios
karmada-io · Apr 10, 2023 · a7ae11c · a7ae11c
2 parents 45fcc38 + 8c63c14
commit a7ae11c
Show file tree

Hide file tree

Showing 4 changed files with 24 additions and 18 deletions.
diff --git a/pkg/util/fedinformer/genericmanager/multi-cluster-manager.go b/pkg/util/fedinformer/genericmanager/multi-cluster-manager.go
@@ -85,13 +85,14 @@ func (m *multiClusterInformerManagerImpl) getManager(cluster string) (SingleClus
 }
 
 func (m *multiClusterInformerManagerImpl) ForCluster(cluster string, client dynamic.Interface, defaultResync time.Duration) SingleClusterInformerManager {
+	m.lock.Lock()
+	defer m.lock.Unlock()
+
 	// If informer manager already exist, just return
-	if manager, exist := m.getManager(cluster); exist {
+	if manager, exist := m.managers[cluster]; exist {
 		return manager
 	}
 
-	m.lock.Lock()
-	defer m.lock.Unlock()
 	manager := NewSingleClusterInformerManager(client, defaultResync, m.stopCh)
 	m.managers[cluster] = manager
 	return manager

diff --git a/pkg/util/fedinformer/genericmanager/single-cluster-manager.go b/pkg/util/fedinformer/genericmanager/single-cluster-manager.go
@@ -87,8 +87,11 @@ type singleClusterInformerManagerImpl struct {
 }
 
 func (s *singleClusterInformerManagerImpl) ForResource(resource schema.GroupVersionResource, handler cache.ResourceEventHandler) {
+	s.lock.Lock()
+	defer s.lock.Unlock()
+
 	// if handler already exist, just return, nothing changed.
-	if s.IsHandlerExist(resource, handler) {
+	if s.isHandlerExist(resource, handler) {
 		return
 	}
 
@@ -113,6 +116,10 @@ func (s *singleClusterInformerManagerImpl) IsHandlerExist(resource schema.GroupV
 	s.lock.RLock()
 	defer s.lock.RUnlock()
 
+	return s.isHandlerExist(resource, handler)
+}
+
+func (s *singleClusterInformerManagerImpl) isHandlerExist(resource schema.GroupVersionResource, handler cache.ResourceEventHandler) bool {
 	handlers, exist := s.handlers[resource]
 	if !exist {
 		return false
@@ -132,9 +139,6 @@ func (s *singleClusterInformerManagerImpl) Lister(resource schema.GroupVersionRe
 }
 
 func (s *singleClusterInformerManagerImpl) appendHandler(resource schema.GroupVersionResource, handler cache.ResourceEventHandler) {
-	s.lock.Lock()
-	defer s.lock.Unlock()
-
 	// assume the handler list exist, caller should ensure for that.
 	handlers := s.handlers[resource]
 

diff --git a/pkg/util/fedinformer/typedmanager/multi-cluster-manager.go b/pkg/util/fedinformer/typedmanager/multi-cluster-manager.go
@@ -94,13 +94,14 @@ func (m *multiClusterInformerManagerImpl) getManager(cluster string) (SingleClus
 }
 
 func (m *multiClusterInformerManagerImpl) ForCluster(cluster string, client kubernetes.Interface, defaultResync time.Duration) SingleClusterInformerManager {
+	m.lock.Lock()
+	defer m.lock.Unlock()
+
 	// If informer manager already exist, just return
-	if manager, exist := m.getManager(cluster); exist {
+	if manager, exist := m.managers[cluster]; exist {
 		return manager
 	}
 
-	m.lock.Lock()
-	defer m.lock.Unlock()
 	manager := NewSingleClusterInformerManager(client, defaultResync, m.stopCh, m.transformFuncs)
 	m.managers[cluster] = manager
 	return manager

diff --git a/pkg/util/fedinformer/typedmanager/single-cluster-manager.go b/pkg/util/fedinformer/typedmanager/single-cluster-manager.go
@@ -107,8 +107,11 @@ type singleClusterInformerManagerImpl struct {
 }
 
 func (s *singleClusterInformerManagerImpl) ForResource(resource schema.GroupVersionResource, handler cache.ResourceEventHandler) error {
+	s.lock.Lock()
+	defer s.lock.Unlock()
+
 	// if handler already exist, just return, nothing changed.
-	if s.IsHandlerExist(resource, handler) {
+	if s.isHandlerExist(resource, handler) {
 		return nil
 	}
 
@@ -117,20 +120,16 @@ func (s *singleClusterInformerManagerImpl) ForResource(resource schema.GroupVers
 		return err
 	}
 
-	s.lock.Lock()
 	if _, exist := s.informers[resource]; !exist {
 		s.informers[resource] = struct{}{}
 	}
-	s.lock.Unlock()
 
-	s.lock.RLock()
 	if resourceTransformFunc, ok := s.transformFuncs[resource]; ok && !s.isInformerStarted(resource) {
 		err = resourceInformer.Informer().SetTransform(resourceTransformFunc)
 		if err != nil {
 			return err
 		}
 	}
-	s.lock.RUnlock()
 
 	_, err = resourceInformer.Informer().AddEventHandler(handler)
 	if err != nil {
@@ -159,6 +158,10 @@ func (s *singleClusterInformerManagerImpl) IsHandlerExist(resource schema.GroupV
 	s.lock.RLock()
 	defer s.lock.RUnlock()
 
+	return s.isHandlerExist(resource, handler)
+}
+
+func (s *singleClusterInformerManagerImpl) isHandlerExist(resource schema.GroupVersionResource, handler cache.ResourceEventHandler) bool {
 	handlers, exist := s.handlers[resource]
 	if !exist {
 		return false
@@ -205,9 +208,6 @@ func (s *singleClusterInformerManagerImpl) Lister(resource schema.GroupVersionRe
 }
 
 func (s *singleClusterInformerManagerImpl) appendHandler(resource schema.GroupVersionResource, handler cache.ResourceEventHandler) {
-	s.lock.Lock()
-	defer s.lock.Unlock()
-
 	// assume the handler list exist, caller should ensure for that.
 	handlers := s.handlers[resource]