From 473199a190963973c947808aa9eb6b93349e17d9 Mon Sep 17 00:00:00 2001 From: liuhy Date: Wed, 17 Jun 2026 16:02:15 +0800 Subject: [PATCH 1/2] fix(registry): add lock protection for serviceListeners map access serviceListeners map was read/written without holding the lock in SubscribeURL and UnSubscribe, while the same struct's serviceMappingListeners already had proper lock protection. This could cause data races when concurrent subscribe/unsubscribe operations access the map. - SubscribeURL: use s.lock.Lock() to protect the check-then-act read+write of serviceListeners (write lock needed to prevent TOCTOU race) - UnSubscribe: use s.lock.RLock() for the read access of serviceListeners Co-Authored-By: Claude --- registry/servicediscovery/service_discovery_registry.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/registry/servicediscovery/service_discovery_registry.go b/registry/servicediscovery/service_discovery_registry.go index e01d42154f..6dc30ace54 100644 --- a/registry/servicediscovery/service_discovery_registry.go +++ b/registry/servicediscovery/service_discovery_registry.go @@ -200,7 +200,9 @@ func (s *serviceDiscoveryRegistry) UnSubscribe(url *common.URL, listener registr } // FIXME ServiceNames.String() is not good serviceNamesKey := services.String() + s.lock.RLock() l := s.serviceListeners[serviceNamesKey] + s.lock.RUnlock() if l != nil { l.RemoveListener(url.ServiceKey()) } @@ -362,6 +364,7 @@ func (s *serviceDiscoveryRegistry) SubscribeURL(url *common.URL, notify registry protocol = url.Protocol } protocolServiceKey := url.ServiceKey() + ":" + protocol + s.lock.Lock() listener := s.serviceListeners[serviceNamesKey] if listener == nil { listener = NewServiceInstancesChangedListener(url.GetParam(constant.ApplicationKey, ""), s.url.GetParam(constant.RegistryIdKey, constant.DefaultKey), services) @@ -379,6 +382,7 @@ func (s *serviceDiscoveryRegistry) SubscribeURL(url *common.URL, notify registry } } s.serviceListeners[serviceNamesKey] = listener + s.lock.Unlock() listener.AddListenerAndNotify(protocolServiceKey, notify) event := metricsMetadata.NewMetadataMetricTimeEvent(metricsMetadata.SubscribeServiceRt) From 43c5aea98493c5caecffeb1840006a111922893f Mon Sep 17 00:00:00 2001 From: liuhy Date: Thu, 18 Jun 2026 23:19:22 +0800 Subject: [PATCH 2/2] fix(registry): use defer+lambda for serviceListeners lock to prevent deadlock on panic Wrap lock/unlock in anonymous functions with defer to ensure the lock is always released even if a panic occurs between Lock() and Unlock(), as suggested in PR review. Co-Authored-By: Claude --- .../service_discovery_registry.go | 46 +++++++++++-------- 1 file changed, 26 insertions(+), 20 deletions(-) diff --git a/registry/servicediscovery/service_discovery_registry.go b/registry/servicediscovery/service_discovery_registry.go index 6dc30ace54..8379a7bb94 100644 --- a/registry/servicediscovery/service_discovery_registry.go +++ b/registry/servicediscovery/service_discovery_registry.go @@ -200,9 +200,12 @@ func (s *serviceDiscoveryRegistry) UnSubscribe(url *common.URL, listener registr } // FIXME ServiceNames.String() is not good serviceNamesKey := services.String() - s.lock.RLock() - l := s.serviceListeners[serviceNamesKey] - s.lock.RUnlock() + var l registry.ServiceInstancesChangedListener + func() { + s.lock.RLock() + defer s.lock.RUnlock() + l = s.serviceListeners[serviceNamesKey] + }() if l != nil { l.RemoveListener(url.ServiceKey()) } @@ -364,25 +367,28 @@ func (s *serviceDiscoveryRegistry) SubscribeURL(url *common.URL, notify registry protocol = url.Protocol } protocolServiceKey := url.ServiceKey() + ":" + protocol - s.lock.Lock() - listener := s.serviceListeners[serviceNamesKey] - if listener == nil { - listener = NewServiceInstancesChangedListener(url.GetParam(constant.ApplicationKey, ""), s.url.GetParam(constant.RegistryIdKey, constant.DefaultKey), services) - for _, serviceNameTmp := range services.Values() { - serviceName := serviceNameTmp.(string) - instances := s.serviceDiscovery.GetInstances(serviceName) - logger.Infof("[Registry][ServiceDiscovery] synchronized instance notification on application %s subscription, instance list size %s", serviceName, len(instances)) - err = listener.OnEvent(®istry.ServiceInstancesChangedEvent{ - ServiceName: serviceName, - Instances: instances, - }) - if err != nil { - logger.Warnf("[Registry][ServiceDiscovery] ServiceInstancesChangedListenerImpl handle error, err=%v", err) + var listener registry.ServiceInstancesChangedListener + func() { + s.lock.Lock() + defer s.lock.Unlock() + listener = s.serviceListeners[serviceNamesKey] + if listener == nil { + listener = NewServiceInstancesChangedListener(url.GetParam(constant.ApplicationKey, ""), s.url.GetParam(constant.RegistryIdKey, constant.DefaultKey), services) + for _, serviceNameTmp := range services.Values() { + serviceName := serviceNameTmp.(string) + instances := s.serviceDiscovery.GetInstances(serviceName) + logger.Infof("[Registry][ServiceDiscovery] synchronized instance notification on application %s subscription, instance list size %s", serviceName, len(instances)) + err = listener.OnEvent(®istry.ServiceInstancesChangedEvent{ + ServiceName: serviceName, + Instances: instances, + }) + if err != nil { + logger.Warnf("[Registry][ServiceDiscovery] ServiceInstancesChangedListenerImpl handle error, err=%v", err) + } } } - } - s.serviceListeners[serviceNamesKey] = listener - s.lock.Unlock() + s.serviceListeners[serviceNamesKey] = listener + }() listener.AddListenerAndNotify(protocolServiceKey, notify) event := metricsMetadata.NewMetadataMetricTimeEvent(metricsMetadata.SubscribeServiceRt)