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/grpc@v1.55.0/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