fishy commented on a change in pull request #2497:
URL: https://github.com/apache/thrift/pull/2497#discussion_r779774049
##########
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:
Go has a runtime bug that a builtin map never actually shrinks when you
delete from it: https://github.com/golang/go/issues/20135
Which means although the behavior is correct (you won't try to close those
clients that's already disconnected during `Close`), for a long running server
this map will keep growing indefinitely and eventually causing memory issues.
For that reason, a `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.
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. We might need to make some breaking change to expose the
`TSocket`/`TSSLSocket` that's wrapped inside this `TTransport` and use its
`.Addr().String()` as the map key (and ignore any `TTransport` that does not
have a `TSocket`/`TSSLSocket` under the hood).
--
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]
