service/rpccommon: fix shutdown related bugs (#2439)

* service/rpcommon: resolve race between Detach and shutdown

Detach will close DisconnectChan causing the server to initiate
shutdown, there is a race between Detach writing its response to the
client and the shutdown terminating the server process.
If Detach loses the race the response to the Detach request is never
sent to the client and the client will report an EOF error instead.

This change delays the start of the shutdown process until after Detach
has written its response.

Fixes an occasional failure of TestContinue.

* service/rpccommon: ignore listener error when shutting down

Ignore the closed listener error when the server is being shut down in
response to a SIGINT signal.

Fixes #1633

service/rpc1/server.go

@@ -35,11 +35,7 @@ func (s *RPCServer) ProcessPid(arg1 interface{}, pid *int) error {
 }
 
 func (s *RPCServer) Detach(kill bool, ret *int) error {
-	err := s.debugger.Detach(kill)
-	if s.config.DisconnectChan != nil {
-		close(s.config.DisconnectChan)
-	}
-	return err
+	return s.debugger.Detach(kill)
 }
 
 func (s *RPCServer) Restart(arg1 interface{}, arg2 *int) error {

service/rpc2/server.go

@@ -58,12 +58,7 @@ type DetachOut struct {
 
 // Detach detaches the debugger, optionally killing the process.
 func (s *RPCServer) Detach(arg DetachIn, out *DetachOut) error {
-	err := s.debugger.Detach(arg.Kill)
-	if s.config.DisconnectChan != nil {
-		close(s.config.DisconnectChan)
-		s.config.DisconnectChan = nil
-	}
-	return err
+	return s.debugger.Detach(arg.Kill)
 }
 
 type RestartIn struct {

service/rpccommon/server.go

@@ -89,8 +89,8 @@ func NewServer(config *service.Config) *ServerImpl {
 
 // Stop stops the JSON-RPC server.
 func (s *ServerImpl) Stop() error {
+	close(s.stopChan)
 	if s.config.AcceptMulti {
-		close(s.stopChan)
 		s.listener.Close()
 	}
 	kill := s.config.Debugger.AttachPid == 0
@@ -323,6 +323,10 @@ func (s *ServerImpl) serveJSONCodec(conn io.ReadWriteCloser) {
 				s.log.Debugf("-> %T%s error: %q", replyv.Interface(), replyvbytes, errmsg)
 			}
 			s.sendResponse(sending, &req, &resp, replyv.Interface(), codec, errmsg)
+			if req.ServiceMethod == "RPCServer.Detach" && s.config.DisconnectChan != nil {
+				close(s.config.DisconnectChan)
+				s.config.DisconnectChan = nil
+			}
 		} else {
 			if logflags.RPC() {
 				argvbytes, _ := json.Marshal(argv.Interface())

service/test/integration2_test.go

@@ -2318,3 +2318,34 @@ func TestToggleBreakpointRestart(t *testing.T) {
 		assertNoDuplicateBreakpoints(t, c)
 	})
 }
+
+func TestStopServerWithClosedListener(t *testing.T) {
+	// Checks that the error erturned by listener.Accept() is ignored when we
+	// are trying to shutdown. See issue #1633.
+	if testBackend == "rr" || buildMode == "pie" {
+		t.Skip("N/A")
+	}
+	listener, err := net.Listen("tcp", "localhost:0")
+	assertNoError(err, t, "listener")
+	fixture := protest.BuildFixture("math", 0)
+	server := rpccommon.NewServer(&service.Config{
+		Listener:           listener,
+		AcceptMulti:        false,
+		APIVersion:         2,
+		CheckLocalConnUser: true,
+		DisconnectChan:     make(chan struct{}),
+		ProcessArgs:        []string{fixture.Path},
+		Debugger: debugger.Config{
+			WorkingDir:  ".",
+			Backend:     "default",
+			Foreground:  false,
+			BuildFlags:  "",
+			ExecuteKind: debugger.ExecutingGeneratedFile,
+		},
+	})
+	assertNoError(server.Run(), t, "blah")
+	time.Sleep(1 * time.Second) // let server start
+	server.Stop()
+	listener.Close()
+	time.Sleep(1 * time.Second) // give time to server to panic
+}