service/dap: fix race condition between disconnect and continue (#4028)

If a disconnect requests comes in while the runUntilStopAndNotify
goroutine is hitting a breakpoint a nil debugger dereference can happen
at various points.

This change resolves the problem by never resetting the debugger field
to nil, instead a separate flag is set (which mostly just exist to
avoid duplicate logging and to keep tests happy).

Fixes #4006, #4007, #4020

Author: aarzilli

service/dap/server.go

@@ -143,6 +143,8 @@ type Session struct {
 	// to ensure that messages do not get interleaved
 	sendingMu sync.Mutex
 
+	disconnected bool // disconnect was called
+
 	// runningCmd tracks whether the server is running an asynchronous
 	// command that resumes execution, which may not correspond to the actual
 	// running state of the process (e.g. if a command is temporarily interrupted).
@@ -412,6 +414,8 @@ func (s *Session) Close() {
 		s.stopDebugSession(killProcess)
 	} else if s.noDebugProcess != nil {
 		s.stopNoDebugProcess()
+	} else {
+		s.disconnected = true
 	}
 	// The binary is no longer in use by the debugger. It is safe to remove it.
 	if s.binaryToRemove != "" {
@@ -1238,6 +1242,7 @@ func (s *Session) newNoDebugProcess(program string, targetArgs []string, wd stri
 // onDisconnectRequest (run goroutine) and requires holding mu lock.
 func (s *Session) stopNoDebugProcess() {
 	if s.noDebugProcess == nil {
+		s.disconnected = true
 		// We already handled termination or there was never a process
 		return
 	}
@@ -1285,7 +1290,7 @@ func (s *Session) onDisconnectRequest(request *dap.DisconnectRequest) {
 		s.send(&dap.DisconnectResponse{Response: *newResponse(request.Request)})
 		s.send(&dap.TerminatedEvent{Event: *newEvent("terminated")})
 		s.conn.Close()
-		s.debugger = nil
+		s.disconnected = true
 		// The target is left in whatever state it is already in - halted or running.
 		// The users therefore have the flexibility to choose the appropriate state
 		// for their case before disconnecting. This is also desirable in case of
@@ -1308,6 +1313,8 @@ func (s *Session) onDisconnectRequest(request *dap.DisconnectRequest) {
 		err = s.stopDebugSession(killProcess)
 	} else if s.noDebugProcess != nil {
 		s.stopNoDebugProcess()
+	} else {
+		s.disconnected = true
 	}
 	if err != nil {
 		s.sendErrorResponse(request.Request, DisconnectError, "Error while disconnecting", err.Error())
@@ -1327,10 +1334,10 @@ func (s *Session) stopDebugSession(killProcess bool) error {
 	defer func() {
 		// Avoid running stop sequence twice.
 		// It's not fatal, but will result in duplicate logging.
-		s.debugger = nil
+		s.disconnected = true
 		s.changeStateMu.Unlock()
 	}()
-	if s.debugger == nil {
+	if s.debugger == nil || s.disconnected {
 		return nil
 	}
 	var exited error

service/dap/server_test.go

@@ -9,6 +9,7 @@ import (
 	"math"
 	"math/rand"
 	"net"
+	"net/http"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -145,8 +146,8 @@ func verifySessionStopped(t *testing.T, session *Session) {
 		t.Error("session must always have a set connection")
 	}
 	verifyConnStopped(t, session.conn)
-	if session.debugger != nil {
-		t.Error("session should have no pointer to debugger after shutdown")
+	if !session.disconnected {
+		t.Error("session should have disconnected set after shutdown")
 	}
 	if session.binaryToRemove != "" {
 		t.Error("session should have no binary to remove after shutdown")
@@ -234,8 +235,9 @@ func TestSessionStop(t *testing.T) {
 		if binaryToRemoveSet && s.binaryToRemove == "" || !binaryToRemoveSet && s.binaryToRemove != "" {
 			t.Errorf("binaryToRemove: got %s, want set=%v", s.binaryToRemove, binaryToRemoveSet)
 		}
-		if debuggerSet && s.debugger == nil || !debuggerSet && s.debugger != nil {
-			t.Errorf("debugger: got %v, want set=%v", s.debugger, debuggerSet)
+		connected := s.debugger != nil && !s.disconnected
+		if debuggerSet != connected {
+			t.Errorf("debugger: got debugger!=nil: %v disconnected: %v, want set=%v", s.debugger != nil, s.disconnected, debuggerSet)
 		}
 		if disconnectChanSet && s.config.DisconnectChan == nil || !disconnectChanSet && s.config.DisconnectChan != nil {
 			t.Errorf("disconnectChan: got %v, want set=%v", s.config.DisconnectChan, disconnectChanSet)
@@ -7694,6 +7696,74 @@ func TestRedirect(t *testing.T) {
 	})
 }
 
+func TestBreakpointAfterDisconnect(t *testing.T) {
+	fixture := protest.BuildFixture(t, "testnextnethttp", protest.AllNonOptimized)
+
+	cmd := exec.Command(fixture.Path)
+	cmd.Stdout = os.Stdout
+	cmd.Stderr = os.Stderr
+	if err := cmd.Start(); err != nil {
+		t.Fatal(err)
+	}
+
+	var server MultiClientCloseServerMock
+	server.stopped = make(chan struct{})
+	server.impl, server.forceStop = startDAPServer(t, false, server.stopped)
+
+	dbg, err := debugger.New(&debugger.Config{Backend: "default", AttachPid: cmd.Process.Pid}, nil)
+	if err != nil {
+		t.Fatalf("debugger.New: %v", err)
+	}
+
+	server.debugger = dbg
+
+	client := server.acceptNewClient(t)
+
+	client.InitializeRequest()
+	client.ExpectInitializeResponse(t)
+
+	client.SetBreakpointsRequestWithArgs(fixture.Source, []int{16}, nil, nil, map[int]string{16: "{w}"})
+	client.ExpectSetBreakpointsResponse(t)
+
+	client.ContinueRequest(1)
+	client.ExpectContinueResponse(t)
+
+	client.DisconnectRequestWithKillOption(false)
+	client.ExpectOutputEvent(t)
+	client.ExpectDisconnectResponse(t)
+	client.Close()
+
+	time.Sleep(200 * time.Millisecond)
+
+	server.impl.session.conn = &connection{ReadWriteCloser: discard{}} // fake a race condition between onDisconnectRequest and the runUntilStopAndNotify goroutine
+
+	httpClient := &http.Client{Timeout: time.Second}
+
+	_, err = httpClient.Get("http://127.0.0.1:9191/nobp")
+	if err != nil {
+		t.Fatalf("Page request after disconnect failed: %v", err)
+	}
+
+	time.Sleep(200 * time.Millisecond)
+
+	cmd.Process.Kill()
+}
+
+type discard struct {
+}
+
+func (discard) Read([]byte) (int, error) {
+	return 0, nil
+}
+
+func (discard) Close() error {
+	return nil
+}
+
+func (discard) Write(buf []byte) (int, error) {
+	return len(buf), nil
+}
+
 // Helper functions for checking ErrorMessage field values.
 
 func checkErrorMessageId(er *dap.ErrorMessage, id int) bool {