Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Panic when call server.Stop() twice #411

Open
hunter007 opened this issue Jun 5, 2023 · 1 comment · May be fixed by #412
Open

Panic when call server.Stop() twice #411

hunter007 opened this issue Jun 5, 2023 · 1 comment · May be fixed by #412
Labels
bug Something isn't working

Comments

@hunter007
Copy link
Contributor

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 hunter007 added the bug Something isn't working label Jun 5, 2023
@hunter007 hunter007 linked a pull request Jun 5, 2023 that will close this issue
@hunter007
Copy link
Contributor Author

/assign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant