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.

Sign up for GitHub

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

Ensure that calling grpcServer.Stop() only once #412

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Ensure that calling grpcServer.Stop() only once #412

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
38008cc
Ensure that server.Start() has ran before server.Stop()
hunter007 Jun 5, 2023
1bd2e04
Ensure that calling grpcServer.Stop() only once
hunter007 Jun 5, 2023
b5e18cf
more test
hunter007 Jun 5, 2023
ae4c514
Fix error
hunter007 Jun 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 14 additions & 10 deletions service/grpc/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ type Server struct {
authToken string
grpcServer *grpc.Server
started uint32
stopped uint32
}

// Deprecated: Use RegisterActorImplFactoryContext instead.
Expand All @@ -101,26 +102,29 @@ func (s *Server) Start() error {
if !atomic.CompareAndSwapUint32(&s.started, 0, 1) {
return errors.New("a gRPC server can only be started once")
}

return s.grpcServer.Serve(s.listener)
}

// Stop stops the previously-started service.
func (s *Server) Stop() error {
if atomic.LoadUint32(&s.started) == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not started, double call will return nil. Is it an issue that we miss update the start status?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I will correct it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we can use start status to check stop, no need to add a new one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can prevent restart and re-stop using stopped.

If no stopped, when started = 0, we can call Start() which set started=1.

But, call Stop(), if set started = 0, we can restart server? if keep started = 1, we can re-stop?

Copy link
Member

@daixiang0 daixiang0 Jun 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When started is:

  • true, can stop, start/re-start but do nothing
  • false, can start, stop/re-stop but do nothing

Only real stop/start will update started.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I write a test:

func TestReStopServer(t *testing.T) {
	server := getTestServer()
	startTestServer(server)
	time.Sleep(time.Second)

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

	fmt.Print("Re-stop server\n")

	err = server.Stop()
	assert.Nilf(t, err, "error re-stopping server")
}

the test panic:

go test -timeout 30s -run ^TestReStopServer$ github.com/dapr/go-sdk/service/grpc -v

=== RUN   TestReStopServer
Re-stop server
--- FAIL: TestReStopServer (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=0x10475103c]

goroutine 34 [running]:
testing.tRunner.func1.2({0x1048f8860, 0x104d0c280})
	/go1.20/src/testing/testing.go:1526 +0x1c8
testing.tRunner.func1()
	/go1.20/src/testing/testing.go:1529 +0x364
panic({0x1048f8860, 0x104d0c280})
	/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(...)
	/dapr_repos/go-sdk/service/grpc/service.go:112
github.com/dapr/go-sdk/service/grpc.TestReStopServer(0x0?)
	/dapr_repos/go-sdk/service/grpc/service_test.go:79 +0x1c4
testing.tRunner(0x140001171e0, 0x10499b2f0)
	/go1.20/src/testing/testing.go:1576 +0x104
created by testing.(*T).Run
	/go1.20/src/testing/testing.go:1629 +0x370

I made this PR to resolve this issue: #411

return nil
}
s.grpcServer.Stop()
s.grpcServer = nil
return nil
return s.gracefulStop(false)
}

// GrecefulStop stops the previously-started service gracefully.
func (s *Server) GracefulStop() error {
if atomic.LoadUint32(&s.started) == 0 {
return nil
return s.gracefulStop(true)
}

func (s *Server) gracefulStop(graceful bool) error {
if atomic.CompareAndSwapUint32(&s.stopped, 0, 1) {
if graceful {
s.grpcServer.GracefulStop()
} else {
s.grpcServer.Stop()
}
s.grpcServer = nil
}
s.grpcServer.GracefulStop()
s.grpcServer = nil
return nil
}

Expand Down
49 changes: 49 additions & 0 deletions service/grpc/service_test.go
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be using the require package for error evaluations e.g. require.Error / require.NoError

Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ package grpc

import (
"testing"
"time"

"github.com/stretchr/testify/assert"
"google.golang.org/grpc"
Expand Down Expand Up @@ -53,6 +54,7 @@ func startTestServer(server *Server) {
panic(err)
}
}()
time.Sleep(time.Second)
}

func stopTestServer(t *testing.T, server *Server) {
Expand All @@ -62,3 +64,50 @@ func stopTestServer(t *testing.T, server *Server) {
err := server.Stop()
assert.Nilf(t, err, "error stopping server")
}

func TestStartServerTimes(t *testing.T) {
server := getTestServer()

startTestServer(server)
assert.PanicsWithError(t, "a gRPC server can only be started once", func() {
if err := server.Start(); err != nil && err.Error() != "closed" {
panic(err)
}
})

time.Sleep(time.Second)

stopTestServer(t, server)
}

func TestStopServerTimes(t *testing.T) {
server := getTestServer()
startTestServer(server)

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

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

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

func TestStartServerAfterStop(t *testing.T) {
server := getTestServer()
startTestServer(server)
stopTestServer(t, server)
err := server.Start()
assert.NotNil(t, err)
}

func TestGracefulStopServer(t *testing.T) {
server := getTestServer()
startTestServer(server)
assert.NotNil(t, server)
err := server.GracefulStop()
assert.Nilf(t, err, "error stopping server")
}