fishy commented on a change in pull request #2497:
URL: https://github.com/apache/thrift/pull/2497#discussion_r779792077
##########
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:
Even with the opt-in approach, consider how big a footgun using
`TTransport` as the map key could be, I think it's still better to:
1. Make an breaking API change, add `Addr() net.Addr` to `TTransport`
interface (`TMemoryBuffer.Addr()` should return `nil`, delegate transports like
`TFramedTransport`, `TBufferedTransport`, `THeaderTransport`, etc. should just
call their wrapped `TTransport.Addr()`)
2. We use `TTransport.Addr().String()` as the map key, and don't put the
`TTransport` into the map if `Addr()` returns `nil` (this should only happen
when we use `TMemoryBuffer` under the hood, I think, which should be fine)
@dcelasun what do you think?
Another alternative is to add something like `TTransport.String()` to the
interface, but the behavior of that would be more undefined.
--
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]
