dcelasun commented on a change in pull request #2497:
URL: https://github.com/apache/thrift/pull/2497#discussion_r779783347
##########
File path: lib/go/thrift/simple_server.go
##########
@@ -193,7 +195,13 @@ func (p *TSimpleServer) innerAccept() (int32, error) {
}
if client != nil {
p.wg.Add(1)
+ p.clients[client] = struct{}{}
go func() {
+ defer func() {
+ p.mu.Lock()
+ defer p.mu.Unlock()
+ delete(p.clients, client)
Review comment:
> `sync.Map` will probably actually be better, as I understood it
`sync.Map` actually occasionally replaces the whole map under the hood so it
actually can be garbage collected.
Looking at the implementation, I don't think `sync.Map` would help either.
The underlying "dirty" map is only replaced after a number of `Load()` misses,
which would never happen here as we only read once during `Close()`.
> Also I'm not sure that you can actually use a TTransport as the map key
(for either builtin map or sync.Map), when you try to use a type that cannot be
used as the map key it will cause a runtime panic instead of some compile time
error.
I'm not seeing any Go test failures on Travis (though I didn't check
thoroughly) so I assume all implementations of `TTransport` are hashable.
--
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]
