dcelasun commented on pull request #2497: URL: https://github.com/apache/thrift/pull/2497#issuecomment-1005011613
Thanks for the PR! This doesn't seem like a good use of `sync.Map`. From [godoc](https://pkg.go.dev/sync#Map): > The Map type is specialized. Most code should use a plain Go map instead, with separate locking or coordination, for better type safety and to make it easier to maintain other invariants along with the map content. > The Map type is optimized for two common use cases: (1) when the entry for a given key is only ever written once but read many times, as in caches that only grow, or (2) when multiple goroutines read, write, and overwrite entries for disjoint sets of keys. In these two cases, use of a Map may significantly reduce lock contention compared to a Go map paired with a separate Mutex or RWMutex. In your case, the map is write-heavy, and reads only happen once during `Close`. It'd be better to replace it with a `map[TTransport]struct{}` and a `sync.Mutex`. It would also give us type safety. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
