fishy commented on a change in pull request #2497:
URL: https://github.com/apache/thrift/pull/2497#discussion_r779820700
##########
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:
I think depending on the real world situation "memory of a server grows
unbounded and will eventually OOM" could be more severe than "the shutdown of
the server will take forever and eventually timeout". But this is a fair point:
>I'm also not a fan of the opt-in or documentation approaches. The former,
because the current behaviour is wrong and should be fixed, and the latter
because I don't think end users should worry about an OOM caused by an internal
implementation detail.
But in order to turn it default on, we must make the memory bounded. One way
to do it is to add a background goroutine with `TSimpleServer` that use a
ticker to copy the map over regularly, but when doing so it also need to hold
the lock, and preventing `TSimpleServer` from processing any new requests (so
it's kind of like a stop-the-world gc). The goroutine function should be
something like this:
```go
func (p *TSimpleServer) mapCleanup() {
p.mu.Lock()
defer p.mu.Unlock()
newMap := make(map[string]TTransport, len(p.clients)) // the actual type
of the map pending on which type we decide in the end
for k, v := range p.clients {
newMap[k] = v
}
p.clients = newMap
}
```
It might not be too bad? This copy should be reasonably efficient in go, but
I would like to see some real world impact first before merging it.
--
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]
