fishy commented on a change in pull request #2497:
URL: https://github.com/apache/thrift/pull/2497#discussion_r779786573
##########
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:
Looks like at least a `*TSocket` is comparable:
```
$ cat main.go
package main
import (
"fmt"
"github.com/apache/thrift/lib/go/thrift"
)
func main() {
sock := thrift.NewTSocketConf("localhost:6060", nil)
m := map[thrift.TTransport]struct{}{
sock: {},
}
fmt.Println(m)
}
$ go run main.go
map[0xc000068120:{}]
```
But still, there's no guarantee that `TTransport` returned by
`TServerSocket.Accept()` will always be comparable, and when it's not trying to
use it as a key will cause a runtime panic and it will be hard to detect in
tests. (For example, we have a pending PR to wrap `TServerSocket` to return a
`TTransport` that counts the number of active clients:
https://github.com/reddit/baseplate.go/pull/464)
So if this is really important to you, make it opt-in instead: This map is
only used when people call an exported function from `TSimpleServer` to enable
it (for example, `TSimpleServer.EnableClientCleanup()`, name tbd as I'm bad at
names). And we should note in the function's doc about the implications:
1. The memory used by this map could be unbounded
2. If you enable this map, `TTransport` returned by your `TServerTransport`
must be comparable or panics will happen
--
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]
