From 2251b215d6ac848ba0c0864c01fa3317794612a5 Mon Sep 17 00:00:00 2001 From: Michael Berlin Date: Fri, 29 Sep 2017 15:27:40 -0700 Subject: [PATCH] throttler: client unit test: Register the test server before calling gRPC's Serve(). Fixes https://github.com/youtube/vitess/issues/3260. I also clarified in grpc_server.go that we are adhering to this requirement when starting up the binary. --- go/vt/servenv/grpc_server.go | 5 +++++ .../grpcthrottlerclient_test.go | 14 +++++++------- .../grpcthrottlerserver/grpcthrottlerserver.go | 6 +++--- go/vt/wrangler/testlib/throttler_test.go | 2 +- 4 files changed, 16 insertions(+), 11 deletions(-) diff --git a/go/vt/servenv/grpc_server.go b/go/vt/servenv/grpc_server.go index e839195663b..3cb6cdc0bc9 100644 --- a/go/vt/servenv/grpc_server.go +++ b/go/vt/servenv/grpc_server.go @@ -143,6 +143,11 @@ func serveGRPC() { } // and serve on it + // NOTE: Before we call Serve(), all services must have registered themselves + // with "GRPCServer". This is the case because go/vt/servenv/run.go + // runs all OnRun() hooks after createGRPCServer() and before + // serveGRPC(). If this was not the case, the binary would crash with + // the error "grpc: Server.RegisterService after Server.Serve". go GRPCServer.Serve(listener) OnTermSync(func() { diff --git a/go/vt/throttler/grpcthrottlerclient/grpcthrottlerclient_test.go b/go/vt/throttler/grpcthrottlerclient/grpcthrottlerclient_test.go index 2ff70db095d..d2cc5810848 100644 --- a/go/vt/throttler/grpcthrottlerclient/grpcthrottlerclient_test.go +++ b/go/vt/throttler/grpcthrottlerclient/grpcthrottlerclient_test.go @@ -30,9 +30,8 @@ import ( // TestThrottlerServer tests the gRPC implementation using a throttler client // and server. func TestThrottlerServer(t *testing.T) { - s, port := startGRPCServer(t) // Use the global manager which is a singleton. - grpcthrottlerserver.StartServer(s, throttler.GlobalManager) + port := startGRPCServer(t, throttler.GlobalManager) // Create a ThrottlerClient gRPC client to talk to the throttler. client, err := factory(fmt.Sprintf("localhost:%v", port)) @@ -47,9 +46,8 @@ func TestThrottlerServer(t *testing.T) { // TestThrottlerServerPanics tests the panic handling of the gRPC throttler // server implementation. func TestThrottlerServerPanics(t *testing.T) { - s, port := startGRPCServer(t) // For testing the panic handling, use a fake Manager instead. - grpcthrottlerserver.StartServer(s, &throttlerclienttest.FakeManager{}) + port := startGRPCServer(t, &throttlerclienttest.FakeManager{}) // Create a ThrottlerClient gRPC client to talk to the throttler. client, err := factory(fmt.Sprintf("localhost:%v", port)) @@ -61,15 +59,17 @@ func TestThrottlerServerPanics(t *testing.T) { throttlerclienttest.TestSuitePanics(t, client) } -func startGRPCServer(t *testing.T) (*grpc.Server, int) { +func startGRPCServer(t *testing.T, m throttler.Manager) int { // Listen on a random port. listener, err := net.Listen("tcp", ":0") if err != nil { t.Fatalf("Cannot listen: %v", err) } - // Create a gRPC server and listen on the port. s := grpc.NewServer() + grpcthrottlerserver.RegisterServer(s, m) + // Call Serve() after our service has been registered. Otherwise, the test + // will fail with the error "grpc: Server.RegisterService after Server.Serve". go s.Serve(listener) - return s, listener.Addr().(*net.TCPAddr).Port + return listener.Addr().(*net.TCPAddr).Port } diff --git a/go/vt/throttler/grpcthrottlerserver/grpcthrottlerserver.go b/go/vt/throttler/grpcthrottlerserver/grpcthrottlerserver.go index 81c71547112..d6cebc0b69c 100644 --- a/go/vt/throttler/grpcthrottlerserver/grpcthrottlerserver.go +++ b/go/vt/throttler/grpcthrottlerserver/grpcthrottlerserver.go @@ -100,15 +100,15 @@ func (s *Server) ResetConfiguration(_ context.Context, request *throttlerdata.Re }, nil } -// StartServer registers the Server instance with the gRPC server. -func StartServer(s *grpc.Server, m throttler.Manager) { +// RegisterServer registers a new throttler server instance with the gRPC server. +func RegisterServer(s *grpc.Server, m throttler.Manager) { throttlerservice.RegisterThrottlerServer(s, NewServer(m)) } func init() { servenv.OnRun(func() { if servenv.GRPCCheckServiceMap("throttler") { - StartServer(servenv.GRPCServer, throttler.GlobalManager) + RegisterServer(servenv.GRPCServer, throttler.GlobalManager) } }) } diff --git a/go/vt/wrangler/testlib/throttler_test.go b/go/vt/wrangler/testlib/throttler_test.go index 29e27572922..a3968bdb594 100644 --- a/go/vt/wrangler/testlib/throttler_test.go +++ b/go/vt/wrangler/testlib/throttler_test.go @@ -41,8 +41,8 @@ func TestVtctlThrottlerCommands(t *testing.T) { t.Fatalf("Cannot listen: %v", err) } s := grpc.NewServer() + grpcthrottlerserver.RegisterServer(s, throttler.GlobalManager) go s.Serve(listener) - grpcthrottlerserver.StartServer(s, throttler.GlobalManager) addr := fmt.Sprintf("localhost:%v", listener.Addr().(*net.TCPAddr).Port)