fishy commented on a change in pull request #2497:
URL: https://github.com/apache/thrift/pull/2497#discussion_r780645928
##########
File path: lib/go/thrift/simple_server_test.go
##########
@@ -154,3 +158,50 @@ 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.Errorf("error when listen")
+ }
+ 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 {
+ // t.Errorf("error accept connection")
Review comment:
just remove this line instead of comment it out.
##########
File path: lib/go/thrift/simple_server_test.go
##########
@@ -154,3 +158,50 @@ 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.Errorf("error when listen")
Review comment:
this should be `t.Fatal` instead of `t.Errorf`. As if we cannot get a
local port to listen none of the rest of this test have any meaning.
##########
File path: lib/go/thrift/simple_server_test.go
##########
@@ -154,3 +158,50 @@ 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.Errorf("error when listen")
+ }
+ 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 {
+ // t.Errorf("error accept connection")
+ return nil, err
+ }
+ return NewTSocketFromConnTimeout(conn, 0), nil
+ },
+ CloseFunc: func() error {
+ return nil
+ },
+ InterruptFunc: func() error {
+ return ln.Close()
+ },
+ }
+
+ serv := NewTSimpleServer2(proc, trans)
+ go serv.Serve()
+
+ port := strings.Split(ln.Addr().String(), ":")[1]
+ netConn, err := net.Dial("tcp", "localhost:"+port)
Review comment:
isn't `net.Dial("tcp", ln.Addr().String())` sufficient? why do we need
to do string split to get the port out then combine that back to the full
address again?
##########
File path: lib/go/thrift/simple_server.go
##########
@@ -234,7 +250,9 @@ func (p *TSimpleServer) Stop() error {
}
atomic.StoreInt32(&p.closed, 1)
p.serverTransport.Interrupt()
+ close(p.stopChan)
p.wg.Wait()
+ p.stopChan = make(chan struct{})
Review comment:
Is the purpose of this line to make sure that calling
`TSimpleServer.Close` twice won't cause any problems? if that's the case I'd
rather just store the return values of
`context.WithCancel(context.Background())` because that handles this kind of
corner cases much more robustly.
##########
File path: lib/go/thrift/simple_server_test.go
##########
@@ -154,3 +158,50 @@ 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.Errorf("error when listen")
+ }
+ 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 {
+ // t.Errorf("error accept connection")
+ return nil, err
+ }
+ return NewTSocketFromConnTimeout(conn, 0), nil
Review comment:
`NewTSocketFromConnTimeout` is already deprecated. please use
`NewTSocketFromConnConf` instead (and since you don't need any timeout here,
you can use `nil` conf)
##########
File path: lib/go/thrift/socket.go
##########
@@ -195,8 +195,9 @@ func (p *TSocket) IsOpen() bool {
// Closes the socket.
func (p *TSocket) Close() error {
// Close the socket
- if p.conn != nil {
- err := p.conn.Close()
+ conn := p.conn
Review comment:
I assume the intention of this change is to make it thread safe, but
this does not achieve that (if you run `go test -race` with the change in this
PR it still complains race condition, because we have 2 goroutines calling
`TSocket.Close` concurrently).
https://github.com/apache/thrift/pull/2500 will address this issue.
##########
File path: lib/go/thrift/simple_server_test.go
##########
@@ -154,3 +158,50 @@ 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.Errorf("error when listen")
Review comment:
also I don't see any format verbs used in other `t.Errorf` calls inside
this function, so they can be changed to `t.Error` instead.
##########
File path: lib/go/thrift/simple_server_test.go
##########
@@ -154,3 +158,50 @@ 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.Errorf("error when listen")
+ }
+ 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 {
+ // t.Errorf("error accept connection")
+ return nil, err
+ }
+ return NewTSocketFromConnTimeout(conn, 0), nil
+ },
+ CloseFunc: func() error {
+ return nil
+ },
+ InterruptFunc: func() error {
+ return ln.Close()
+ },
+ }
+
+ serv := NewTSimpleServer2(proc, trans)
+ go serv.Serve()
+
+ port := strings.Split(ln.Addr().String(), ":")[1]
+ netConn, err := net.Dial("tcp", "localhost:"+port)
+ if err != nil || netConn == nil {
+ t.Errorf("error when dial server")
+ }
+ time.Sleep(time.Second)
Review comment:
this means this test takes at least 1 second to finish. currently all
the tests under this directory only takes slightly longer than 1s:
```
$ go test
PASS
ok github.com/apache/thrift/lib/go/thrift 1.379s
```
so a test that takes at least 1s will double the time of running tests here.
please use a shorter sleep time.
also I'm not sure why this sleep is necessary?
##########
File path: lib/go/thrift/simple_server.go
##########
@@ -234,7 +250,9 @@ func (p *TSimpleServer) Stop() error {
}
atomic.StoreInt32(&p.closed, 1)
p.serverTransport.Interrupt()
+ close(p.stopChan)
p.wg.Wait()
+ p.stopChan = make(chan struct{})
Review comment:
in that case this should be done in `Serve` instead.
--
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]
