Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

dap: support suspendDebuggee in multi-client mode #3003

Closed
wants to merge 4 commits into from
Closed

dap: support suspendDebuggee in multi-client mode #3003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dc3d4d2
dap: remote attach must not use terminateDebuggee in sigle-client lau…
polinasok May 2, 2022
d58656a
dap: support suspendDebuggee in multi-client mode
polinasok May 5, 2022
4faf342
Merge master
polinasok May 18, 2022
427a017
Fix staticcheck
polinasok May 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions cmd/dlv/dlv_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -757,8 +757,10 @@ func TestDAPCmdWithNoDebugBinary(t *testing.T) {
func newDAPRemoteClient(t *testing.T, addr string, isDlvAttach bool, isMulti bool) *daptest.Client {
c := daptest.NewClient(addr)
c.AttachRequest(map[string]interface{}{"mode": "remote", "stopOnEntry": true})
if isDlvAttach || isMulti {
c.ExpectCapabilitiesEventSupportTerminateDebuggee(t)
if isDlvAttach {
c.ExpectCapabilitiesEventSupportDisconnectOptions(t, true, false)
} else if isMulti {
c.ExpectCapabilitiesEventSupportDisconnectOptions(t, true, true)
}
c.ExpectInitializedEvent(t)
c.ExpectAttachResponse(t)
Expand Down Expand Up @@ -814,7 +816,7 @@ func TestRemoteDAPClient(t *testing.T) {
}

func closeDAPRemoteMultiClient(t *testing.T, c *daptest.Client, expectStatus string) {
c.DisconnectRequest()
c.DisconnectRequestWithOptions(false, true)
c.ExpectOutputEventClosingClient(t, expectStatus)
c.ExpectDisconnectResponse(t)
c.ExpectTerminatedEvent(t)
Expand Down Expand Up @@ -863,7 +865,7 @@ func TestRemoteDAPClientMulti(t *testing.T) {
dapclient.ExpectContinueResponse(t)
dapclient.ExpectStoppedEvent(t)
dapclient.CheckStopLocation(t, 1, "main.main", 5)
closeDAPRemoteMultiClient(t, dapclient, "halted")
closeDAPRemoteMultiClient(t, dapclient, "suspended")

// Client 2 reconnects at main.main and continues to process exit
dapclient2 := newDAPRemoteClient(t, listenAddr, false, true)
Expand Down
18 changes: 15 additions & 3 deletions service/dap/daptest/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,11 +136,14 @@ func (c *Client) ExpectUnsupportedCommandErrorResponse(t *testing.T) *dap.ErrorR
return c.ExpectErrorResponseWith(t, 9999, "Unsupported command", false)
}

func (c *Client) ExpectCapabilitiesEventSupportTerminateDebuggee(t *testing.T) *dap.CapabilitiesEvent {
func (c *Client) ExpectCapabilitiesEventSupportDisconnectOptions(t *testing.T, terminate bool, suspend bool) *dap.CapabilitiesEvent {
t.Helper()
e := c.ExpectCapabilitiesEvent(t)
if !e.Body.Capabilities.SupportTerminateDebuggee {
t.Errorf("\ngot %#v\nwant SupportTerminateDebuggee=true", e.Body.Capabilities.SupportTerminateDebuggee)
if e.Body.Capabilities.SupportTerminateDebuggee != terminate {
t.Errorf("\ngot %#v\nwant SupportTerminateDebuggee=%v", e.Body.Capabilities.SupportTerminateDebuggee, terminate)
}
if e.Body.Capabilities.SupportSuspendDebuggee != suspend {
t.Errorf("\ngot %#v\nwant SupportSuspendDebuggee=%v", e.Body.Capabilities.SupportSuspendDebuggee, suspend)
}
return e
}
Expand Down Expand Up @@ -281,6 +284,15 @@ func (c *Client) DisconnectRequestWithKillOption(kill bool) {
c.send(request)
}

// DisconnectRequestWithOptions sends a 'disconnect' request with an option to specify
// `terminateDebuggee` and `suspendDebuggee`
func (c *Client) DisconnectRequestWithOptions(terminate bool, suspend bool) {
request := &dap.DisconnectRequest{Request: *c.newRequest("disconnect")}
request.Arguments.TerminateDebuggee = terminate
request.Arguments.SuspendDebuggee = suspend
c.send(request)
}

// SetBreakpointsRequest sends a 'setBreakpoints' request.
func (c *Client) SetBreakpointsRequest(file string, lines []int) {
c.SetBreakpointsRequestWithArgs(file, lines, nil, nil, nil)
Expand Down
37 changes: 23 additions & 14 deletions service/dap/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,9 +843,10 @@ func (s *Session) onInitializeRequest(request *dap.InitializeRequest) {
response.Body.SupportsSteppingGranularity = true
response.Body.SupportsLogPoints = true
response.Body.SupportsDisassembleRequest = true
// To be enabled by CapabilitiesEvent based on launch configuration
// To be enabled by CapabilitiesEvent based on launch/attach configuration
response.Body.SupportsStepBack = false
response.Body.SupportTerminateDebuggee = false
response.Body.SupportSuspendDebuggee = false
// TODO(polina): support these requests in addition to vscode-go feature parity
response.Body.SupportsTerminateRequest = false
response.Body.SupportsRestartRequest = false
Expand Down Expand Up @@ -1136,25 +1137,34 @@ func (s *Session) onDisconnectRequest(request *dap.DisconnectRequest) {
// This is a multi-use server/debugger, so a disconnect request that doesn't
// terminate the debuggee should clean up only the client connection and pointer to debugger,
// but not the entire server.
status := "halted"
if s.isRunningCmd() {
status := "running"
if request.Arguments.SuspendDebuggee {
status = "suspended"
s.changeStateMu.Lock()
defer s.changeStateMu.Unlock()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This defer gets executed when the function exits. Is this really the intended scope? There's a lot of code after this if/else that will get executed either with or without holding the mutex.

s.setHaltRequested(true)
if _, err := s.halt(); err != nil {
s.config.log.Debug("halt returned error: ", err)
}
} else if !s.isRunningCmd() {
status = "running"
} else if s, err := s.debugger.State(false); processExited(s, err) {
asyncSetupDone := make(chan struct{})
go func() {
defer closeIfOpen(asyncSetupDone)
if _, err := s.runUntilStop(api.Continue, asyncSetupDone); err != nil {
s.config.log.Debug("continue returned error: ", err)
}
}()
<-asyncSetupDone
}
if state, err := s.debugger.State(true /*nowait*/); processExited(state, err) {
status = "exited"
}
s.logToConsole(fmt.Sprintf("Closing client session, but leaving multi-client DAP server at %s with debuggee %s", s.config.Listener.Addr().String(), status))
s.send(&dap.DisconnectResponse{Response: *newResponse(request.Request)})
s.send(&dap.TerminatedEvent{Event: *newEvent("terminated")})
s.conn.Close()
s.debugger = nil
// 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
// the client connection fails unexpectedly and the user needs to reconnect.
// TODO(polina): should we always issue a continue here if it is not running
// like is done in vscode-go legacy adapter?
// Ideally we want to use bool suspendDebuggee flag, but it is not yet
// available in vscode: https://github.com/microsoft/vscode/issues/134412
return
}

Expand Down Expand Up @@ -1773,8 +1783,7 @@ func (s *Session) onAttachRequest(request *dap.AttachRequest) {
// Customize termination options for debugger and debuggee
if s.config.AcceptMulti {
// User can stop debugger with process or leave it running
s.send(&dap.CapabilitiesEvent{Event: *newEvent("capabilities"), Body: dap.CapabilitiesEventBody{Capabilities: dap.Capabilities{SupportTerminateDebuggee: true}}})
// TODO(polina): support SupportSuspendDebuggee when available
s.send(&dap.CapabilitiesEvent{Event: *newEvent("capabilities"), Body: dap.CapabilitiesEventBody{Capabilities: dap.Capabilities{SupportTerminateDebuggee: true, SupportSuspendDebuggee: true}}})
} else if s.config.Debugger.AttachPid > 0 {
// User can stop debugger with process or leave the processs running
s.send(&dap.CapabilitiesEvent{Event: *newEvent("capabilities"), Body: dap.CapabilitiesEventBody{Capabilities: dap.Capabilities{SupportTerminateDebuggee: true}}})
Expand Down
33 changes: 17 additions & 16 deletions service/dap/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,7 @@ func TestAttachStopOnEntry(t *testing.T) {
// 2 >> attach, << initialized, << attach
client.AttachRequest(
map[string]interface{}{"mode": "local", "processId": cmd.Process.Pid, "stopOnEntry": true, "backend": "default"})
client.ExpectCapabilitiesEventSupportTerminateDebuggee(t)
client.ExpectCapabilitiesEventSupportDisconnectOptions(t, true, false)
initEvent := client.ExpectInitializedEvent(t)
if initEvent.Seq != 0 {
t.Errorf("\ngot %#v\nwant Seq=0", initEvent)
Expand Down Expand Up @@ -3682,7 +3682,7 @@ func substitutePathTestHelper(t *testing.T, fixture protest.Fixture, client *dap
switch request {
case "attach":
client.AttachRequest(launchAttachConfig)
client.ExpectCapabilitiesEventSupportTerminateDebuggee(t)
client.ExpectCapabilitiesEventSupportDisconnectOptions(t, true, false)
case "launch":
client.LaunchRequestWithArgs(launchAttachConfig)
default:
Expand Down Expand Up @@ -5670,7 +5670,7 @@ func TestAttachRequest(t *testing.T) {
func() {
client.AttachRequest(map[string]interface{}{
/*"mode": "local" by default*/ "processId": cmd.Process.Pid, "stopOnEntry": false})
client.ExpectCapabilitiesEventSupportTerminateDebuggee(t)
client.ExpectCapabilitiesEventSupportDisconnectOptions(t, true, false)
},
// Set breakpoints
fixture.Source, []int{8},
Expand Down Expand Up @@ -6589,7 +6589,7 @@ func TestAttachRemoteToDlvAttachHaltedStopOnEntry(t *testing.T) {
cmd, dbg := attachDebuggerWithTargetHalted(t, "http_server")
runTestWithDebugger(t, dbg, func(client *daptest.Client) {
client.AttachRequest(map[string]interface{}{"mode": "remote", "stopOnEntry": true})
client.ExpectCapabilitiesEventSupportTerminateDebuggee(t)
client.ExpectCapabilitiesEventSupportDisconnectOptions(t, true, false)
client.ExpectInitializedEvent(t)
client.ExpectAttachResponse(t)
client.ConfigurationDoneRequest()
Expand Down Expand Up @@ -6662,6 +6662,7 @@ type MultiClientCloseServerMock struct {
debugger *debugger.Debugger
forceStop chan struct{}
stopped chan struct{}
cmd *exec.Cmd
}

func NewMultiClientCloseServerMock(t *testing.T, fixture string) *MultiClientCloseServerMock {
Expand Down Expand Up @@ -6708,17 +6709,21 @@ func (s *MultiClientCloseServerMock) verifyStopped(t *testing.T) {

// TestAttachRemoteMultiClientDisconnect tests that that remote attach doesn't take down
// the server in multi-client mode unless terminateDebuggee is explicitely set.
// Also tests suspendDebugggee option.
func TestAttachRemoteMultiClientDisconnect(t *testing.T) {
closingClientSessionOnly := fmt.Sprintf(daptest.ClosingClient, "halted")
detachingAndTerminating := "Detaching and terminating target process"
closingClientSessionOnlyRunning := fmt.Sprintf(daptest.ClosingClient, "running")
closingClientSessionOnlySuspended := fmt.Sprintf(daptest.ClosingClient, "suspended")
detachingAndTerminating := "Detaching and terminating target process\n"
tests := []struct {
name string
disconnectRequest func(client *daptest.Client)
expect string
}{
{"default", func(c *daptest.Client) { c.DisconnectRequest() }, closingClientSessionOnly},
{"terminate=true", func(c *daptest.Client) { c.DisconnectRequestWithKillOption(true) }, detachingAndTerminating},
{"terminate=false", func(c *daptest.Client) { c.DisconnectRequestWithKillOption(false) }, closingClientSessionOnly},
{"default", func(c *daptest.Client) { c.DisconnectRequest() }, closingClientSessionOnlyRunning},
{"terminate=false,suspend=false", func(c *daptest.Client) { c.DisconnectRequestWithOptions(false, false) }, closingClientSessionOnlyRunning},
{"terminate=false,suspend=true", func(c *daptest.Client) { c.DisconnectRequestWithOptions(false, true) }, closingClientSessionOnlySuspended},
{"terminate=true,suspend=false", func(c *daptest.Client) { c.DisconnectRequestWithOptions(true, false) }, detachingAndTerminating},
{"terminate=true,suspend=true", func(c *daptest.Client) { c.DisconnectRequestWithOptions(true, false) }, detachingAndTerminating},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
Expand All @@ -6730,7 +6735,7 @@ func TestAttachRemoteMultiClientDisconnect(t *testing.T) {
client.ExpectInitializeResponseAndCapabilities(t)

client.AttachRequest(map[string]interface{}{"mode": "remote", "stopOnEntry": true})
client.ExpectCapabilitiesEventSupportTerminateDebuggee(t)
client.ExpectCapabilitiesEventSupportDisconnectOptions(t, true, true)
client.ExpectInitializedEvent(t)
client.ExpectAttachResponse(t)
client.ConfigurationDoneRequest()
Expand All @@ -6746,13 +6751,9 @@ func TestAttachRemoteMultiClientDisconnect(t *testing.T) {
client.ExpectTerminatedEvent(t)
time.Sleep(10 * time.Millisecond) // give time for things to shut down

if tc.expect == closingClientSessionOnly {
// At this point a multi-client server is still running. but session should be done.
if e.Body.Output != detachingAndTerminating {
// At this point a multi-client server is still running, but session should be doone.
verifySessionStopped(t, server.impl.session)
// Verify target's running state.
if server.debugger.IsRunning() {
t.Errorf("\ngot running=true, want false")
}
server.stop(t)
}
<-server.stopped
Expand Down