Repository: go-delve/delve
Pull request #2677 (2021-08-27 08:13:44)

service/rpccommon: halt before detach in Stop
Context: https://github.com/go-delve/delve/pull/2648#issuecomment-906954585


Patch:
diff --git a/pkg/config/config.go b/pkg/config/config.go
index f1a24e46..83ef688d 100644
--- a/pkg/config/config.go
+++ b/pkg/config/config.go
@@ -53,9 +53,8 @@ type Config struct {
 	// expression for its argument.
 	ShowLocationExpr bool `yaml:"show-location-expr"`
 
-	// Source list line-number color (3/4 bit color codes as defined
-	// here: https://en.wikipedia.org/wiki/ANSI_escape_code#Colors),
-	// or a string containing a terminal escape sequence.
+	// Source list line-number color, as a terminal escape sequence.
+	// For historic reasons, this can also be an integer color code.
 	SourceListLineColor interface{} `yaml:"source-list-line-color"`
 
 	// Source list arrow color, as a terminal escape sequence.
@@ -228,11 +227,10 @@ func writeDefaultConfig(f *os.File) error {
 # This is the default configuration file. Available options are provided, but disabled.
 # Delete the leading hash mark to enable an item.
 
-# Uncomment the following line and set your preferred ANSI foreground color
-# for source line numbers in the (list) command (if unset, default is 34,
-# dark blue) See https://en.wikipedia.org/wiki/ANSI_escape_code#3/4_bit
-# Alternatively a string containing an escape sequence can also be used.
-# source-list-line-color: 34
+# Uncomment the following line and set your preferred ANSI color for source
+# line numbers in the (list) command. The default is 34 (dark blue). See
+# https://en.wikipedia.org/wiki/ANSI_escape_code#3/4_bit
+# source-list-line-color: "\x1b[34m"
 
 # Uncomment the following lines to change the colors used by syntax highlighting.
 # source-list-keyword-color: "\x1b[0m"
diff --git a/pkg/terminal/config.go b/pkg/terminal/config.go
index 3bcda96e..2d1ef51e 100644
--- a/pkg/terminal/config.go
+++ b/pkg/terminal/config.go
@@ -82,6 +82,13 @@ func configureList(t *Term) error {
 		}
 
 		switch field.Kind() {
+		case reflect.Interface:
+			switch field := field.Interface().(type) {
+			case string:
+				fmt.Fprintf(w, "%s\t%q\n", fieldName, field)
+			default:
+				fmt.Fprintf(w, "%s\t%v\n", fieldName, field)
+			}
 		case reflect.Ptr:
 			if !field.IsNil() {
 				fmt.Fprintf(w, "%s\t%v\n", fieldName, field.Elem())
diff --git a/service/dap/server.go b/service/dap/server.go
index 3e90c430..b2edbdb3 100644
--- a/service/dap/server.go
+++ b/service/dap/server.go
@@ -193,7 +193,11 @@ const (
 	// what is presented. A common use case of a call injection is to
 	// stringify complex data conveniently.
 	maxStringLenInCallRetVars = 1 << 10 // 1024
+)
+
+var (
 	// Max number of goroutines that we will return.
+	// This is a var for testing
 	maxGoroutines = 1 << 10
 )
 
@@ -1358,8 +1362,8 @@ func (s *Server) onThreadsRequest(request *dap.ThreadsRequest) {
 	if s.debugger != nil {
 		gs, next, err = s.debugger.Goroutines(0, maxGoroutines)
 	}
-	threads := make([]dap.Thread, len(gs))
 
+	var threads []dap.Thread
 	if err != nil {
 		switch err.(type) {
 		case proc.ErrProcessExited:
@@ -1375,16 +1379,41 @@ func (s *Server) onThreadsRequest(request *dap.ThreadsRequest) {
 				}})
 		}
 		threads = []dap.Thread{{Id: 1, Name: "Dummy"}}
-	} else if len(threads) == 0 {
+	} else if len(gs) == 0 {
 		threads = []dap.Thread{{Id: 1, Name: "Dummy"}}
 	} else {
-		if next >= 0 {
-			s.logToConsole(fmt.Sprintf("Too many goroutines, only loaded %d", len(gs)))
-		}
 		state, err := s.debugger.State( /*nowait*/ true)
 		if err != nil {
 			s.log.Debug("Unable to get debugger state: ", err)
 		}
+
+		if next >= 0 {
+			s.logToConsole(fmt.Sprintf("Too many goroutines, only loaded %d", len(gs)))
+
+			// Make sure the selected goroutine is included in the list of threads
+			// to return.
+			if state != nil && state.SelectedGoroutine != nil {
+				var selectedFound bool
+				for _, g := range gs {
+					if g.ID == state.SelectedGoroutine.ID {
+						selectedFound = true
+						break
+					}
+				}
+				if !selectedFound {
+					g, err := s.debugger.FindGoroutine(state.SelectedGoroutine.ID)
+					if err != nil {
+						s.log.Debug("Error getting selected goroutine: ", err)
+					} else {
+						// TODO(suzmue): Consider putting the selected goroutine at the top.
+						// To be consistent we may want to do this for all threads requests.
+						gs = append(gs, g)
+					}
+				}
+			}
+		}
+
+		threads = make([]dap.Thread, len(gs))
 		s.debugger.LockTarget()
 		defer s.debugger.UnlockTarget()
 
diff --git a/service/rpccommon/server.go b/service/rpccommon/server.go
index e31bd4f1..ff96cfe4 100644
--- a/service/rpccommon/server.go
+++ b/service/rpccommon/server.go
@@ -90,10 +90,14 @@ func NewServer(config *service.Config) *ServerImpl {
 
 // Stop stops the JSON-RPC server.
 func (s *ServerImpl) Stop() error {
+	s.log.Debug("stopping")
 	close(s.stopChan)
 	if s.config.AcceptMulti {
 		s.listener.Close()
 	}
+	if s.debugger.IsRunning() {
+		s.debugger.Command(&api.DebuggerCommand{Name: api.Halt}, nil)
+	}
 	kill := s.config.Debugger.AttachPid == 0
 	return s.debugger.Detach(kill)
 }


Test patch:
diff --git a/cmd/dlv/dlv_test.go b/cmd/dlv/dlv_test.go
index fcc81e31..64778746 100644
--- a/cmd/dlv/dlv_test.go
+++ b/cmd/dlv/dlv_test.go
@@ -254,7 +254,7 @@ func TestContinue(t *testing.T) {
 	cmd := exec.Command(dlvbin, "debug", "--headless", "--continue", "--accept-multiclient", "--listen", listenAddr)
 	cmd.Dir = buildtestdir
 	stdout, err := cmd.StdoutPipe()
-	assertNoError(err, t, "stderr pipe")
+	assertNoError(err, t, "stdout pipe")
 	defer stdout.Close()
 
 	assertNoError(cmd.Start(), t, "start headless instance")
@@ -299,7 +299,7 @@ func TestChildProcessExitWhenNoDebugInfo(t *testing.T) {
 	// search the running process named fix.Name
 	cmd := exec.Command("ps", "-aux")
 	stdout, err := cmd.StdoutPipe()
-	assertNoError(err, t, "stderr pipe")
+	assertNoError(err, t, "stdout pipe")
 	defer stdout.Close()
 
 	assertNoError(cmd.Start(), t, "start `ps -aux`")
@@ -330,7 +330,7 @@ func TestRedirect(t *testing.T) {
 	catfixture := filepath.Join(protest.FindFixturesDir(), "cat.go")
 	cmd := exec.Command(dlvbin, "debug", "--headless", "--continue", "--accept-multiclient", "--listen", listenAddr, "-r", catfixture, catfixture)
 	stdout, err := cmd.StdoutPipe()
-	assertNoError(err, t, "stderr pipe")
+	assertNoError(err, t, "stdout pipe")
 	defer stdout.Close()
 
 	assertNoError(cmd.Start(), t, "start headless instance")
diff --git a/pkg/proc/proc_unix_test.go b/pkg/proc/proc_unix_test.go
index 5d7e2f85..74a39cc6 100644
--- a/pkg/proc/proc_unix_test.go
+++ b/pkg/proc/proc_unix_test.go
@@ -1,3 +1,4 @@
+//go:build linux || darwin
 // +build linux darwin
 
 package proc_test
@@ -36,7 +37,6 @@ func TestIssue419(t *testing.T) {
 
 	// SIGINT directed at the inferior should be passed along not swallowed by delve
 	withTestProcess("issue419", t, func(p *proc.Target, fixture protest.Fixture) {
-		defer close(errChan)
 		setFunctionBreakpoint(p, t, "main.main")
 		assertNoError(p.Continue(), t, "Continue()")
 		resumeChan := make(chan struct{}, 1)
diff --git a/service/dap/server_test.go b/service/dap/server_test.go
index 080768a9..ca7b3f4d 100644
--- a/service/dap/server_test.go
+++ b/service/dap/server_test.go
@@ -54,21 +54,27 @@ func runTestBuildFlags(t *testing.T, name string, test func(c *daptest.Client, f
 	fixture := protest.BuildFixture(name, buildFlags)
 
 	// Start the DAP server.
-	client := startDapServer(t)
-	// client.Close will close the client connectinon, which will cause a connection error
-	// on the server side and signal disconnect to unblock Stop() above.
+	serverStopped := make(chan struct{})
+	client := startDapServerWithClient(t, serverStopped)
 	defer client.Close()
 
 	test(client, fixture)
+	<-serverStopped
 }
 
-func startDapServer(t *testing.T) *daptest.Client {
+func startDapServerWithClient(t *testing.T, serverStopped chan struct{}) *daptest.Client {
+	listener, _ := startDapServer(t, serverStopped)
+	client := daptest.NewClient(listener.Addr().String())
+	return client
+}
+
+func startDapServer(t *testing.T, serverStopped chan struct{}) (listener net.Listener, disconnectChan chan struct{}) {
 	// Start the DAP server.
 	listener, err := net.Listen("tcp", ":0")
 	if err != nil {
 		t.Fatal(err)
 	}
-	disconnectChan := make(chan struct{})
+	disconnectChan = make(chan struct{})
 	server := NewServer(&service.Config{
 		Listener:       listener,
 		DisconnectChan: disconnectChan,
@@ -81,12 +87,51 @@ func startDapServer(t *testing.T) *daptest.Client {
 	// This helps us test that certain events cause the server to stop as
 	// expected.
 	go func() {
+		defer func() {
+			if serverStopped != nil {
+				close(serverStopped)
+			}
+		}()
 		<-disconnectChan
 		server.Stop()
 	}()
 
+	return listener, disconnectChan
+}
+
+func TestForceQuitNoClient(t *testing.T) {
+	serverStopped := make(chan struct{})
+	_, disconnectChan := startDapServer(t, serverStopped)
+	close(disconnectChan) // trigger server.Stop()
+	<-serverStopped
+}
+
+func TestForceQuitNoTarget(t *testing.T) {
+	serverStopped := make(chan struct{})
+	listener, disconnectChan := startDapServer(t, serverStopped)
 	client := daptest.NewClient(listener.Addr().String())
-	return client
+	defer client.Close()
+
+	client.InitializeRequest()
+	client.ExpectInitializeResponseAndCapabilities(t)
+	close(disconnectChan)
+	<-serverStopped
+}
+
+func TestForceQuitWithTarget(t *testing.T) {
+	serverStopped := make(chan struct{})
+	listener, disconnectChan := startDapServer(t, serverStopped)
+	client := daptest.NewClient(listener.Addr().String())
+	defer client.Close()
+
+	client.InitializeRequest()
+	client.ExpectInitializeResponseAndCapabilities(t)
+	fixture := protest.BuildFixture("increment", protest.AllNonOptimized)
+	client.LaunchRequest("exec", fixture.Path, stopOnEntry)
+	client.ExpectInitializedEvent(t)
+	client.ExpectLaunchResponse(t)
+	close(disconnectChan)
+	<-serverStopped
 }
 
 // TestLaunchStopOnEntry emulates the message exchange that can be observed with
@@ -922,6 +967,63 @@ func TestStackTraceRequest(t *testing.T) {
 	})
 }
 
+func TestSelectedThreadsRequest(t *testing.T) {
+	runTest(t, "goroutinestackprog", func(client *daptest.Client, fixture protest.Fixture) {
+		runDebugSessionWithBPs(t, client, "launch",
+			// Launch
+			func() {
+				client.LaunchRequest("exec", fixture.Path, !stopOnEntry)
+			},
+			// Set breakpoints
+			fixture.Source, []int{20},
+			[]onBreakpoint{{
+				execute: func() {
+					checkStop(t, client, 1, "main.main", 20)
+
+					defaultMaxGoroutines := maxGoroutines
+					defer func() { maxGoroutines = defaultMaxGoroutines }()
+
+					maxGoroutines = 1
+					client.SetBreakpointsRequest(fixture.Source, []int{8})
+					client.ExpectSetBreakpointsResponse(t)
+
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+
+					se := client.ExpectStoppedEvent(t)
+					if se.Body.Reason != "breakpoint" || se.Body.ThreadId == 1 {
+						t.Errorf("got %#v, want Reason=%q, ThreadId!=1", se, "breakpoint")
+					}
+
+					client.ThreadsRequest()
+					oe := client.ExpectOutputEvent(t)
+					if !strings.HasPrefix(oe.Body.Output, "Too many goroutines") {
+						t.Errorf("got %#v, expected Output=\"Too many goroutines...\"\n", oe)
+
+					}
+					tr := client.ExpectThreadsResponse(t)
+
+					if len(tr.Body.Threads) != 2 {
+						t.Errorf("got %d threads, expected 2\n", len(tr.Body.Threads))
+					}
+
+					var selectedFound bool
+					for _, thread := range tr.Body.Threads {
+						if thread.Id == se.Body.ThreadId {
+							selectedFound = true
+							break
+						}
+					}
+					if !selectedFound {
+						t.Errorf("got %#v, want ThreadId=%d\n", tr.Body.Threads, se.Body.ThreadId)
+					}
+				},
+				disconnect: true,
+			}})
+
+	})
+}
+
 // TestScopesAndVariablesRequests executes to a breakpoint and tests different
 // configurations of 'scopes' and 'variables' requests.
 func TestScopesAndVariablesRequests(t *testing.T) {
@@ -4215,8 +4317,9 @@ func TestNoDebug_AcceptNoRequestsButDisconnect(t *testing.T) {
 }
 
 func TestLaunchRequestWithRelativeBuildPath(t *testing.T) {
-	client := startDapServer(t)
-	defer client.Close() // will trigger Stop()
+	serverStopped := make(chan struct{})
+	client := startDapServerWithClient(t, serverStopped)
+	defer client.Close()
 
 	fixdir := protest.FindFixturesDir()
 	if filepath.IsAbs(fixdir) {
@@ -4231,6 +4334,7 @@ func TestLaunchRequestWithRelativeBuildPath(t *testing.T) {
 		client.LaunchRequestWithArgs(map[string]interface{}{
 			"mode": "debug", "program": program, "cwd": filepath.Dir(dlvwd)})
 	})
+	<-serverStopped
 }
 
 func TestLaunchRequestWithRelativeExecPath(t *testing.T) {
@@ -4249,14 +4353,16 @@ func TestLaunchRequestWithRelativeExecPath(t *testing.T) {
 }
 
 func TestLaunchTestRequest(t *testing.T) {
-	client := startDapServer(t)
-	defer client.Close() // will trigger Stop()
+	serverStopped := make(chan struct{})
+	client := startDapServerWithClient(t, serverStopped)
+	defer client.Close()
 	runDebugSession(t, client, "launch", func() {
 		fixtures := protest.FindFixturesDir()
 		testdir, _ := filepath.Abs(filepath.Join(fixtures, "buildtest"))
 		client.LaunchRequestWithArgs(map[string]interface{}{
 			"mode": "test", "program": testdir, "output": "__mytestdir"})
 	})
+	<-serverStopped
 }
 
 // Tests that 'args' from LaunchRequest are parsed and passed to the target
@@ -4412,6 +4518,9 @@ func TestUnupportedCommandResponses(t *testing.T) {
 
 		client.ModulesRequest()
 		expectUnsupportedCommand("modules")
+
+		client.DisconnectRequest()
+		client.ExpectDisconnectResponse(t)
 	})
 }
 
@@ -4816,6 +4925,9 @@ func TestOptionalNotYetImplementedResponses(t *testing.T) {
 
 		client.CancelRequest()
 		expectNotYetImplemented("cancel")
+
+		client.DisconnectRequest()
+		client.ExpectDisconnectResponse(t)
 	})
 }
 
@@ -5127,9 +5239,8 @@ func TestBadInitializeRequest(t *testing.T) {
 		t.Helper()
 		// Only one initialize request is allowed, so use a new server
 		// for each test.
-		client := startDapServer(t)
-		// client.Close will close the client connectinon, which will cause a connection error
-		// on the server side and signal disconnect to unblock Stop() above.
+		serverStopped := make(chan struct{})
+		client := startDapServerWithClient(t, serverStopped)
 		defer client.Close()
 
 		client.InitializeRequestWithArgs(args)
@@ -5146,6 +5257,10 @@ func TestBadInitializeRequest(t *testing.T) {
 		if response.Body.Error.Format != err {
 			t.Errorf("\ngot  %q\nwant %q", response.Body.Error.Format, err)
 		}
+
+		client.DisconnectRequest()
+		client.ExpectDisconnectResponse(t)
+		<-serverStopped
 	}
 
 	// Bad path format.
@@ -5207,5 +5322,8 @@ func TestBadlyFormattedMessageToServer(t *testing.T) {
 		// Make sure that the unknown request did not kill the server.
 		client.InitializeRequest()
 		client.ExpectInitializeResponse(t)
+
+		client.DisconnectRequest()
+		client.ExpectDisconnectResponse(t)
 	})
 }
diff --git a/service/test/integration2_test.go b/service/test/integration2_test.go
index e3bad857..f332b9b4 100644
--- a/service/test/integration2_test.go
+++ b/service/test/integration2_test.go
@@ -1892,6 +1892,39 @@ func TestAcceptMulticlient(t *testing.T) {
 	<-serverDone
 }
 
+func TestForceStopWhileContinue(t *testing.T) {
+	listener, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("couldn't start listener: %s\n", err)
+	}
+	serverStopped := make(chan struct{})
+	disconnectChan := make(chan struct{})
+	go func() {
+		defer close(serverStopped)
+		defer listener.Close()
+		server := rpccommon.NewServer(&service.Config{
+			Listener:       listener,
+			ProcessArgs:    []string{protest.BuildFixture("http_server", protest.AllNonOptimized).Path},
+			AcceptMulti:    true,
+			DisconnectChan: disconnectChan,
+			Debugger: debugger.Config{
+				Backend: "default",
+			},
+		})
+		if err := server.Run(); err != nil {
+			panic(err)
+		}
+		<-disconnectChan
+		server.Stop()
+	}()
+
+	client := rpc2.NewClient(listener.Addr().String())
+	client.Disconnect(true /*continue*/)
+	time.Sleep(10 * time.Millisecond) // give server time to start running
+	close(disconnectChan)             // stop the server
+	<-serverStopped                   // Stop() didn't block on detach because we halted first
+}
+
 func TestClientServerFunctionCall(t *testing.T) {
 	protest.MustSupportFunctionCalls(t, testBackend)
 	withTestClient2("fncall", t, func(c service.Client) {