From 694b45c8939efcc56320dfbe4bceb5274eb8b38a Mon Sep 17 00:00:00 2001 From: polinasok <51177946+polinasok@users.noreply.github.com> Date: Mon, 16 Aug 2021 08:51:23 -0700 Subject: [PATCH] service/dap: fix noDebug mode to handle requests while running (#2658) Co-authored-by: Polina Sokolova --- service/dap/daptest/client.go | 5 +++ service/dap/error_ids.go | 1 + service/dap/server.go | 61 ++++++++++++++++++--------------- service/dap/server_test.go | 64 ++++++++++++++++++++++++----------- 4 files changed, 84 insertions(+), 47 deletions(-) diff --git a/service/dap/daptest/client.go b/service/dap/daptest/client.go index 53ba1b25..8378b30b 100644 --- a/service/dap/daptest/client.go +++ b/service/dap/daptest/client.go @@ -156,6 +156,11 @@ func (c *Client) ExpectOutputEventDetachingNoKill(t *testing.T) *dap.OutputEvent return c.ExpectOutputEventRegex(t, `Detaching without terminating target process\n`) } +func (c *Client) ExpectOutputEventTerminating(t *testing.T) *dap.OutputEvent { + t.Helper() + return c.ExpectOutputEventRegex(t, `Terminating process [0-9]+\n`) +} + // InitializeRequest sends an 'initialize' request. func (c *Client) InitializeRequest() { request := &dap.InitializeRequest{Request: *c.newRequest("initialize")} diff --git a/service/dap/error_ids.go b/service/dap/error_ids.go index 23d9d3c1..07a4d770 100644 --- a/service/dap/error_ids.go +++ b/service/dap/error_ids.go @@ -25,6 +25,7 @@ const ( UnableToGetExceptionInfo = 2011 UnableToSetVariable = 2012 // Add more codes as we support more requests + NoDebugIsRunning = 3000 DebuggeeIsRunning = 4000 DisconnectError = 5000 ) diff --git a/service/dap/server.go b/service/dap/server.go index 0feaa231..78d2c764 100644 --- a/service/dap/server.go +++ b/service/dap/server.go @@ -411,6 +411,19 @@ func (s *Server) handleRequest(request dap.Message) { return } + if s.isNoDebug() { + switch request := request.(type) { + case *dap.DisconnectRequest: + s.onDisconnectRequest(request) + case *dap.RestartRequest: + s.sendUnsupportedErrorResponse(request.Request) + default: + r := request.(dap.RequestMessage).GetRequest() + s.sendErrorResponse(*r, NoDebugIsRunning, "noDebug mode", fmt.Sprintf("unable to process '%s' request", r.Command)) + } + return + } + // These requests, can be handled regardless of whether the targret is running switch request := request.(type) { case *dap.DisconnectRequest: @@ -926,7 +939,7 @@ func (s *Server) onLaunchRequest(request *dap.LaunchRequest) { s.log.Debugf("running program in %s\n", s.config.Debugger.WorkingDir) if noDebug, ok := request.Arguments["noDebug"].(bool); ok && noDebug { s.mu.Lock() - cmd, err := s.startNoDebugProcess(program, targetArgs, s.config.Debugger.WorkingDir) + cmd, err := s.newNoDebugProcess(program, targetArgs, s.config.Debugger.WorkingDir) s.mu.Unlock() if err != nil { s.sendErrorResponse(request.Request, FailedToLaunch, "Failed to launch", err.Error()) @@ -936,20 +949,22 @@ func (s *Server) onLaunchRequest(request *dap.LaunchRequest) { // debug-related requests. s.send(&dap.LaunchResponse{Response: *newResponse(request.Request)}) - // Then, block until the program terminates or is stopped. - if err := cmd.Wait(); err != nil { - s.log.Debugf("program exited with error: %v", err) - } - stopped := false - s.mu.Lock() - stopped = s.noDebugProcess == nil // if it was stopped, this should be nil. - s.noDebugProcess = nil - s.mu.Unlock() + // Start the program on a different goroutine, so we can listen for disconnect request. + go func() { + if err := cmd.Wait(); err != nil { + s.log.Debugf("program exited with error: %v", err) + } + stopped := false + s.mu.Lock() + stopped = s.noDebugProcess == nil // if it was stopped, this should be nil + s.noDebugProcess = nil + s.mu.Unlock() - if !stopped { - s.logToConsole(proc.ErrProcessExited{Pid: cmd.ProcessState.Pid(), Status: cmd.ProcessState.ExitCode()}.Error()) - s.send(&dap.TerminatedEvent{Event: *newEvent("terminated")}) - } + if !stopped { // process terminated on its own + s.logToConsole(proc.ErrProcessExited{Pid: cmd.ProcessState.Pid(), Status: cmd.ProcessState.ExitCode()}.Error()) + s.send(&dap.TerminatedEvent{Event: *newEvent("terminated")}) + } + }() return } @@ -974,9 +989,9 @@ func (s *Server) onLaunchRequest(request *dap.LaunchRequest) { s.send(&dap.LaunchResponse{Response: *newResponse(request.Request)}) } -// startNoDebugProcess is called from onLaunchRequest (run goroutine) and -// requires holding mu lock. -func (s *Server) startNoDebugProcess(program string, targetArgs []string, wd string) (*exec.Cmd, error) { +// newNoDebugProcess is called from onLaunchRequest (run goroutine) and +// requires holding mu lock. It prepares process exec.Cmd to be started. +func (s *Server) newNoDebugProcess(program string, targetArgs []string, wd string) (*exec.Cmd, error) { if s.noDebugProcess != nil { return nil, fmt.Errorf("another launch request is in progress") } @@ -996,7 +1011,7 @@ func (s *Server) stopNoDebugProcess() { // We already handled termination or there was never a process return } - if s.noDebugProcess.ProcessState.Exited() { + if s.noDebugProcess.ProcessState != nil && s.noDebugProcess.ProcessState.Exited() { s.logToConsole(proc.ErrProcessExited{Pid: s.noDebugProcess.ProcessState.Pid(), Status: s.noDebugProcess.ProcessState.ExitCode()}.Error()) } else { // TODO(hyangah): gracefully terminate the process and its children processes. @@ -1133,11 +1148,6 @@ func (s *Server) isNoDebug() bool { } func (s *Server) onSetBreakpointsRequest(request *dap.SetBreakpointsRequest) { - if s.isNoDebug() { - s.sendErrorResponse(request.Request, UnableToSetBreakpoints, "Unable to set or clear breakpoints", "running in noDebug mode") - return - } - if request.Arguments.Source.Path == "" { s.sendErrorResponse(request.Request, UnableToSetBreakpoints, "Unable to set or clear breakpoints", "empty file path") return @@ -1234,11 +1244,6 @@ func updateBreakpointsResponse(breakpoints []dap.Breakpoint, i int, err error, g const functionBpPrefix = "functionBreakpoint" func (s *Server) onSetFunctionBreakpointsRequest(request *dap.SetFunctionBreakpointsRequest) { - if s.noDebugProcess != nil { - s.sendErrorResponse(request.Request, UnableToSetBreakpoints, "Unable to set or clear breakpoints", "running in noDebug mode") - return - } - // According to the spec, setFunctionBreakpoints "replaces all existing function // breakpoints with new function breakpoints." The simplest way is // to clear all and then set all. To maintain state (for hit count conditions) diff --git a/service/dap/server_test.go b/service/dap/server_test.go index 678063b4..ee73e1f7 100644 --- a/service/dap/server_test.go +++ b/service/dap/server_test.go @@ -4125,48 +4125,74 @@ func TestLaunchRequestDefaults(t *testing.T) { }) } -func TestLaunchRequestNoDebug_GoodStatus(t *testing.T) { +func TestNoDebug_GoodExitStatus(t *testing.T) { runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) { - runNoDebugDebugSession(t, client, func() { + runNoDebugSession(t, client, func() { client.LaunchRequestWithArgs(map[string]interface{}{ - "noDebug": true, - "mode": "debug", - "program": fixture.Source, - "output": "__mybin"}) - }, fixture.Source, []int{8}, 0) + "noDebug": true, "mode": "debug", "program": fixture.Source, "output": "__mybin"}) + }, 0) }) } -func TestLaunchRequestNoDebug_BadStatus(t *testing.T) { +func TestNoDebug_BadExitStatus(t *testing.T) { runTest(t, "issue1101", func(client *daptest.Client, fixture protest.Fixture) { - runNoDebugDebugSession(t, client, func() { + runNoDebugSession(t, client, func() { client.LaunchRequestWithArgs(map[string]interface{}{ - "noDebug": true, - "mode": "debug", - "program": fixture.Source, - "output": "__mybin"}) - }, fixture.Source, []int{8}, 2) + "noDebug": true, "mode": "exec", "program": fixture.Path}) + }, 2) }) } -// runNoDebugDebugSession tests the session started with noDebug=true runs uninterrupted -// even when breakpoint is set. -func runNoDebugDebugSession(t *testing.T, client *daptest.Client, cmdRequest func(), source string, breakpoints []int, status int) { +// runNoDebugSession tests the session started with noDebug=true runs +// to completion and logs termination status. +func runNoDebugSession(t *testing.T, client *daptest.Client, launchRequest func(), exitStatus int) { client.InitializeRequest() client.ExpectInitializeResponseAndCapabilities(t) - cmdRequest() + launchRequest() // no initialized event. // noDebug mode applies only to "launch" requests. client.ExpectLaunchResponse(t) - client.ExpectOutputEventProcessExited(t, status) + client.ExpectOutputEventProcessExited(t, exitStatus) client.ExpectTerminatedEvent(t) client.DisconnectRequestWithKillOption(true) client.ExpectDisconnectResponse(t) client.ExpectTerminatedEvent(t) } +func TestNoDebug_AcceptNoRequestsButDisconnect(t *testing.T) { + runTest(t, "http_server", func(client *daptest.Client, fixture protest.Fixture) { + client.InitializeRequest() + client.ExpectInitializeResponseAndCapabilities(t) + client.LaunchRequestWithArgs(map[string]interface{}{ + "noDebug": true, "mode": "exec", "program": fixture.Path}) + client.ExpectLaunchResponse(t) + + // Anything other than disconnect should get rejected + var ExpectNoDebugError = func(cmd string) { + er := client.ExpectErrorResponse(t) + if er.Body.Error.Format != fmt.Sprintf("noDebug mode: unable to process '%s' request", cmd) { + t.Errorf("\ngot %#v\nwant 'noDebug mode: unable to process '%s' request'", er, cmd) + } + } + client.SetBreakpointsRequest(fixture.Source, []int{8}) + ExpectNoDebugError("setBreakpoints") + client.SetFunctionBreakpointsRequest(nil) + ExpectNoDebugError("setFunctionBreakpoints") + client.PauseRequest(1) + ExpectNoDebugError("pause") + client.RestartRequest() + client.ExpectUnsupportedCommandErrorResponse(t) + + // Disconnect request is ok + client.DisconnectRequestWithKillOption(true) + client.ExpectOutputEventTerminating(t) + client.ExpectDisconnectResponse(t) + client.ExpectTerminatedEvent(t) + }) +} + func TestLaunchTestRequest(t *testing.T) { runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) { runDebugSession(t, client, "launch", func() {