From b144b81017547bc7da6173ef4c448e7050b83158 Mon Sep 17 00:00:00 2001 From: "KIm, JinSan" Date: Tue, 29 Dec 2020 18:34:36 +0900 Subject: [PATCH 1/4] test: grpc_client_test and fix grpc_client `StopForError()` hang bug (#155) --- abci/client/grpc_client_test.go | 93 +++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 abci/client/grpc_client_test.go diff --git a/abci/client/grpc_client_test.go b/abci/client/grpc_client_test.go new file mode 100644 index 000000000..aba6662ec --- /dev/null +++ b/abci/client/grpc_client_test.go @@ -0,0 +1,93 @@ +package abcicli_test + +import ( + "errors" + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + abcicli "github.com/tendermint/tendermint/abci/client" + "github.com/tendermint/tendermint/abci/server" + "github.com/tendermint/tendermint/abci/types" + tmrand "github.com/tendermint/tendermint/libs/rand" + "github.com/tendermint/tendermint/libs/service" +) + +type errorStopper interface { + StopForError(error) +} + +func TestSocketClientStopForErrorDeadlock(t *testing.T) { + c := abcicli.NewGRPCClient(":80", false).(errorStopper) + err := errors.New("foo-tendermint") + + // See Issue https://github.com/tendermint/abci/issues/114 + doneChan := make(chan bool) + go func() { + defer close(doneChan) + c.StopForError(err) + c.StopForError(err) + }() + + select { + case <-doneChan: + case <-time.After(time.Second * 4): + t.Fatalf("Test took too long, potential deadlock still exists") + } +} + +func TestProperSyncCalls(t *testing.T) { + app := slowApp{} + + s, c := setupClientServer(t, app) + defer s.Stop() + defer c.Stop() + + resp := make(chan error, 1) + go func() { + // This is BeginBlockSync unrolled.... + reqres := c.BeginBlockAsync(types.RequestBeginBlock{}) + c.FlushSync() + res := reqres.Response.GetBeginBlock() + require.NotNil(t, res) + resp <- c.Error() + }() + + select { + case <-time.After(time.Second): + require.Fail(t, "No response arrived") + case err, ok := <-resp: + require.True(t, ok, "Must not close channel") + assert.NoError(t, err, "This should return success") + } +} + +func setupClientServer(t *testing.T, app types.Application) ( + service.Service, abcicli.Client) { + // some port between 20k and 30k + port := 20000 + tmrand.Int32()%10000 + addr := fmt.Sprintf("localhost:%d", port) + + s, err := server.NewServer(addr, "grpc", app) + require.NoError(t, err) + err = s.Start() + require.NoError(t, err) + + c := abcicli.NewGRPCClient(addr, true) + err = c.Start() + require.NoError(t, err) + + return s, c +} + +type slowApp struct { + types.BaseApplication +} + +func (slowApp) BeginBlock(req types.RequestBeginBlock) types.ResponseBeginBlock { + time.Sleep(200 * time.Millisecond) + return types.ResponseBeginBlock{} +} From 59e7b7edc45b66b07f3835d90cf08e8a277a0ed0 Mon Sep 17 00:00:00 2001 From: "Kim, JinSan" Date: Thu, 15 Apr 2021 19:15:27 +0900 Subject: [PATCH 2/4] chore: revise imports --- abci/client/grpc_client_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/abci/client/grpc_client_test.go b/abci/client/grpc_client_test.go index aba6662ec..0f0bbd5df 100644 --- a/abci/client/grpc_client_test.go +++ b/abci/client/grpc_client_test.go @@ -9,11 +9,11 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - abcicli "github.com/tendermint/tendermint/abci/client" - "github.com/tendermint/tendermint/abci/server" - "github.com/tendermint/tendermint/abci/types" - tmrand "github.com/tendermint/tendermint/libs/rand" - "github.com/tendermint/tendermint/libs/service" + abcicli "github.com/line/ostracon/abci/client" + "github.com/line/ostracon/abci/server" + "github.com/line/ostracon/abci/types" + ostrand "github.com/line/ostracon/libs/rand" + "github.com/line/ostracon/libs/service" ) type errorStopper interface { @@ -68,7 +68,7 @@ func TestProperSyncCalls(t *testing.T) { func setupClientServer(t *testing.T, app types.Application) ( service.Service, abcicli.Client) { // some port between 20k and 30k - port := 20000 + tmrand.Int32()%10000 + port := 20000 + ostrand.Int32()%10000 addr := fmt.Sprintf("localhost:%d", port) s, err := server.NewServer(addr, "grpc", app) From d77fb48fec338022aa724d1e50bc08a57f870537 Mon Sep 17 00:00:00 2001 From: "Kim, JinSan" Date: Thu, 15 Apr 2021 20:53:41 +0900 Subject: [PATCH 3/4] chore: revise grpc_client_test.go refer to socket_client_test.go --- abci/client/grpc_client_test.go | 73 ++++++++++++++++++++++----------- 1 file changed, 50 insertions(+), 23 deletions(-) diff --git a/abci/client/grpc_client_test.go b/abci/client/grpc_client_test.go index 0f0bbd5df..e6a382f0e 100644 --- a/abci/client/grpc_client_test.go +++ b/abci/client/grpc_client_test.go @@ -1,7 +1,6 @@ package abcicli_test import ( - "errors" "fmt" "testing" "time" @@ -16,43 +15,71 @@ import ( "github.com/line/ostracon/libs/service" ) -type errorStopper interface { - StopForError(error) -} +func TestProperSyncCalls(t *testing.T) { + app := slowApp{} -func TestSocketClientStopForErrorDeadlock(t *testing.T) { - c := abcicli.NewGRPCClient(":80", false).(errorStopper) - err := errors.New("foo-tendermint") + s, c := setupClientServer(t, app) + t.Cleanup(func() { + if err := s.Stop(); err != nil { + t.Error(err) + } + }) + t.Cleanup(func() { + if err := c.Stop(); err != nil { + t.Error(err) + } + }) - // See Issue https://github.com/tendermint/abci/issues/114 - doneChan := make(chan bool) + resp := make(chan error, 1) go func() { - defer close(doneChan) - c.StopForError(err) - c.StopForError(err) + // This is BeginBlockSync unrolled.... + reqres := c.BeginBlockAsync(types.RequestBeginBlock{}) + err := c.FlushSync() + require.NoError(t, err) + res := reqres.Response.GetBeginBlock() + require.NotNil(t, res) + resp <- c.Error() }() select { - case <-doneChan: - case <-time.After(time.Second * 4): - t.Fatalf("Test took too long, potential deadlock still exists") + case <-time.After(time.Second): + require.Fail(t, "No response arrived") + case err, ok := <-resp: + require.True(t, ok, "Must not close channel") + assert.NoError(t, err, "This should return success") } } -func TestProperSyncCalls(t *testing.T) { +func TestHangingSyncCalls(t *testing.T) { app := slowApp{} s, c := setupClientServer(t, app) - defer s.Stop() - defer c.Stop() + t.Cleanup(func() { + if err := s.Stop(); err != nil { + t.Log(err) + } + }) + t.Cleanup(func() { + if err := c.Stop(); err != nil { + t.Log(err) + } + }) resp := make(chan error, 1) go func() { - // This is BeginBlockSync unrolled.... + // Start BeginBlock and flush it reqres := c.BeginBlockAsync(types.RequestBeginBlock{}) - c.FlushSync() - res := reqres.Response.GetBeginBlock() - require.NotNil(t, res) + flush := c.FlushAsync() + // wait 20 ms for all events to travel socket, but + // no response yet from server + time.Sleep(20 * time.Millisecond) + // kill the server, so the connections break + err := s.Stop() + require.NoError(t, err) + + // wait for the response from BeginBlock + reqres.Wait() + flush.Wait() resp <- c.Error() }() @@ -61,7 +88,7 @@ func TestProperSyncCalls(t *testing.T) { require.Fail(t, "No response arrived") case err, ok := <-resp: require.True(t, ok, "Must not close channel") - assert.NoError(t, err, "This should return success") + assert.Error(t, err, "We should get EOF error") } } From babe6af9dd0c9dc5b2bcd9e85c37ce34fb482e88 Mon Sep 17 00:00:00 2001 From: "Kim, JinSan" Date: Thu, 15 Apr 2021 21:12:55 +0900 Subject: [PATCH 4/4] chore: revise test --- abci/client/grpc_client_test.go | 64 ++++++++++++--------------------- 1 file changed, 23 insertions(+), 41 deletions(-) diff --git a/abci/client/grpc_client_test.go b/abci/client/grpc_client_test.go index e6a382f0e..60e4f46f7 100644 --- a/abci/client/grpc_client_test.go +++ b/abci/client/grpc_client_test.go @@ -1,6 +1,7 @@ package abcicli_test import ( + "errors" "fmt" "testing" "time" @@ -15,71 +16,52 @@ import ( "github.com/line/ostracon/libs/service" ) -func TestProperSyncCalls(t *testing.T) { - app := slowApp{} +type errorStopper interface { + StopForError(error) +} - s, c := setupClientServer(t, app) - t.Cleanup(func() { - if err := s.Stop(); err != nil { - t.Error(err) - } - }) - t.Cleanup(func() { - if err := c.Stop(); err != nil { - t.Error(err) - } - }) +func TestSocketClientStopForErrorDeadlock(t *testing.T) { + c := abcicli.NewGRPCClient(":80", false).(errorStopper) + err := errors.New("foo-ostracon") - resp := make(chan error, 1) + // See Issue https://github.com/tendermint/abci/issues/114 + doneChan := make(chan bool) go func() { - // This is BeginBlockSync unrolled.... - reqres := c.BeginBlockAsync(types.RequestBeginBlock{}) - err := c.FlushSync() - require.NoError(t, err) - res := reqres.Response.GetBeginBlock() - require.NotNil(t, res) - resp <- c.Error() + defer close(doneChan) + c.StopForError(err) + c.StopForError(err) }() select { - case <-time.After(time.Second): - require.Fail(t, "No response arrived") - case err, ok := <-resp: - require.True(t, ok, "Must not close channel") - assert.NoError(t, err, "This should return success") + case <-doneChan: + case <-time.After(time.Second * 4): + t.Fatalf("Test took too long, potential deadlock still exists") } } -func TestHangingSyncCalls(t *testing.T) { +func TestProperSyncCalls(t *testing.T) { app := slowApp{} s, c := setupClientServer(t, app) t.Cleanup(func() { if err := s.Stop(); err != nil { - t.Log(err) + t.Error(err) } }) t.Cleanup(func() { if err := c.Stop(); err != nil { - t.Log(err) + t.Error(err) } }) resp := make(chan error, 1) go func() { - // Start BeginBlock and flush it + // This is BeginBlockSync unrolled.... reqres := c.BeginBlockAsync(types.RequestBeginBlock{}) - flush := c.FlushAsync() - // wait 20 ms for all events to travel socket, but - // no response yet from server - time.Sleep(20 * time.Millisecond) - // kill the server, so the connections break - err := s.Stop() + err := c.FlushSync() require.NoError(t, err) - - // wait for the response from BeginBlock - reqres.Wait() - flush.Wait() + res := reqres.Response.GetBeginBlock() + require.NotNil(t, res) resp <- c.Error() }() @@ -88,7 +70,7 @@ func TestHangingSyncCalls(t *testing.T) { require.Fail(t, "No response arrived") case err, ok := <-resp: require.True(t, ok, "Must not close channel") - assert.Error(t, err, "We should get EOF error") + assert.NoError(t, err, "This should return success") } }