fishy commented on a change in pull request #2497:
URL: https://github.com/apache/thrift/pull/2497#discussion_r790974784
##########
File path: lib/go/thrift/simple_server.go
##########
@@ -234,7 +252,14 @@ func (p *TSimpleServer) Stop() error {
}
atomic.StoreInt32(&p.closed, 1)
p.serverTransport.Interrupt()
+
+ if ServerCloseTimeout > 0 {
+ <-time.After(ServerCloseTimeout)
+ close(p.stopChan)
+ }
Review comment:
This means that in the scenario that the server has a socket timeout of
5s, and `ServerCloseTimeout` of 1min, all the clients should be already closed
after 5s, but we still block for 1min in `TSimpleServer.Stop`.
this should be done in a separated goroutine instead, something like this:
```go
if ServerCloseTimeout > 0 {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
go func(ctx context.Context) {
timer := time.NewTimer(ServerCloseTimeout)
defer timer.Stop()
// whichever comes first
select {
case <- ctx.Done():
case <- timer.C:
}
close(p.stopChan)
p.stopChan = make(chan struct{}) // and remove this from line 262
}(ctx)
}
```
please also add a test case for this scenario.
##########
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:
Please add detailed comment for this global variable.
--
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]
