service/dap: support remote-attaching to running debugger (#2737)

Co-authored-by: Polina Sokolova <polinasok@users.noreply.github.com>

service/dap/server.go

@@ -523,6 +523,18 @@ func (s *Session) handleRequest(request dap.Message) {
 
 	// These requests, can be handled regardless of whether the targret is running
 	switch request := request.(type) {
+	case *dap.InitializeRequest:
+		// Required
+		s.onInitializeRequest(request)
+		return
+	case *dap.LaunchRequest:
+		// Required
+		s.onLaunchRequest(request)
+		return
+	case *dap.AttachRequest:
+		// Required
+		s.onAttachRequest(request)
+		return
 	case *dap.DisconnectRequest:
 		// Required
 		s.onDisconnectRequest(request)
@@ -560,7 +572,7 @@ func (s *Session) handleRequest(request dap.Message) {
 	// the next stop. In addition, the editor itself might block waiting
 	// for these requests to return. We are not aware of any requests
 	// that would benefit from this approach at this time.
-	if s.debugger != nil && s.isRunningCmd() {
+	if s.debugger != nil && s.debugger.IsRunning() || s.isRunningCmd() {
 		switch request := request.(type) {
 		case *dap.ThreadsRequest:
 			// On start-up, the client requests the baseline of currently existing threads
@@ -664,17 +676,6 @@ func (s *Session) 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)
-	case *dap.LaunchRequest:
-		// Required
-		s.onLaunchRequest(request)
-	case *dap.AttachRequest:
-		// Required
-		s.onAttachRequest(request)
 	case *dap.SetBreakpointsRequest:
 		// Required
 		s.onSetBreakpointsRequest(request)
@@ -1514,7 +1515,7 @@ func closeIfOpen(ch chan struct{}) {
 // onConfigurationDoneRequest handles 'configurationDone' request.
 // This is an optional request enabled by capability ‘supportsConfigurationDoneRequest’.
 // It gets triggered after all the debug requests that follow initalized event,
-// so the s.debugger is guaranteed to be set.
+// so the s.debugger is guaranteed to be set. Expects the target to be halted.
 func (s *Session) onConfigurationDoneRequest(request *dap.ConfigurationDoneRequest, allowNextStateChange chan struct{}) {
 	defer closeIfOpen(allowNextStateChange)
 	if s.args.stopOnEntry {
@@ -1705,10 +1706,14 @@ func (s *Session) onAttachRequest(request *dap.AttachRequest) {
 			return
 		}
 		s.config.log.Debug("debugger already started")
-		// TODO(polina): once we allow initialize and attach request while running,
+		// Halt for configuration sequence. onConfigurationDone will restart
-		// halt before sending initialized event. onConfigurationDone will restart
 		// execution if user requested !stopOnEntry.
-
+		s.changeStateMu.Lock()
+		defer s.changeStateMu.Unlock()
+		if _, err := s.halt(); err != nil {
+			s.sendShowUserErrorResponse(request.Request, FailedToAttach, "Failed to attach", err.Error())
+			return
+		}
 		// 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}}})

service/dap/server_test.go

@@ -4452,12 +4452,12 @@ func verifyStopLocation(t *testing.T, client *daptest.Client, thread int, name s
 // The details have been tested by other tests,
 // so this is just a sanity check.
 // Skips line check if line is -1.
-func checkStop(t *testing.T, client *daptest.Client, thread int, name string, line int) {
+func checkStop(t *testing.T, client *daptest.Client, thread int, fname string, line int) {
 	t.Helper()
 	client.ThreadsRequest()
 	client.ExpectThreadsResponse(t)
 
-	verifyStopLocation(t, client, thread, name, line)
+	verifyStopLocation(t, client, thread, fname, line)
 
 	client.ScopesRequest(1000)
 	client.ExpectScopesResponse(t)
@@ -5748,23 +5748,44 @@ func TestBadAttachRequest(t *testing.T) {
 	})
 }
 
-// TODO(polina): also add launchDebuggerWithTargetRunning
+func launchDebuggerWithTargetRunning(t *testing.T, fixture string) (*protest.Fixture, *debugger.Debugger) {
-func launchDebuggerWithTargetHalted(t *testing.T, fixture string) *debugger.Debugger {
 	t.Helper()
-	bin := protest.BuildFixture(fixture, protest.AllNonOptimized)
+	fixbin, dbg := launchDebuggerWithTargetHalted(t, fixture)
+	running := make(chan struct{})
+	var err error
+	go func() {
+		t.Helper()
+		_, err = dbg.Command(&api.DebuggerCommand{Name: api.Continue}, running)
+		select {
+		case <-running:
+		default:
+			close(running)
+		}
+	}()
+	<-running
+	if err != nil {
+		t.Fatal("failed to continue on launch", err)
+	}
+	return fixbin, dbg
+}
+
+func launchDebuggerWithTargetHalted(t *testing.T, fixture string) (*protest.Fixture, *debugger.Debugger) {
+	t.Helper()
+	fixbin := protest.BuildFixture(fixture, protest.AllNonOptimized)
 	cfg := service.Config{
-		ProcessArgs: []string{bin.Path},
+		ProcessArgs: []string{fixbin.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
+	return &fixbin, 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.
+// runs test, then disconnects. Expects no running async handler at the end of test() (either
+// process is halted or debug session never launched.)
 func runTestWithDebugger(t *testing.T, dbg *debugger.Debugger, test func(c *daptest.Client)) {
 	serverStopped := make(chan struct{})
 	server, _ := startDAPServer(t, serverStopped)
@@ -5797,7 +5818,8 @@ func runTestWithDebugger(t *testing.T, dbg *debugger.Debugger, test func(c *dapt
 
 func TestAttachRemoteToHaltedTargetStopOnEntry(t *testing.T) {
 	// Halted + stop on entry
-	runTestWithDebugger(t, launchDebuggerWithTargetHalted(t, "increment"), func(client *daptest.Client) {
+	_, dbg := launchDebuggerWithTargetHalted(t, "increment")
+	runTestWithDebugger(t, dbg, func(client *daptest.Client) {
 		client.AttachRequest(map[string]interface{}{"mode": "remote", "stopOnEntry": true})
 		client.ExpectInitializedEvent(t)
 		client.ExpectAttachResponse(t)
@@ -5809,7 +5831,8 @@ func TestAttachRemoteToHaltedTargetStopOnEntry(t *testing.T) {
 
 func TestAttachRemoteToHaltedTargetContinueOnEntry(t *testing.T) {
 	// Halted + continue on entry
-	runTestWithDebugger(t, launchDebuggerWithTargetHalted(t, "http_server"), func(client *daptest.Client) {
+	_, dbg := launchDebuggerWithTargetHalted(t, "http_server")
+	runTestWithDebugger(t, dbg, func(client *daptest.Client) {
 		client.AttachRequest(map[string]interface{}{"mode": "remote", "stopOnEntry": false})
 		client.ExpectInitializedEvent(t)
 		client.ExpectAttachResponse(t)
@@ -5823,7 +5846,41 @@ func TestAttachRemoteToHaltedTargetContinueOnEntry(t *testing.T) {
 	})
 }
 
-// TODO(polina): Running + stop/continue on entry
+func TestAttachRemoteToRunningTargetStopOnEntry(t *testing.T) {
+	fixture, dbg := launchDebuggerWithTargetRunning(t, "loopprog")
+	runTestWithDebugger(t, dbg, func(client *daptest.Client) {
+		client.AttachRequest(map[string]interface{}{"mode": "remote", "stopOnEntry": true})
+		client.ExpectInitializedEvent(t)
+		client.ExpectAttachResponse(t)
+		// Target is halted here
+		client.SetBreakpointsRequest(fixture.Source, []int{8})
+		expectSetBreakpointsResponse(t, client, []Breakpoint{{8, fixture.Source, true, ""}})
+		client.ConfigurationDoneRequest()
+		client.ExpectStoppedEvent(t)
+		client.ExpectConfigurationDoneResponse(t)
+		client.ContinueRequest(1)
+		client.ExpectContinueResponse(t)
+		client.ExpectStoppedEvent(t)
+		checkStop(t, client, 1, "main.loop", 8)
+	})
+}
+
+func TestAttachRemoteToRunningTargetContinueOnEntry(t *testing.T) {
+	fixture, dbg := launchDebuggerWithTargetRunning(t, "loopprog")
+	runTestWithDebugger(t, dbg, func(client *daptest.Client) {
+		client.AttachRequest(map[string]interface{}{"mode": "remote", "stopOnEntry": false})
+		client.ExpectInitializedEvent(t)
+		client.ExpectAttachResponse(t)
+		// Target is halted here
+		client.SetBreakpointsRequest(fixture.Source, []int{8})
+		expectSetBreakpointsResponse(t, client, []Breakpoint{{8, fixture.Source, true, ""}})
+		client.ConfigurationDoneRequest()
+		// Target is restarted here
+		client.ExpectConfigurationDoneResponse(t)
+		client.ExpectStoppedEvent(t)
+		checkStop(t, client, 1, "main.loop", 8)
+	})
+}
 
 // TestMultiClient tests that that remote attach doesn't take down
 // the server in multi-client mode unless terminateDebugee is explicitely set.
@@ -5853,7 +5910,7 @@ func TestAttachRemoteMultiClient(t *testing.T) {
 			// hack to test the inner connection logic that can be used by a server that does.
 			server.session.config.AcceptMulti = true
 			// TODO(polina): update once the server interface is refactored to take debugger as arg
-			server.session.debugger = launchDebuggerWithTargetHalted(t, "increment")
+			_, server.session.debugger = launchDebuggerWithTargetHalted(t, "increment")
 			server.sessionMu.Unlock()
 
 			client.InitializeRequest()
@@ -5887,25 +5944,44 @@ func TestAttachRemoteMultiClient(t *testing.T) {
 }
 
 func TestLaunchAttachErrorWhenDebugInProgress(t *testing.T) {
-	runTestWithDebugger(t, launchDebuggerWithTargetHalted(t, "increment"), func(client *daptest.Client) {
+	tests := []struct {
-		client.AttachRequest(map[string]interface{}{"mode": "local", "processId": 100})
+		name string
-		er := client.ExpectVisibleErrorResponse(t)
+		dbg  func() *debugger.Debugger
-		msg := "Failed to attach: debugger already started - use remote mode to connect"
+	}{
-		if er.Body.Error.Id != FailedToAttach || er.Body.Error.Format != msg {
+		{"halted", func() *debugger.Debugger { _, dbg := launchDebuggerWithTargetHalted(t, "increment"); return dbg }},
-			t.Errorf("got %#v, want Id=%d Format=%q", er, FailedToAttach, msg)
+		{"running", func() *debugger.Debugger { _, dbg := launchDebuggerWithTargetRunning(t, "loopprog"); return dbg }},
-		}
+	}
-		tests := []string{"debug", "test", "exec", "replay", "core"}
+	for _, tc := range tests {
-		for _, mode := range tests {
+		t.Run(tc.name, func(t *testing.T) {
-			t.Run(mode, func(t *testing.T) {
+			runTestWithDebugger(t, tc.dbg(), func(client *daptest.Client) {
-				client.LaunchRequestWithArgs(map[string]interface{}{"mode": mode})
+				client.EvaluateRequest("1==1", 0 /*no frame specified*/, "repl")
+				if tc.name == "running" {
+					client.ExpectInvisibleErrorResponse(t)
+				} else {
+					client.ExpectEvaluateResponse(t)
+				}
+
+				// Both launch and attach requests should go through for additional error checking
+				client.AttachRequest(map[string]interface{}{"mode": "local", "processId": 100})
 				er := client.ExpectVisibleErrorResponse(t)
-				msg := "Failed to launch: debugger already started - use remote attach to connect to a server with an active debug session"
+				msg := "Failed to attach: debugger already started - use remote mode to connect"
-				if er.Body.Error.Id != FailedToLaunch || er.Body.Error.Format != msg {
+				if er.Body.Error.Id != FailedToAttach || er.Body.Error.Format != msg {
-					t.Errorf("got %#v, want Id=%d Format=%q", er, FailedToLaunch, msg)
+					t.Errorf("got %#v, want Id=%d Format=%q", er, FailedToAttach, 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 != FailedToLaunch || er.Body.Error.Format != msg {
+							t.Errorf("got %#v, want Id=%d Format=%q", er, FailedToLaunch, msg)
+						}
+					})
 				}
 			})
-		}
+		})
-	})
+	}
 }
 
 func TestBadInitializeRequest(t *testing.T) {