Repository: go-delve/delve
Pull request #2709 (2021-09-21 05:51:22)

service/dap: initial remote attach (handler support only)
This is part 1 of the remote attach support. For now the logic is not exposed via the server interface and is tested by the unittests by directly setting the debugger in the server. Doesn't yet support accept-multi shutdown or running target on attach.

Updates https://github.com/go-delve/delve/issues/2328

polinasok: @suzmue @hyangah 
hyangah: LGTM
aarzilli: See merge conflicts.

Patch:
diff --git a/service/dap/server.go b/service/dap/server.go
index 53caba4b..ed6b602a 100644
--- a/service/dap/server.go
+++ b/service/dap/server.go
@@ -561,6 +561,8 @@ func (s *Server) handleRequest(request dap.Message) {
 		}()
 		<-resumeRequestLoop
 	//--- Synchronous requests ---
+	// TODO(polina): target might be running when remote attach debug session
+	// is started. Support handling initialize and attach requests while running.
 	case *dap.InitializeRequest:
 		// Required
 		s.onInitializeRequest(request)
@@ -705,7 +707,8 @@ func (s *Server) onInitializeRequest(request *dap.InitializeRequest) {
 		return
 	}
 
-	// TODO(polina): Respond with an error if debug session is in progress?
+	// TODO(polina): Respond with an error if debug session started
+	// with an initialize request is in progress?
 	response := &dap.InitializeResponse{Response: *newResponse(request.Request)}
 	response.Body.SupportsConfigurationDoneRequest = true
 	response.Body.SupportsConditionalBreakpoints = true
