fishy commented on a change in pull request #2497:
URL: https://github.com/apache/thrift/pull/2497#discussion_r791356116
##########
File path: lib/go/thrift/simple_server.go
##########
@@ -229,12 +247,29 @@ func (p *TSimpleServer) Serve() error {
func (p *TSimpleServer) Stop() error {
p.mu.Lock()
defer p.mu.Unlock()
+
if atomic.LoadInt32(&p.closed) != 0 {
return nil
}
atomic.StoreInt32(&p.closed, 1)
p.serverTransport.Interrupt()
- p.wg.Wait()
+
+ ctx, cancel := context.WithCancel(context.Background())
+ go func() {
+ defer cancel()
+ p.wg.Wait()
+ }()
+
+ if ServerCloseTimeout > 0 {
+ select {
+ case <-time.After(ServerCloseTimeout):
Review comment:
Say `ServerCloseTimeout` is 1min and all clients closed after 5s, so we
go with the other case, this timer will still occupy the resources for the
whole 1min and only gets recycled after the timer has fired, even though it's
no longer needed after 5s. This is why we should defer `timer.Stop()` so we can
release the resources sooner.
##########
File path: lib/go/thrift/simple_server.go
##########
@@ -47,16 +48,18 @@ var ErrAbandonRequest = errors.New("request abandoned")
//
// If it's changed to <=0, the feature will be disabled.
var ServerConnectivityCheckInterval = time.Millisecond * 5
+var ServerCloseTimeout = time.Duration(0)
Review comment:
and please also add some explanation to the README file similar to
https://github.com/apache/thrift/blob/master/lib/go/README.md#a-note-about-server-handler-implementations.
--
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]
