Fix the potential race condition of InitializeSubnetService #879
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #879 +/- ##
==========================================
- Coverage 70.61% 70.60% -0.01%
==========================================
Files 95 95
Lines 15094 15110 +16
==========================================
+ Hits 10658 10669 +11
- Misses 3710 3715 +5
Partials 726 726
|
zhengxiexie
force-pushed
the
topic/zhengxie/main/race_condition
branch
from
2534c5b
to
076af3a
Compare
It is possible that an error occurs when trying to send to the already closed `fatalErrors` channel, resulting in a panic.
zhengxiexie
force-pushed
the
topic/zhengxie/main/race_condition
branch
from
076af3a
to
8fba48c
Compare
| go.work.sum | ||
| .tool-versions |
| @@ -65,20 +65,39 @@ func InitializeSubnetService(service common.Service) (*SubnetService, error) { | |||
| }, | |||
| } | |||
|
|
|||
| // Use sync.Once to ensure channel is closed only once | |||
There was a problem hiding this comment.
I would prefer to change like this,
func InitializeSubnetService(service common.Service) (*SubnetService, error) {
wg := sync.WaitGroup{}
fatalErrors := make(chan error, 1)
defer close(fatalErrors)
subnetService := &SubnetService{
Service: service,
SubnetStore: &SubnetStore{
ResourceStore: common.ResourceStore{
Indexer: cache.NewIndexer(keyFunc, cache.Indexers{
common.TagScopeSubnetCRUID: subnetIndexFunc,
common.TagScopeSubnetSetCRUID: subnetSetIndexFunc,
common.TagScopeVMNamespace: subnetIndexVMNamespaceFunc,
common.TagScopeNamespace: subnetIndexNamespaceFunc,
}),
BindingType: model.VpcSubnetBindingType(),
},
},
}
wg.Add(1)
go subnetService.InitializeResourceStore(&wg, fatalErrors, ResourceTypeSubnet, nil, subnetService.SubnetStore)
wg.Wait()
if len(fatalErrors) > 0 {
err := <-fatalErrors
return subnetService, err
}
return subnetService, nil
}
There was a problem hiding this comment.
https://github.com/vmware-tanzu/nsx-operator/blob/main/pkg/nsx/services/securitypolicy/firewall.go#L116-L116
The initial reason is to return as early as possible if there are multiple resources.
You should consider closing channel fatalErrors in this way.
There was a problem hiding this comment.
fatalErrors is closed in "defer" after it is declared in my example.
Even if there are multiple resources, we still need to wait until all sub-tasks are done rather than leave them out of control, otherwise it may lead to unexpected issues (e.g., a zombie routine).
There was a problem hiding this comment.
Yes, this version has no zombie routine.
|
/e2e |
2 similar comments
|
/e2e |
|
/e2e |
It is possible that an error occurs when trying to send to the already closed
fatalErrorschannel, resulting in a panic.