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

@@ -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)
 	}
 }

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()