Ensure that calling grpcServer.Stop() only once #412
Conversation
Signed-off-by: hunter007 <[email protected]>
Signed-off-by: hunter007 <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #412 +/- ##
==========================================
+ Coverage 69.97% 70.37% +0.39%
==========================================
Files 33 33
Lines 2651 2653 +2
==========================================
+ Hits 1855 1867 +12
+ Misses 695 686 -9
+ Partials 101 100 -1
|
Signed-off-by: hunter007 <[email protected]>
| return s.grpcServer.Serve(s.listener) | ||
| } | ||
|
|
||
| // Stop stops the previously-started service. | ||
| func (s *Server) Stop() error { | ||
| if atomic.LoadUint32(&s.started) == 0 { |
There was a problem hiding this comment.
If not started, double call will return nil. Is it an issue that we miss update the start status?
There was a problem hiding this comment.
Yes. I will correct it
There was a problem hiding this comment.
Also, we can use start status to check stop, no need to add a new one.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Signed-off-by: hunter007 <[email protected]>
There was a problem hiding this comment.
I think we should be using the require package for error evaluations e.g. require.Error / require.NoError
Fix #411