Repository: go-delve/delve
Pull request #2686 (2021-09-02 17:12:17)

service/dap: do not send a halt request if the debuggee is not running
This change makes sure to avoid sending a pause request while the debuggee is not running, to try to protect
the user from the undefined Halt behavior:

```
	// Halt suspends the process.
	// The effect of Halt while the target process is stopped, or in the
	// process of stopping, is operating system and timing dependent. It will
	// either have no effect or cause the following resume to stop immediately.
```

It is not possible to fully protect the dap client, and the halt request may still have the same issue.

This adds test to service/dap to test this issue.

Updates go-delve/delve#2685

suzmue: @aarzilli @derekparker If there is a better way to go about fixing this, please feel free to re-use my tests in a different PR
aarzilli: See test failures. In general: I don't think there is a way to do what you want to do. The behavior of RequestManualStop is documented in service/api/types.go:

```
	// Halt suspends the process.
	// The effect of Halt while the target process is stopped, or in the
	// process of stopping, is operating system and timing dependent. It will
	// either have no effect or cause the following resume to stop immediately.
```

For #2634 I would suggest writing the tests in a way that does not rely on undefined behavior.
aarzilli: PS. the code that you are changing is our critical section, it's very easy to introduce race conditions with the kernel there that happen maybe 1 or 2% of the times our tests are run. Usually when I change anything in there, unless it's a completely trivial change, I'll run all the tests in pkg/proc 200 times on all affected architectures and the specific test I'm looking for 1000 times.

suzmue: > See test failures. In general: I don't think there is a way to do what you want to do. The behavior of RequestManualStop is documented in service/api/types.go:
> 
> ```
> 	// Halt suspends the process.
> 	// The effect of Halt while the target process is stopped, or in the
> 	// process of stopping, is operating system and timing dependent. It will
> 	// either have no effect or cause the following resume to stop immediately.
> ```
> 
> For #2634 I would suggest writing the tests in a way that does not rely on undefined behavior.

Thanks for pointing me to that. For some reason I thought that Halt while stopped was a no-op, I see it is clearly defined as otherwise.

I can instead make this a DAP issue and try to minimize the likelihood of hitting the race condition (and write tests that do not rely on this.)

Patch:
diff --git a/service/dap/server.go b/service/dap/server.go
index b2edbdb3..c5314a84 100644
--- a/service/dap/server.go
+++ b/service/dap/server.go
@@ -1568,10 +1568,12 @@ func (s *Server) doStepCommand(command string, threadId int, asyncSetupDone chan
 // onPauseRequest handles 'pause' request.
 // This is a mandatory request to support.
 func (s *Server) onPauseRequest(request *dap.PauseRequest) {
-	_, err := s.debugger.Command(&api.DebuggerCommand{Name: api.Halt}, nil)
-	if err != nil {
-		s.sendErrorResponse(request.Request, UnableToHalt, "Unable to halt execution", err.Error())
-		return
+	if s.debugger.IsRunning() {
+		_, err := s.debugger.Command(&api.DebuggerCommand{Name: api.Halt}, nil)
+		if err != nil {
+			s.sendErrorResponse(request.Request, UnableToHalt, "Unable to halt execution", err.Error())
+			return
+		}
 	}
 	s.send(&dap.PauseResponse{Response: *newResponse(request.Request)})
 	// No need to send any event here.


Test patch:
diff --git a/service/dap/server_test.go b/service/dap/server_test.go
index ca7b3f4d..bbfa0e4f 100644
--- a/service/dap/server_test.go
+++ b/service/dap/server_test.go
@@ -2296,6 +2296,64 @@ func TestSetBreakpoint(t *testing.T) {
 	})
 }
 
+func TestPauseAtStop(t *testing.T) {
+	runTest(t, "loopprog", func(client *daptest.Client, fixture protest.Fixture) {
+		runDebugSessionWithBPs(t, client, "launch",
+			// Launch
+			func() {
+				client.LaunchRequest("exec", fixture.Path, !stopOnEntry)
+			},
+			// Set breakpoints
+			fixture.Source, []int{16},
+			[]onBreakpoint{{
+				execute: func() {
+					checkStop(t, client, 1, "main.main", 16)
+
+					client.SetBreakpointsRequest(fixture.Source, []int{6, 8})
+					expectSetBreakpointsResponse(t, client, []Breakpoint{{6, fixture.Source, true, ""}, {8, fixture.Source, true, ""}})
+
+					// Send a pause request while stopped on a cleared breakpoint.
+					client.PauseRequest(1)
+					client.ExpectPauseResponse(t)
+
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+					client.ExpectStoppedEvent(t)
+					checkStop(t, client, 1, "main.loop", 6)
+
+					// Send a pause request while stopped on a breakpoint.
+					client.PauseRequest(1)
+					client.ExpectPauseResponse(t)
+
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+					se := client.ExpectStoppedEvent(t)
+					if se.Body.Reason != "breakpoint" {
+						t.Errorf("got %#v, expected breakpoint", se)
+					}
+					checkStop(t, client, 1, "main.loop", 8)
+
+					// Send a pause request while stopped after stepping.
+					client.NextRequest(1)
+					client.ExpectNextResponse(t)
+					client.ExpectStoppedEvent(t)
+					checkStop(t, client, 1, "main.loop", 9)
+
+					client.PauseRequest(1)
+					client.ExpectPauseResponse(t)
+
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+
+					client.ExpectStoppedEvent(t)
+					checkStop(t, client, 1, "main.loop", 8)
+				},
+				// The program has an infinite loop, so we must kill it by disconnecting.
+				disconnect: true,
+			}})
+	})
+}
+
 func checkHitBreakpointIds(t *testing.T, se *dap.StoppedEvent, reason string, id int) {
 	if se.Body.ThreadId != 1 || se.Body.Reason != reason || len(se.Body.HitBreakpointIds) != 1 || se.Body.HitBreakpointIds[0] != id {
 		t.Errorf("got %#v, want Reason=%q, ThreadId=1, HitBreakpointIds=[]int{%d}", se, reason, id)