fishy commented on a change in pull request #2497:
URL: https://github.com/apache/thrift/pull/2497#discussion_r792925067
##########
File path: lib/go/thrift/simple_server.go
##########
@@ -48,15 +49,26 @@ var ErrAbandonRequest = errors.New("request abandoned")
// If it's changed to <=0, the feature will be disabled.
var ServerConnectivityCheckInterval = time.Millisecond * 5
+// ServerStopTimeout defines max close wait duration used by
Review comment:
nit: "max close wait" -> "max stop wait"
##########
File path: lib/go/thrift/simple_server_test.go
##########
@@ -154,3 +160,130 @@ func
TestNoHangDuringStopFromDanglingLockAcquireDuringAcceptLoop(t *testing.T) {
runtime.Gosched()
serv.Stop()
}
+
+func TestNoHangDuringStopFromClientNoDataSendDuringAcceptLoop(t *testing.T) {
+ ln, err := net.Listen("tcp", "localhost:0")
+
+ if err != nil {
+ t.Fatalf("Failed to listen: %v", err)
+ }
+
+ proc := &mockProcessor{
+ ProcessFunc: func(in, out TProtocol) (bool, TException) {
+ in.ReadMessageBegin(context.Background())
+ return false, nil
+ },
+ }
+
+ trans := &mockServerTransport{
+ ListenFunc: func() error {
+ return nil
+ },
+ AcceptFunc: func() (TTransport, error) {
+ conn, err := ln.Accept()
+ if err != nil {
+ return nil, err
+ }
+
+ return NewTSocketFromConnConf(conn, nil), nil
+ },
+ CloseFunc: func() error {
+ return nil
+ },
+ InterruptFunc: func() error {
+ return ln.Close()
+ },
+ }
+
+ serv := NewTSimpleServer2(proc, trans)
+ go serv.Serve()
+ time.Sleep(networkWaitDuration)
+
+ netConn, err := net.Dial("tcp", ln.Addr().String())
+ if err != nil || netConn == nil {
+ t.Fatal("error when dial server")
+ }
+ time.Sleep(networkWaitDuration)
+
+ expectStopTimeout := 50 * time.Millisecond
+ backupServerStopTimeout := ServerStopTimeout
+ t.Cleanup(func() {
+ ServerStopTimeout = backupServerStopTimeout
+ })
+ ServerStopTimeout = expectStopTimeout
+
+ st := time.Now()
+ err = serv.Stop()
+ if err != nil {
+ t.Errorf("error when stop server:%v", err)
+ }
+
+ if elapsed := time.Since(st); elapsed < expectStopTimeout {
+ t.Errorf("stop cost less time than server stop timeout, server
stop timeout:%v,cost time:%v", ServerStopTimeout, elapsed)
+ }
+}
+
+func TestStopTimeoutWithSocketTimeout(t *testing.T) {
+ ln, err := net.Listen("tcp", "localhost:0")
+
+ if err != nil {
+ t.Fatalf("Failed to listen: %v", err)
+ }
+
+ proc := &mockProcessor{
+ ProcessFunc: func(in, out TProtocol) (bool, TException) {
+ in.ReadMessageBegin(context.Background())
+ return false, nil
+ },
+ }
+
+ conf := &TConfiguration{SocketTimeout: 5 * time.Millisecond}
+ wg := &sync.WaitGroup{}
+ trans := &mockServerTransport{
+ ListenFunc: func() error {
+ return nil
+ },
+ AcceptFunc: func() (TTransport, error) {
+ conn, err := ln.Accept()
+ if err != nil {
+ return nil, err
+ }
+ defer wg.Done()
+ return NewTSocketFromConnConf(conn, conf), nil
+ },
+ CloseFunc: func() error {
+ return nil
+ },
+ InterruptFunc: func() error {
+ return ln.Close()
+ },
+ }
+
+ serv := NewTSimpleServer2(proc, trans)
+ go serv.Serve()
+ time.Sleep(networkWaitDuration)
+
+ wg.Add(1)
+ netConn, err := net.Dial("tcp", ln.Addr().String())
+ if err != nil || netConn == nil {
+ t.Fatal("error when dial server")
+ }
+ wg.Wait()
+
+ expectedStopTimeout := time.Second
Review comment:
semantically this is not the "expected" stop timeout (the actual
expectation is around the socket timeout), so I would rename it to just
`serverStopTimeout`.
##########
File path: lib/go/README.md
##########
@@ -132,3 +132,28 @@ if this interval is set to a value too low (for example,
1ms), it might cause
excessive cpu overhead.
This feature is also only enabled on non-oneway endpoints.
+
+A note about server stop implementations
+========================================
+
+[TSimpleServer.Stop](https://pkg.go.dev/github.com/apache/thrift/lib/go/thrift#TSimpleServer.Stop)
will wait for all client connections to be closed after
+the last received or to be received request to be hanled, so the time spent by
stop
Review comment:
1. typo: `s/hanled/handled/`
2. "after the last received" sounds a bit weird to me.
I would word it like this:
```
When TSimpleServer.Stop is called,
it stops accepting any new client connections,
but will wait for all existing client connections to be closed.
The client connections will only be closed if:
1. The client closes the connection; or
2. The SocketTimeout set on the server is hit.
You can set global variable thrift.ServerStopTimeout early
in your main function to control how long `TSimpleServer.Stop`
would wait:
// code example here
By default (when it's <=0), it waits indefinitely.
When you set it to a positive value (for example, 5 seconds),
TSimpleServer.Stop will close all the remaining client connections
after the timeout.
Please note that this will cause any ongoing requests to fail,
so in general you should set it to a value that's long enough for
your handlers to handle the requests.
```
--
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]
