meshery: [Server] Race condition in K8s component registration

In the following code, the reference to range value ctx at line 86 generates a race condition because it is a reference in the body of a gorutine.

https://github.com/meshery/meshery/blob/f2b1ef8c7b987175f6e3af8e1d1cee2d0a9f3163/server/models/k8s_components_registration.go#L50-L96

(here a simplified version of the bug)

Bug found by revive

About this issue

  • Original URL
  • State: closed
  • Created 2 years ago
  • Reactions: 2
  • Comments: 22 (12 by maintainers)

Most upvoted comments

I also considered changing the type of ctxRegStatusMap from map[string]RegistrationStatus to sync.Map. It’d be a good practice for this use case (manipulating a map in goroutines) but trading type-safety for thread-safety doesn’t sound that good considering the fact the we don’t play with the same key in multiple goroutines.

Would love to hear any thoughts on this.

@Azanul Btw the race condition was fixed and the ctx variable is properly passed into the go routine’s scope. So the original PR only refactors and makes the code pretty. Will you go ahead and rename the PR to say something like “Refactoring registration function to conform to idiomatic go” About the concern regarding concurrent map access, I think adding a mutex Lock and Unlock while writing on the map in the deferred function is good enough.

@Azanul that’d be great, yes. Actually, @chavacava, did open up a PR. Sadly, the stalebot got to it before a maintainer did. I highly recommend reviving that closed PR.