@@ -748,6 +751,12 @@ func cleanExeName(name string) string {
 }
 
 func (s *Server) onLaunchRequest(request *dap.LaunchRequest) {
+	if s.debugger != nil {
+		s.sendShowUserErrorResponse(request.Request, FailedToLaunch,
+			"Failed to launch", "debugger already started - use remote attach to connect to a server with an active debug session")
+		return
+	}
+
 	var args = defaultLaunchConfig // narrow copy for initializing non-zero default values
 	if err := unmarshalLaunchAttachArgs(request.Arguments, &args); err != nil {
 		s.sendShowUserErrorResponse(request.Request,
@@ -764,7 +773,7 @@ func (s *Server) onLaunchRequest(request *dap.LaunchRequest) {
 			fmt.Sprintf("invalid debug configuration - unsupported 'mode' attribute %q", mode))
 		return
 	}
-	// TODO(polina): Respond with an error if debug session is in progress?
+
 	program := args.Program
 	if program == "" && mode != "replay" { // Only fail on modes requiring a program
 		s.sendShowUserErrorResponse(request.Request, FailedToLaunch, "Failed to launch",
@@ -951,6 +960,8 @@ func (s *Server) stopNoDebugProcess() {
 // onDisconnectRequest handles the DisconnectRequest. Per the DAP spec,
 // it disconnects the debuggee and signals that the debug adaptor
 // (in our case this TCP server) can be terminated.
+// TODO(polina): differentiate between single- and multi-client
+// server mode when handling requests for debug session shutdown.
 func (s *Server) onDisconnectRequest(request *dap.DisconnectRequest) {
 	defer s.triggerServerStop()
 	s.mu.Lock()
@@ -1021,7 +1032,6 @@ func (s *Server) stopDebugSession(killProcess bool) error {
 		s.logToConsole("Detaching without terminating target processs")
 	}
 	err = s.debugger.Detach(killProcess)
-	s.debugger = nil
 	if err != nil {
 		switch err.(type) {
 		case proc.ErrProcessExited:
@@ -1301,7 +1311,7 @@ func (s *Server) asyncCommandDone(asyncSetupDone chan struct{}) {
 
 // onConfigurationDoneRequest handles 'configurationDone' request.
 // This is an optional request enabled by capability ‘supportsConfigurationDoneRequest’.
-// It gets triggered after all the debug requests that followinitalized event,
+// It gets triggered after all the debug requests that follow initalized event,
 // so the s.debugger is guaranteed to be set.
 func (s *Server) onConfigurationDoneRequest(request *dap.ConfigurationDoneRequest, asyncSetupDone chan struct{}) {
 	defer s.asyncCommandDone(asyncSetupDone)
@@ -1442,6 +1452,11 @@ func (s *Server) onThreadsRequest(request *dap.ThreadsRequest) {
 
 // onAttachRequest handles 'attach' request.
 // This is a mandatory request to support.
+// Attach debug sessions support the following modes:
+// -- [DEFAULT] "local" -- attaches debugger to a local running process
+//      Required args: processID
+// -- "remote" - attaches client to a debugger already attached to a process
+//      Required args: none (host/port are used externally to connect)
 func (s *Server) onAttachRequest(request *dap.AttachRequest) {
 	var args AttachConfig = defaultAttachConfig // narrow copy for initializing non-zero default values
 	if err := unmarshalLaunchAttachArgs(request.Arguments, &args); err != nil {
@@ -1450,26 +1465,24 @@ func (s *Server) onAttachRequest(request *dap.AttachRequest) {
 	}
 
 	mode := args.Mode
-	if mode == "" {
+	switch mode {
+	case "":
 		mode = "local"
-	}
-
-	if !isValidAttachMode(mode) {
-		s.sendShowUserErrorResponse(request.Request, FailedToAttach, "Failed to attach",
-			fmt.Sprintf("invalid debug configuration - unsupported 'mode' attribute %q", args.Mode))
-		return
-	}
-	if mode == "local" {
+		fallthrough
+	case "local":
+		if s.debugger != nil {
+			s.sendShowUserErrorResponse(
+				request.Request, FailedToAttach,
+				"Failed to attach", "debugger already started - use remote mode to connect")
+			return
+		}
 		if args.ProcessID == 0 {
 			s.sendShowUserErrorResponse(request.Request, FailedToAttach, "Failed to attach",
 				"The 'processId' attribute is missing in debug configuration")
 			return
 		}
 		s.config.Debugger.AttachPid = args.ProcessID
-		if err := s.setLaunchAttachArgs(args.LaunchAttachCommonConfig); err != nil {
-			s.sendShowUserErrorResponse(request.Request, FailedToAttach, "Failed to attach", err.Error())
-			return
-		}
+		s.log.Debugf("attaching to pid %d", args.ProcessID)
 		if backend := args.Backend; backend != "" {
 			s.config.Debugger.Backend = backend
 		} else {
@@ -1485,7 +1498,31 @@ func (s *Server) onAttachRequest(request *dap.AttachRequest) {
 			s.sendShowUserErrorResponse(request.Request, FailedToAttach, "Failed to attach", err.Error())
 			return
 		}
+	case "remote":
+		if s.debugger == nil {
+			s.sendShowUserErrorResponse(request.Request, FailedToAttach, "Failed to attach", "no debugger found")
+			return
+		}
+		s.log.Debug("debugger already started")
+		// TODO(polina): once we allow initialize and attach request while running,
+		// halt before sending initialized event. onConfigurationDone will restart
+		// execution if user requested !stopOnEntry.
+
+		// Enable StepBack controls on supported backends
+		if s.config.Debugger.Backend == "rr" {
+			s.send(&dap.CapabilitiesEvent{Event: *newEvent("capabilities"), Body: dap.CapabilitiesEventBody{Capabilities: dap.Capabilities{SupportsStepBack: true}}})
+		}
+	default:
+		s.sendShowUserErrorResponse(request.Request, FailedToAttach, "Failed to attach",
+			fmt.Sprintf("invalid debug configuration - unsupported 'mode' attribute %q", args.Mode))
+		return
 	}
+
+	if err := s.setLaunchAttachArgs(args.LaunchAttachCommonConfig); err != nil {
+		s.sendShowUserErrorResponse(request.Request, FailedToAttach, "Failed to attach", err.Error())
+		return
+	}
+
 	// Notify the client that the debugger is ready to start accepting
 	// configuration requests for setting breakpoints, etc. The client
 	// will end the configuration sequence with 'configurationDone'.
diff --git a/service/dap/types.go b/service/dap/types.go
index 5aadf25b..832dfb67 100644
--- a/service/dap/types.go
+++ b/service/dap/types.go
@@ -34,14 +34,6 @@ func isValidLaunchMode(mode string) bool {
 	return false
 }
 
-// Attach debug sessions support the following modes:
-// -- [DEFAULT] "local" -- attaches debugger to a local running process
-//      Required args: processID
-// TODO(polina): support "remote" mode
-func isValidAttachMode(mode string) bool {
-	return mode == "local"
-}
-
 // Default values for Launch/Attach configs.
 // Used to initialize configuration variables before decoding
 // arguments in launch/attach requests.


Test patch:
diff --git a/service/dap/server_test.go b/service/dap/server_test.go
index 0f7d3d6a..1962c7d6 100644
--- a/service/dap/server_test.go
+++ b/service/dap/server_test.go
@@ -22,6 +22,7 @@ import (
 	protest "github.com/go-delve/delve/pkg/proc/test"
 	"github.com/go-delve/delve/service"
 	"github.com/go-delve/delve/service/dap/daptest"
+	"github.com/go-delve/delve/service/debugger"
 	"github.com/google/go-dap"
 )
 
@@ -63,19 +64,19 @@ func runTestBuildFlags(t *testing.T, name string, test func(c *daptest.Client, f
 }
 
 func startDapServerWithClient(t *testing.T, serverStopped chan struct{}) *daptest.Client {
-	listener, _ := startDapServer(t, serverStopped)
-	client := daptest.NewClient(listener.Addr().String())
+	server, _ := startDapServer(t, serverStopped)
+	client := daptest.NewClient(server.config.Listener.Addr().String())
 	return client
 }
 
-func startDapServer(t *testing.T, serverStopped chan struct{}) (listener net.Listener, forceStop chan struct{}) {
+func startDapServer(t *testing.T, serverStopped chan struct{}) (server *Server, forceStop chan struct{}) {
 	// Start the DAP server.
 	listener, err := net.Listen("tcp", ":0")
 	if err != nil {
 		t.Fatal(err)
 	}
 	disconnectChan := make(chan struct{})
-	server := NewServer(&service.Config{
+	server = NewServer(&service.Config{
 		Listener:       listener,
 		DisconnectChan: disconnectChan,
 	})
@@ -100,7 +101,7 @@ func startDapServer(t *testing.T, serverStopped chan struct{}) (listener net.Lis
 		server.Stop()
 	}()
 
-	return listener, forceStop
+	return server, forceStop
 }
 
 func TestForceStopNoClient(t *testing.T) {
@@ -112,9 +113,9 @@ func TestForceStopNoClient(t *testing.T) {
 
 func TestForceStopNoTarget(t *testing.T) {
 	serverStopped := make(chan struct{})
-	listener, forceStop := startDapServer(t, serverStopped)
-	client := daptest.NewClient(listener.Addr().String())
-	defer client.Close() // does not trigger Stop
+	server, forceStop := startDapServer(t, serverStopped)
+	client := daptest.NewClient(server.config.Listener.Addr().String())
+	defer client.Close()
 
 	client.InitializeRequest()
 	client.ExpectInitializeResponseAndCapabilities(t)
@@ -124,9 +125,9 @@ func TestForceStopNoTarget(t *testing.T) {
 
 func TestForceStopWithTarget(t *testing.T) {
 	serverStopped := make(chan struct{})
-	listener, forceStop := startDapServer(t, serverStopped)
-	client := daptest.NewClient(listener.Addr().String())
-	defer client.Close() // does not trigger Stop
+	server, forceStop := startDapServer(t, serverStopped)
+	client := daptest.NewClient(server.config.Listener.Addr().String())
+	defer client.Close()
 
 	client.InitializeRequest()
 	client.ExpectInitializeResponseAndCapabilities(t)
@@ -140,8 +141,8 @@ func TestForceStopWithTarget(t *testing.T) {
 
 func TestForceStopWhileStopping(t *testing.T) {
 	serverStopped := make(chan struct{})
-	listener, forceStop := startDapServer(t, serverStopped)
-	client := daptest.NewClient(listener.Addr().String())
+	server, forceStop := startDapServer(t, serverStopped)
+	client := daptest.NewClient(server.config.Listener.Addr().String())
 
 	client.InitializeRequest()
 	client.ExpectInitializeResponseAndCapabilities(t)
@@ -4509,6 +4510,24 @@ func TestAttachRequest(t *testing.T) {
 	})
 }
 
+// Since we are in async mode while running, we might receive thee messages after pause request
+// in either order.
+func expectPauseResponseAndStoppedEvent(t *testing.T, client *daptest.Client) {
+	t.Helper()
+	for i := 0; i < 2; i++ {
+		msg := client.ExpectMessage(t)
+		switch m := msg.(type) {
+		case *dap.StoppedEvent:
+			if m.Body.Reason != "pause" || m.Body.ThreadId != 0 && m.Body.ThreadId != 1 {
+				t.Errorf("\ngot %#v\nwant ThreadId=0/1 Reason='pause'", m)
+			}
+		case *dap.PauseResponse:
+		default:
+			t.Fatalf("got %#v, want StoppedEvent or PauseResponse", m)
+		}
+	}
+}
+
 func TestPauseAndContinue(t *testing.T) {
 	runTest(t, "loopprog", func(client *daptest.Client, fixture protest.Fixture) {
 		runDebugSessionWithBPs(t, client, "launch",
@@ -4530,19 +4549,7 @@ func TestPauseAndContinue(t *testing.T) {
 
 					// Halt pauses all goroutines, so thread id is ignored
 					client.PauseRequest(56789)
-					// Since we are in async mode while running, we might receive next two messages in either order.
-					for i := 0; i < 2; i++ {
-						msg := client.ExpectMessage(t)
-						switch m := msg.(type) {
-						case *dap.StoppedEvent:
-							if m.Body.Reason != "pause" || m.Body.ThreadId != 0 && m.Body.ThreadId != 1 {
-								t.Errorf("\ngot %#v\nwant ThreadId=0/1 Reason='pause'", m)
-							}
-						case *dap.PauseResponse:
-						default:
-							t.Fatalf("got %#v, want StoppedEvent or PauseResponse", m)
-						}
-					}
+					expectPauseResponseAndStoppedEvent(t, client)
 
 					// Pause will be a no-op at a pause: there will be no additional stopped events
 					client.PauseRequest(1)
@@ -5124,7 +5131,7 @@ func TestBadLaunchRequests(t *testing.T) {
 			"Failed to launch: invalid debug configuration - cannot use {\"from\":\"path1\",\"to\":123} as 'substitutePath' of type {\"from\":string, \"to\":string}")
 		// Bad "cwd"
 		client.LaunchRequestWithArgs(map[string]interface{}{"mode": "debug", "program": fixture.Source, "cwd": 123})
-		checkFailedToLaunchWithMessage(client.ExpectErrorResponse(t),
+		checkFailedToLaunchWithMessage(client.ExpectVisibleErrorResponse(t),
 			"Failed to launch: invalid debug configuration - cannot unmarshal number into \"cwd\" of type string")
 
 		// Skip detailed message checks for potentially different OS-specific errors.
@@ -5242,10 +5249,6 @@ func TestBadAttachRequest(t *testing.T) {
 		}
 
 		// Bad "mode"
-		client.AttachRequest(map[string]interface{}{"mode": "remote"})
-		checkFailedToAttachWithMessage(client.ExpectVisibleErrorResponse(t),
-			"Failed to attach: invalid debug configuration - unsupported 'mode' attribute \"remote\"")
-
 		client.AttachRequest(map[string]interface{}{"mode": "blah blah blah"})
 		checkFailedToAttachWithMessage(client.ExpectVisibleErrorResponse(t),
 			"Failed to attach: invalid debug configuration - unsupported 'mode' attribute \"blah blah blah\"")
@@ -5318,6 +5321,102 @@ func TestBadAttachRequest(t *testing.T) {
 	})
 }
 
+// TODO(polina): also add launchDebuggerWithTargetRunning
+func launchDebuggerWithTargetHalted(t *testing.T, fixture string) *debugger.Debugger {
+	t.Helper()
+	bin := protest.BuildFixture(fixture, protest.AllNonOptimized)
+	cfg := service.Config{
+		ProcessArgs: []string{bin.Path},
+		Debugger:    debugger.Config{Backend: "default"},
+	}
+	dbg, err := debugger.New(&cfg.Debugger, cfg.ProcessArgs) // debugger halts process on entry
+	if err != nil {
+		t.Fatal("failed to start debugger:", err)
+	}
+	return dbg
+}
+
+// runTestWithDebugger starts the server and sets its debugger, initializes a debug session,
+// runs test, then disconnects. Expects the process at the end of test() to be halted
+func runTestWithDebugger(t *testing.T, dbg *debugger.Debugger, test func(c *daptest.Client)) {
+	serverStopped := make(chan struct{})
+	server, _ := startDapServer(t, serverStopped)
+	// TODO(polina): update once the server interface is refactored to take debugger as arg
+	server.debugger = dbg
+	client := daptest.NewClient(server.listener.Addr().String())
+	defer client.Close()
+	client.InitializeRequest()
+	client.ExpectInitializeResponseAndCapabilities(t)
+
+	test(client)
+
+	client.DisconnectRequest()
+	if server.config.AcceptMulti {
+		// TODO(polina): support multi-client mode
+		t.Fatal("testing of accept-multiclient not yet supporteed")
+	} else if server.config.Debugger.AttachPid == 0 { // launched target
+		client.ExpectOutputEventDetachingKill(t)
+	} else { // attached to target
+		client.ExpectOutputEventDetachingNoKill(t)
+	}
+	client.ExpectDisconnectResponse(t)
+	client.ExpectTerminatedEvent(t)
+
+	<-serverStopped
+}
+
+func TestAttachRemoteToHaltedTargetStopOnEntry(t *testing.T) {
+	// Halted + stop on entry
+	runTestWithDebugger(t, launchDebuggerWithTargetHalted(t, "increment"), func(client *daptest.Client) {
+		client.AttachRequest(map[string]interface{}{"mode": "remote", "stopOnEntry": true})
+		client.ExpectInitializedEvent(t)
+		client.ExpectAttachResponse(t)
+		client.ConfigurationDoneRequest()
+		client.ExpectStoppedEvent(t)
+		client.ExpectConfigurationDoneResponse(t)
+	})
+}
+
+func TestAttachRemoteToHaltedTargetContinueOnEntry(t *testing.T) {
+	// Halted + continue on entry
+	runTestWithDebugger(t, launchDebuggerWithTargetHalted(t, "http_server"), func(client *daptest.Client) {
+		client.AttachRequest(map[string]interface{}{"mode": "remote", "stopOnEntry": false})
+		client.ExpectInitializedEvent(t)
+		client.ExpectAttachResponse(t)
+		client.ConfigurationDoneRequest()
+		client.ExpectConfigurationDoneResponse(t)
+		// Continuing
+		time.Sleep(time.Second)
+		// Halt to make the disconnect sequence more predictable.
+		client.PauseRequest(1)
+		expectPauseResponseAndStoppedEvent(t, client)
+	})
+}
+
+// TODO(polina): Running + stop/continue on entry
+
+func TestLaunchAttachErrorWhenDebugInProgress(t *testing.T) {
+	runTestWithDebugger(t, launchDebuggerWithTargetHalted(t, "increment"), func(client *daptest.Client) {
+		client.AttachRequest(map[string]interface{}{"mode": "local", "processId": 100})
+		er := client.ExpectVisibleErrorResponse(t)
+		msg := "Failed to attach: debugger already started - use remote mode to connect"
+		if er.Body.Error.Id != 3001 || er.Body.Error.Format != msg {
+			t.Errorf("got %#v, want Id=3001 Format=%q", er, msg)
+		}
+		tests := []string{"debug", "test", "exec", "replay", "core"}
+		for _, mode := range tests {
+			t.Run(mode, func(t *testing.T) {
+				client.LaunchRequestWithArgs(map[string]interface{}{"mode": mode})
+				er := client.ExpectVisibleErrorResponse(t)
+				msg := "Failed to launch: debugger already started - use remote attach to connect to a server with an active debug session"
+				if er.Body.Error.Id != 3000 || er.Body.Error.Format != msg {
+					t.Errorf("got %#v, want Id=3001 Format=%q", er, msg)
+				}
+			})
+		}
+	})
+}
+
 func TestBadInitializeRequest(t *testing.T) {
 	runInitializeTest := func(args dap.InitializeRequestArguments, err string) {
 		t.Helper()