Panic when call server.Stop() twice

[hunter007]

I stop grpc.Service twice, and panic occurred.

When I read service's unit test codes, I find the question:

func TestServer(t *testing.T) {
	server := getTestServer()
	startTestServer(server)
	stopTestServer(t, server)
}

func startTestServer(server *Server) {
	go func() {
		if err := server.Start(); err != nil && err.Error() != "closed" {
			panic(err)
		}
	}()
}

func stopTestServer(t *testing.T, server *Server) {
	t.Helper()

	assert.NotNil(t, server)
	err := server.Stop()
	assert.Nilf(t, err, "error stopping server")
}

This test is invalid because server hasn't started yet when stopping it.

To Reproduce
Add fmt.Println("Started") before returing,like this:

func (s *Server) Start() error {
	if !atomic.CompareAndSwapUint32(&s.started, 0, 1) {
		return errors.New("a gRPC server can only be started once")
	}
	fmt.Println("Started")
	return s.grpcServer.Serve(s.listener)
}

And add code fmt.Println("Stopped") after err := server.Stop() in func stopTestServer(t *testing.T, server *Server),
like this:

func stopTestServer(t *testing.T, server *Server) {
	t.Helper()

	assert.NotNil(t, server)
	err := server.Stop()
	fmt.Println("Stopped")
	assert.Nilf(t, err, "error stopping server")
}

Run this test again, output:

Running tool: ./go1.20/bin/go test -timeout 30s -run ^TestServer$ github.com/dapr/go-sdk/service/grpc -v

=== RUN   TestServer
Stopped
--- PASS: TestServer (0.00s)
PASS
Started
ok  	github.com/dapr/go-sdk/service/grpc	0.140s

This is an error test. To correcte it, add time.Sleep(time.Second) to last line of startTestServer to ensure server has started:

func startTestServer(server *Server) {
	go func() {
		if err := server.Start(); err != nil && err.Error() != "closed" {
			panic(err)
		}
	}()
	time.Sleep(time.Second)
}

Run this test again, output:

Running tool: ./go1.20/bin/go test -timeout 30s -run ^TestServer$ github.com/dapr/go-sdk/service/grpc -v

=== RUN   TestServer
Started
Stopped
--- PASS: TestServer (1.00s)
PASS
ok  	github.com/dapr/go-sdk/service/grpc	1.453s

Now write another test for stopping server twice:

func TestServer2(t *testing.T) {
	server := getTestServer()
	startTestServer(server)
	stopTestServer(t, server)
	stopTestServer(t, server)
}

Run it:

Running tool: ./go1.20/bin/go test -timeout 30s -run ^TestServer2$ github.com/dapr/go-sdk/service/grpc -v

=== RUN   TestServer2
Started
Stopped
--- FAIL: TestServer2 (1.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x180 pc=0x10284903c]

goroutine 34 [running]:
testing.tRunner.func1.2({0x1029f0860, 0x102e04280})
	/go1.20/src/testing/testing.go:1526 +0x1c8
testing.tRunner.func1()
	/go1.20/src/testing/testing.go:1529 +0x364
panic({0x1029f0860, 0x102e04280})
	/go1.20/src/runtime/panic.go:884 +0x1f4
google.golang.org/grpc.(*Server).Stop(0x0)
	/gopkg/mod/google.golang.org/[email protected]/server.go:1800 +0x2c
github.com/dapr/go-sdk/service/grpc.(*Server).Stop(...)
	/go-sdk/service/grpc/service.go:113
github.com/dapr/go-sdk/service/grpc.stopTestServer(0x140001191e0, 0x1400013c730)
	/go-sdk/service/grpc/service_test.go:65 +0x70
github.com/dapr/go-sdk/service/grpc.TestServer2(0x0?)
	/go-sdk/service/grpc/service_test.go:87 +0xd0

Alter calling Stop() firstly, s.grpcServer = nil. When calling Stop() again, s.grpcServer.Stop() will panic.

How to resolve it ?

If we remove s.grpcServer = nil from Stop(), the error will not occur, but codes from grpc will run every time, that's not a good thing.

We can add stopped field to indicate if the server has stop.

Expected behavior

[hunter007]

/assign