From da27e3421704a1ed02189f8197b2bfcebb9091b6 Mon Sep 17 00:00:00 2001 From: Hyang-Ah Hana Kim Date: Mon, 5 Apr 2021 14:44:02 -0400 Subject: [PATCH] dap: change how noDebug launch request is served (#2407) * dap: change how noDebug launch request is served PR #2400 added support for noDebug launch requests - that was done by directly starting the target program using os/exec.Cmd and blocking the launch request indefinitely until the program terminates or is stopped with a disconnect request (when dlv dap can support it). Even though the approach seemed to work for user, that adds an extra control flow and complexity to the codebase. This change takes a different approach to implement the noDebug launch feature. Instead of using os/exec.Cmd, this uses the existing debug launch path, but avoids setting breakpoints. Finally, the program will start upon receiving onConfigurationDoneRequest and blocks there until the program terminates. This simplifies the code. The DAP spec does not explicitly specify what to do about other requests. It seems like VSCode can issue evaluate requests or other requests after the configuration done request. Currently they are blocked because the program is in running state. We can consider checking s.noDebug and responding with an error in the future if we want/need to. See the log below for a typical DAP request/response sequence: 2021-03-29T01:42:53-04:00 debug layer=dap building binary at /Users/hakim/projects/s/__debug_bin 2021-03-29T01:42:55-04:00 info layer=debugger launching process with args: [/Users/hakim/projects/s/__debug_bin] 2021-03-29T01:42:55-04:00 debug layer=dap [-> to client]{"seq":0,"type":"event","event":"initialized"} 2021-03-29T01:42:55-04:00 debug layer=dap [-> to client]{"seq":0,"type":"response","request_seq":2,"success":true,"command":"launch"} 2021-03-29T01:42:55-04:00 debug layer=dap [<- from client]{"seq":3,"type":"request","command":"setBreakpoints","arguments":{"source":{"name":"main.go","path":"/Users/hakim/projects/s/main.go"},"breakpoints":[{"line":9}],"lines":[9]}} 2021-03-29T01:42:55-04:00 error layer=dap Unable to set or clear breakpoints: running in noDebug mode 2021-03-29T01:42:55-04:00 debug layer=dap [-> to client]{"seq":0,"type":"response","request_seq":3,"success":false,"command":"setBreakpoints","message":"Unable to set or clear breakpoints","body":{"error":{"id":2002,"format":"Unable to set or clear breakpoints: running in noDebug mode"}}} 2021-03-29T01:42:55-04:00 debug layer=dap [<- from client]{"seq":4,"type":"request","command":"configurationDone","arguments":{}} 2021-03-29T01:42:55-04:00 debug layer=dap [-> to client]{"seq":0,"type":"response","request_seq":4,"success":true,"command":"configurationDone"} 2021-03-29T01:42:55-04:00 debug layer=debugger continuing Hello 2021-03-29T01:43:00-04:00 debug layer=dap [-> to client]{"seq":0,"type":"event","event":"terminated","body":{}} 2021-03-29T01:43:00-04:00 debug layer=dap [<- from client]{"seq":5,"type":"request","command":"threads"} 2021-03-29T01:43:00-04:00 debug layer=dap [-> to client]{"seq":0,"type":"response","request_seq":5,"success":true,"command":"threads","body":{"threads":null}} 2021-03-29T01:43:00-04:00 debug layer=dap [<- from client]{"seq":6,"type":"request","command":"disconnect","arguments":{}} 2021-03-29T01:43:00-04:00 debug layer=dap [-> to client]{"seq":0,"type":"response","request_seq":6,"success":true,"command":"disconnect"} 2021-03-29T01:43:00-04:00 debug layer=debugger halting 2021-03-29T01:43:00-04:00 error layer=dap Process 27219 has exited with status 0 2021-03-29T01:43:00-04:00 debug layer=debugger detaching Updates https://github.com/golang/vscode-go/issues/1111 * service/dap: address polina's comments for noDebug logic Reworked so the noDebug launch request again blocks launch request handler after responding with the launch response. By skipping the initialized event, it should prevent well-behaving clients from sending any further requests. Currently, doesn't matter because the handler goroutine will be blocked until the program termination anyway. Placed mutex back, since I noticed that's the only way to prevent racing between Stop and the handler goroutine. The synchronization lotic (in particular, during teardown) should be revisited after asynchronous request support work is done. * dap: address more comments --- service/dap/server.go | 89 +++++++++++++++++++------------------- service/dap/server_test.go | 50 +++++++++++---------- 2 files changed, 70 insertions(+), 69 deletions(-) diff --git a/service/dap/server.go b/service/dap/server.go index c43828b4..39e74a82 100644 --- a/service/dap/server.go +++ b/service/dap/server.go @@ -166,6 +166,8 @@ func (s *Server) Stop() { s.log.Error(err) } } + } else { + s.stopNoDebugProcess() } } @@ -554,74 +556,66 @@ func (s *Server) onLaunchRequest(request *dap.LaunchRequest) { s.config.Debugger.WorkingDir = wdParsed } - noDebug, ok := request.Arguments["noDebug"] - if ok { - if v, ok := noDebug.(bool); ok && v { // noDebug == true - if err := s.runWithoutDebug(program, targetArgs, s.config.Debugger.WorkingDir); err != nil { - s.sendErrorResponse(request.Request, FailedToLaunch, "Failed to launch", err.Error()) - } else { // program terminated. - s.send(&dap.TerminatedEvent{Event: *newEvent("terminated")}) - } + if noDebug, ok := request.Arguments["noDebug"].(bool); ok && noDebug { + cmd, err := s.startNoDebugProcess(program, targetArgs, s.config.Debugger.WorkingDir) + if err != nil { + s.sendErrorResponse(request.Request, FailedToLaunch, "Failed to launch", err.Error()) return } + // Skip 'initialized' event, which will prevent the client from sending + // 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: %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.send(&dap.TerminatedEvent{Event: *newEvent("terminated")}) + } + return } + var err error if s.debugger, err = debugger.New(&s.config.Debugger, s.config.ProcessArgs); err != nil { - s.sendErrorResponse(request.Request, - FailedToLaunch, "Failed to launch", err.Error()) + s.sendErrorResponse(request.Request, FailedToLaunch, "Failed to launch", 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'. s.send(&dap.InitializedEvent{Event: *newEvent("initialized")}) s.send(&dap.LaunchResponse{Response: *newResponse(request.Request)}) } -func (s *Server) runWithoutDebug(program string, targetArgs []string, wd string) error { - s.log.Debugln("Running without debug: ", program) - - cmd := exec.Command(program, targetArgs...) - // TODO: send stdin/out/err as OutputEvent messages - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr - cmd.Stdin = os.Stdin - cmd.Dir = s.config.Debugger.WorkingDir - +func (s *Server) startNoDebugProcess(program string, targetArgs []string, wd string) (*exec.Cmd, error) { s.mu.Lock() defer s.mu.Unlock() if s.noDebugProcess != nil { - return fmt.Errorf("previous process (pid=%v) is still active", s.noDebugProcess.Process.Pid) + return nil, fmt.Errorf("another launch request is in progress") } + cmd := exec.Command(program, targetArgs...) + cmd.Stdout, cmd.Stderr, cmd.Stdin, cmd.Dir = os.Stdout, os.Stderr, os.Stdin, wd if err := cmd.Start(); err != nil { - return err + return nil, err } s.noDebugProcess = cmd - s.mu.Unlock() // allow disconnect or restart requests to call stopNoDebugProcess. - - // block until the process terminates. - if err := cmd.Wait(); err != nil { - s.log.Debugf("process exited with %v", err) - } - - s.mu.Lock() - if s.noDebugProcess == cmd { - s.noDebugProcess = nil - } - return nil // Program ran and terminated. + return cmd, nil } func (s *Server) stopNoDebugProcess() { s.mu.Lock() + p := s.noDebugProcess + s.noDebugProcess = nil defer s.mu.Unlock() - if s.noDebugProcess == nil { - return - } // TODO(hyangah): gracefully terminate the process and its children processes. - if err := s.noDebugProcess.Process.Kill(); err != nil { - s.log.Debugf("killing process (pid=%v) failed: %v", s.noDebugProcess.Process.Pid, err) + if p != nil && !p.ProcessState.Exited() { + p.Process.Kill() // Don't check error. Process killing and self-termination may race. } } @@ -670,11 +664,16 @@ func (s *Server) onDisconnectRequest(request *dap.DisconnectRequest) { } else { s.stopNoDebugProcess() } + // TODO(polina): make thread-safe when handlers become asynchronous. s.signalDisconnect() } func (s *Server) onSetBreakpointsRequest(request *dap.SetBreakpointsRequest) { + if s.noDebugProcess != nil { + s.sendErrorResponse(request.Request, UnableToSetBreakpoints, "Unable to set or clear breakpoints", "running in noDebug mode") + return + } // TODO(polina): handle this while running by halting first. if request.Arguments.Source.Path == "" { @@ -738,7 +737,7 @@ func (s *Server) onSetExceptionBreakpointsRequest(request *dap.SetExceptionBreak } func (s *Server) onConfigurationDoneRequest(request *dap.ConfigurationDoneRequest) { - if s.args.stopOnEntry { + if s.debugger != nil && s.args.stopOnEntry { e := &dap.StoppedEvent{ Event: *newEvent("stopped"), Body: dap.StoppedEventBody{Reason: "entry", ThreadId: 1, AllThreadsStopped: true}, @@ -746,7 +745,7 @@ func (s *Server) onConfigurationDoneRequest(request *dap.ConfigurationDoneReques s.send(e) } s.send(&dap.ConfigurationDoneResponse{Response: *newResponse(request.Request)}) - if !s.args.stopOnEntry { + if s.debugger != nil && !s.args.stopOnEntry { s.doCommand(api.Continue) } } diff --git a/service/dap/server_test.go b/service/dap/server_test.go index 261416a8..6ae40e08 100644 --- a/service/dap/server_test.go +++ b/service/dap/server_test.go @@ -2549,48 +2549,42 @@ func TestLaunchRequestDefaults(t *testing.T) { // writes to default output dir __debug_bin }, fixture.Source) }) -} -func TestLaunchRequestDefaultsNoDebug(t *testing.T) { + // if noDebug is not a bool, behave as if it is the default value (false). runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) { - runNoDebugDebugSession(t, client, "launch", func() { + runDebugSession(t, client, "launch", func() { client.LaunchRequestWithArgs(map[string]interface{}{ - "noDebug": true, - "mode": "", /*"debug" by default*/ - "program": fixture.Source, - "output": cleanExeName("__mybin")}) + "mode": "debug", "program": fixture.Source, "noDebug": "true"}) }, fixture.Source) }) +} + +func TestLaunchRequestNoDebug(t *testing.T) { runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) { - runNoDebugDebugSession(t, client, "launch", func() { - // Use the default output directory. + runNoDebugDebugSession(t, client, func() { client.LaunchRequestWithArgs(map[string]interface{}{ "noDebug": true, - /*"mode":"debug" by default*/ + "mode": "", /*"debug" by default*/ "program": fixture.Source, "output": cleanExeName("__mybin")}) - }, fixture.Source) - }) - runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) { - runNoDebugDebugSession(t, client, "launch", func() { - // Use the default output directory. - client.LaunchRequestWithArgs(map[string]interface{}{ - "noDebug": true, - "mode": "debug", - "program": fixture.Source}) - // writes to default output dir __debug_bin - }, fixture.Source) + }, fixture.Source, []int{8}) }) } -func runNoDebugDebugSession(t *testing.T, client *daptest.Client, cmd string, cmdRequest func(), source string) { +// 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) { client.InitializeRequest() client.ExpectInitializeResponse(t) cmdRequest() - // ! client.InitializedEvent. - // ! client.ExpectLaunchResponse + // no initialized event. + // noDebug mode applies only to "launch" requests. + client.ExpectLaunchResponse(t) + client.ExpectTerminatedEvent(t) + client.DisconnectRequestWithKillOption(true) + client.ExpectDisconnectResponse(t) } func TestLaunchTestRequest(t *testing.T) { @@ -2896,6 +2890,14 @@ func TestBadLaunchRequests(t *testing.T) { client.LaunchRequestWithArgs(map[string]interface{}{"mode": "debug", "program": fixture.Source, "buildFlags": "bad flags"}) expectFailedToLaunchWithMessage(client.ExpectErrorResponse(t), "Failed to launch: Build error: exit status 1") + client.LaunchRequestWithArgs(map[string]interface{}{"mode": "debug", "program": fixture.Source, "noDebug": true, "buildFlags": "bad flags"}) + expectFailedToLaunchWithMessage(client.ExpectErrorResponse(t), "Failed to launch: Build error: exit status 1") + + // Bad "wd". + client.LaunchRequestWithArgs(map[string]interface{}{"mode": "debug", "program": fixture.Source, "noDebug": false, "wd": "dir/invalid"}) + expectFailedToLaunch(client.ExpectErrorResponse(t)) // invalid directory, the error message is system-dependent. + client.LaunchRequestWithArgs(map[string]interface{}{"mode": "debug", "program": fixture.Source, "noDebug": true, "wd": "dir/invalid"}) + expectFailedToLaunch(client.ExpectErrorResponse(t)) // invalid directory, the error message is system-dependent. // We failed to launch the program. Make sure shutdown still works. client.DisconnectRequest() -- GitLab