service: remove stop method and use contexts (#7292)

internal/proxy/app_conn_test.go

@@ -51,16 +51,15 @@ func TestEcho(t *testing.T) {
 	logger := log.TestingLogger()
 	clientCreator := abciclient.NewRemoteCreator(logger, sockPath, SOCKET, true)
 
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
 	// Start server
 	s := server.NewSocketServer(logger.With("module", "abci-server"), sockPath, kvstore.NewApplication())
-	if err := s.Start(); err != nil {
+	if err := s.Start(ctx); err != nil {
 		t.Fatalf("Error starting socket server: %v", err.Error())
 	}
-	t.Cleanup(func() {
-		if err := s.Stop(); err != nil {
-			t.Error(err)
-		}
-	})
+	t.Cleanup(func() { cancel(); s.Wait() })
 
 	// Start client
 	cli, err := clientCreator(logger.With("module", "abci-client"))
@@ -68,14 +67,13 @@ func TestEcho(t *testing.T) {
 		t.Fatalf("Error creating ABCI client: %v", err.Error())
 	}
 
-	if err := cli.Start(); err != nil {
+	if err := cli.Start(ctx); err != nil {
 		t.Fatalf("Error starting ABCI client: %v", err.Error())
 	}
 
 	proxy := newAppConnTest(cli)
 	t.Log("Connected")
 
-	ctx := context.Background()
 	for i := 0; i < 1000; i++ {
 		_, err = proxy.EchoAsync(ctx, fmt.Sprintf("echo-%v", i))
 		if err != nil {
@@ -99,16 +97,15 @@ func BenchmarkEcho(b *testing.B) {
 	logger := log.TestingLogger()
 	clientCreator := abciclient.NewRemoteCreator(logger, sockPath, SOCKET, true)
 
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
 	// Start server
 	s := server.NewSocketServer(logger.With("module", "abci-server"), sockPath, kvstore.NewApplication())
-	if err := s.Start(); err != nil {
+	if err := s.Start(ctx); err != nil {
 		b.Fatalf("Error starting socket server: %v", err.Error())
 	}
-	b.Cleanup(func() {
-		if err := s.Stop(); err != nil {
-			b.Error(err)
-		}
-	})
+	b.Cleanup(func() { cancel(); s.Wait() })
 
 	// Start client
 	cli, err := clientCreator(logger.With("module", "abci-client"))
@@ -116,7 +113,7 @@ func BenchmarkEcho(b *testing.B) {
 		b.Fatalf("Error creating ABCI client: %v", err.Error())
 	}
 
-	if err := cli.Start(); err != nil {
+	if err := cli.Start(ctx); err != nil {
 		b.Fatalf("Error starting ABCI client: %v", err.Error())
 	}
 
@@ -125,7 +122,6 @@ func BenchmarkEcho(b *testing.B) {
 	echoString := strings.Repeat(" ", 200)
 	b.StartTimer() // Start benchmarking tests
 
-	ctx := context.Background()
 	for i := 0; i < b.N; i++ {
 		_, err = proxy.EchoAsync(ctx, echoString)
 		if err != nil {
@@ -152,16 +148,15 @@ func TestInfo(t *testing.T) {
 	logger := log.TestingLogger()
 	clientCreator := abciclient.NewRemoteCreator(logger, sockPath, SOCKET, true)
 
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
 	// Start server
 	s := server.NewSocketServer(logger.With("module", "abci-server"), sockPath, kvstore.NewApplication())
-	if err := s.Start(); err != nil {
+	if err := s.Start(ctx); err != nil {
 		t.Fatalf("Error starting socket server: %v", err.Error())
 	}
-	t.Cleanup(func() {
-		if err := s.Stop(); err != nil {
-			t.Error(err)
-		}
-	})
+	t.Cleanup(func() { cancel(); s.Wait() })
 
 	// Start client
 	cli, err := clientCreator(logger.With("module", "abci-client"))
@@ -169,7 +164,7 @@ func TestInfo(t *testing.T) {
 		t.Fatalf("Error creating ABCI client: %v", err.Error())
 	}
 
-	if err := cli.Start(); err != nil {
+	if err := cli.Start(ctx); err != nil {
 		t.Fatalf("Error starting ABCI client: %v", err.Error())
 	}
 

internal/proxy/multi_app_conn.go

@@ -1,6 +1,8 @@
 package proxy
 
 import (
+	"context"
+	"errors"
 	"fmt"
 	"os"
 	"syscall"
@@ -51,14 +53,22 @@ type multiAppConn struct {
 	queryConn     AppConnQuery
 	snapshotConn  AppConnSnapshot
 
-	consensusConnClient abciclient.Client
-	mempoolConnClient   abciclient.Client
-	queryConnClient     abciclient.Client
-	snapshotConnClient  abciclient.Client
+	consensusConnClient stoppableClient
+	mempoolConnClient   stoppableClient
+	queryConnClient     stoppableClient
+	snapshotConnClient  stoppableClient
 
 	clientCreator abciclient.Creator
 }
 
+// TODO: this is a totally internal and quasi permanent shim for
+// clients. eventually we can have a single client and have some kind
+// of reasonable lifecycle witout needing an explicit stop method.
+type stoppableClient interface {
+	abciclient.Client
+	Stop() error
+}
+
 // NewMultiAppConn makes all necessary abci connections to the application.
 func NewMultiAppConn(clientCreator abciclient.Creator, logger log.Logger, metrics *Metrics) AppConns {
 	multiAppConn := &multiAppConn{
@@ -85,36 +95,36 @@ func (app *multiAppConn) Snapshot() AppConnSnapshot {
 	return app.snapshotConn
 }
 
-func (app *multiAppConn) OnStart() error {
-	c, err := app.abciClientFor(connQuery)
+func (app *multiAppConn) OnStart(ctx context.Context) error {
+	c, err := app.abciClientFor(ctx, connQuery)
 	if err != nil {
 		return err
 	}
-	app.queryConnClient = c
+	app.queryConnClient = c.(stoppableClient)
 	app.queryConn = NewAppConnQuery(c, app.metrics)
 
-	c, err = app.abciClientFor(connSnapshot)
+	c, err = app.abciClientFor(ctx, connSnapshot)
 	if err != nil {
 		app.stopAllClients()
 		return err
 	}
-	app.snapshotConnClient = c
+	app.snapshotConnClient = c.(stoppableClient)
 	app.snapshotConn = NewAppConnSnapshot(c, app.metrics)
 
-	c, err = app.abciClientFor(connMempool)
+	c, err = app.abciClientFor(ctx, connMempool)
 	if err != nil {
 		app.stopAllClients()
 		return err
 	}
-	app.mempoolConnClient = c
+	app.mempoolConnClient = c.(stoppableClient)
 	app.mempoolConn = NewAppConnMempool(c, app.metrics)
 
-	c, err = app.abciClientFor(connConsensus)
+	c, err = app.abciClientFor(ctx, connConsensus)
 	if err != nil {
 		app.stopAllClients()
 		return err
 	}
-	app.consensusConnClient = c
+	app.consensusConnClient = c.(stoppableClient)
 	app.consensusConn = NewAppConnConsensus(c, app.metrics)
 
 	// Kill Tendermint if the ABCI application crashes.
@@ -160,34 +170,42 @@ func (app *multiAppConn) killTMOnClientError() {
 func (app *multiAppConn) stopAllClients() {
 	if app.consensusConnClient != nil {
 		if err := app.consensusConnClient.Stop(); err != nil {
-			app.Logger.Error("error while stopping consensus client", "error", err)
+			if !errors.Is(err, service.ErrAlreadyStopped) {
+				app.Logger.Error("error while stopping consensus client", "error", err)
+			}
 		}
 	}
 	if app.mempoolConnClient != nil {
 		if err := app.mempoolConnClient.Stop(); err != nil {
-			app.Logger.Error("error while stopping mempool client", "error", err)
+			if !errors.Is(err, service.ErrAlreadyStopped) {
+				app.Logger.Error("error while stopping mempool client", "error", err)
+			}
 		}
 	}
 	if app.queryConnClient != nil {
 		if err := app.queryConnClient.Stop(); err != nil {
-			app.Logger.Error("error while stopping query client", "error", err)
+			if !errors.Is(err, service.ErrAlreadyStopped) {
+				app.Logger.Error("error while stopping query client", "error", err)
+			}
 		}
 	}
 	if app.snapshotConnClient != nil {
 		if err := app.snapshotConnClient.Stop(); err != nil {
-			app.Logger.Error("error while stopping snapshot client", "error", err)
+			if !errors.Is(err, service.ErrAlreadyStopped) {
+				app.Logger.Error("error while stopping snapshot client", "error", err)
+			}
 		}
 	}
 }
 
-func (app *multiAppConn) abciClientFor(conn string) (abciclient.Client, error) {
+func (app *multiAppConn) abciClientFor(ctx context.Context, conn string) (abciclient.Client, error) {
 	c, err := app.clientCreator(app.Logger.With(
 		"module", "abci-client",
 		"connection", conn))
 	if err != nil {
 		return nil, fmt.Errorf("error creating ABCI client (%s connection): %w", conn, err)
 	}
-	if err := c.Start(); err != nil {
+	if err := c.Start(ctx); err != nil {
 		return nil, fmt.Errorf("error starting ABCI client (%s connection): %w", conn, err)
 	}
 	return c, nil

internal/proxy/multi_app_conn_test.go

@@ -1,6 +1,7 @@
 package proxy
 
 import (
+	"context"
 	"errors"
 	"os"
 	"os/signal"
@@ -17,31 +18,42 @@ import (
 	"github.com/tendermint/tendermint/libs/log"
 )
 
+type noopStoppableClientImpl struct {
+	abciclient.Client
+	count int
+}
+
+func (c *noopStoppableClientImpl) Stop() error { c.count++; return nil }
+
 func TestAppConns_Start_Stop(t *testing.T) {
 	quitCh := make(<-chan struct{})
 
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
 	clientMock := &abcimocks.Client{}
-	clientMock.On("Start").Return(nil).Times(4)
-	clientMock.On("Stop").Return(nil).Times(4)
+	clientMock.On("Start", mock.Anything).Return(nil).Times(4)
 	clientMock.On("Quit").Return(quitCh).Times(4)
+	cl := &noopStoppableClientImpl{Client: clientMock}
 
 	creatorCallCount := 0
 	creator := func(logger log.Logger) (abciclient.Client, error) {
 		creatorCallCount++
-		return clientMock, nil
+		return cl, nil
 	}
 
 	appConns := NewAppConns(creator, log.TestingLogger(), NopMetrics())
 
-	err := appConns.Start()
+	err := appConns.Start(ctx)
 	require.NoError(t, err)
 
 	time.Sleep(100 * time.Millisecond)
 
-	err = appConns.Stop()
-	require.NoError(t, err)
+	cancel()
+	appConns.Wait()
 
 	clientMock.AssertExpectations(t)
+	assert.Equal(t, 4, cl.count)
 	assert.Equal(t, 4, creatorCallCount)
 }
 
@@ -56,31 +68,30 @@ func TestAppConns_Failure(t *testing.T) {
 		}
 	}()
 
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
 	quitCh := make(chan struct{})
 	var recvQuitCh <-chan struct{} // nolint:gosimple
 	recvQuitCh = quitCh
 
 	clientMock := &abcimocks.Client{}
 	clientMock.On("SetLogger", mock.Anything).Return()
-	clientMock.On("Start").Return(nil)
-	clientMock.On("Stop").Return(nil)
+	clientMock.On("Start", mock.Anything).Return(nil)
 
 	clientMock.On("Quit").Return(recvQuitCh)
 	clientMock.On("Error").Return(errors.New("EOF")).Once()
+	cl := &noopStoppableClientImpl{Client: clientMock}
 
 	creator := func(log.Logger) (abciclient.Client, error) {
-		return clientMock, nil
+		return cl, nil
 	}
 
 	appConns := NewAppConns(creator, log.TestingLogger(), NopMetrics())
 
-	err := appConns.Start()
+	err := appConns.Start(ctx)
 	require.NoError(t, err)
-	t.Cleanup(func() {
-		if err := appConns.Stop(); err != nil {
-			t.Error(err)
-		}
-	})
+	t.Cleanup(func() { cancel(); appConns.Wait() })
 
 	// simulate failure
 	close(quitCh)