Introduce sync.Map wrapper with generics support

[giorio94]

sync.Map provides a map-like API that is safe for concurrent use by
multiple goroutines without additional locking or coordination. Its
main drawback, though, is that predates generics support in Go, which
means that all methods operate on keys/values of any type. Hence, it
is not type safe, and requires annoying type conversions.

This PR introduces a thin generic wrapper around it, allowing
consumers to specify the exact type of keys and values. Code comments
from the standard library are copied (and slightly adapted where
necessary) for users' convenience.

Additionally, it converts current usages over to it, and extends the
lock-check.sh linting script to additionally flag usages of sync.Map.

[giorio94]

/test

[giorio94]

Cilium Cluster Mesh upgrade failure looks caused by #29078. Rerunning

[learnitall]

Great idea! I am wondering if we need the MapCmpValues struct in addition to the Map struct here. What was the reasoning behind the split? From my understanding, if the CompareAnd{Swap,Delete} methods were attached to the Map struct instead, I don't think there would be any difference to users, since the corresponding sync.Map methods can already support taking a value with type any.

[giorio94]

Great idea! I am wondering if we need the MapCmpValues struct in addition to the Map struct here. What was the reasoning behind the split? From my understanding, if the CompareAnd{Swap,Delete} methods were attached to the Map struct instead, I don't think there would be any difference to users, since the corresponding sync.Map methods can already support taking a value with type any.

Yeah, I agree that the MapCmpValues struct looks a bit weird, and I don't like its name (but I couldn't come up with anything really better -- MapWithComparableValues could be an alternative but looks long, and ComparableMap sounds misleading -- I'm open to suggestions).

Overall the reasoning behind the split is that the CompareAnd{Swap,Delete} methods require also the values to be comparable (in addition to keys being comparable), and it seemed reasonable to enforce that through the generics constraints. Indeed, while the original sync.Map supports taking a value with type any also for these functions, it would then panic if values are not comparable. OTOH requiring values to be always comparable for all methods looks a no go, as it would limit the usability of the abstraction, preventing for instance to store references to functions.

Another alternative could be simply dropping the MapCmpValues struct (and the associated CompareAnd{Swap,Delete}) methods for the moment, as we are not using them anyway in any part of the code.

WDYT?

[qmonnet]

I only took a brief look at the changes in the agent, focused more on the script. Looks good to me, thanks

[marseel]

Looks good!
I would probably drop MapCmpValues though.

[learnitall]

Great idea! I am wondering if we need the MapCmpValues struct in addition to the Map struct here. What was the reasoning behind the split? From my understanding, if the CompareAnd{Swap,Delete} methods were attached to the Map struct instead, I don't think there would be any difference to users, since the corresponding sync.Map methods can already support taking a value with type any.

Yeah, I agree that the MapCmpValues struct looks a bit weird, and I don't like its name (but I couldn't come up with anything really better -- MapWithComparableValues could be an alternative but looks long, and ComparableMap sounds misleading -- I'm open to suggestions).

Overall the reasoning behind the split is that the CompareAnd{Swap,Delete} methods require also the values to be comparable (in addition to keys being comparable), and it seemed reasonable to enforce that through the generics constraints. Indeed, while the original sync.Map supports taking a value with type any also for these functions, it would then panic if values are not comparable. OTOH requiring values to be always comparable for all methods looks a no go, as it would limit the usability of the abstraction, preventing for instance to store references to functions.

Another alternative could be simply dropping the MapCmpValues struct (and the associated CompareAnd{Swap,Delete}) methods for the moment, as we are not using them anyway in any part of the code.

WDYT?

Ah I didn't realize that it would panic 🤦. I'm on the same page now.

IMO, keeping the extra struct is probably a good idea. We could remove it for now, since no code uses it, but then we may have to reimplement it in the future anyways. Not a dealbreaker thoughl

[giorio94]

IMO, keeping the extra struct is probably a good idea. We could remove it for now, since no code uses it, but then we may have to reimplement it in the future anyways. Not a dealbreaker thoughl

Yeah, I agree. Let's keep it there for the moment, we can always drop it afterwards.