From d0898e4de13e4ac3dcade0f10b774cb7d4fe3a9c Mon Sep 17 00:00:00 2001 From: polinasok <51177946+polinasok@users.noreply.github.com> Date: Tue, 7 Dec 2021 09:23:26 -0800 Subject: [PATCH] service/dap: misc remote attach improvements (#2778) Co-authored-by: Polina Sokolova --- cmd/dlv/dlv_test.go | 15 ++++++++++----- service/dap/daptest/client.go | 6 ++++-- service/dap/server.go | 25 +++++++++++++++++++++---- service/dap/server_test.go | 18 +++++++++--------- service/dap/types.go | 1 + 5 files changed, 45 insertions(+), 20 deletions(-) diff --git a/cmd/dlv/dlv_test.go b/cmd/dlv/dlv_test.go index 9bd56606..e9f101fa 100644 --- a/cmd/dlv/dlv_test.go +++ b/cmd/dlv/dlv_test.go @@ -750,9 +750,9 @@ func TestRemoteDAPClient(t *testing.T) { cmd.Wait() } -func closeDAPRemoteMultiClient(t *testing.T, c *daptest.Client) { +func closeDAPRemoteMultiClient(t *testing.T, c *daptest.Client, expectStatus string) { c.DisconnectRequest() - c.ExpectOutputEventClosingClient(t) + c.ExpectOutputEventClosingClient(t, expectStatus) c.ExpectDisconnectResponse(t) c.ExpectTerminatedEvent(t) c.Close() @@ -787,6 +787,11 @@ func TestRemoteDAPClientMulti(t *testing.T) { } }() + // Client 0 connects but with the wrong attach request + dapclient0 := daptest.NewClient(listenAddr) + dapclient0.AttachRequest(map[string]interface{}{"mode": "local"}) + dapclient0.ExpectErrorResponse(t) + // Client 1 connects and continues to main.main dapclient := newDAPRemoteClient(t, listenAddr) dapclient.SetFunctionBreakpointsRequest([]godap.FunctionBreakpoint{{Name: "main.main"}}) @@ -795,7 +800,7 @@ func TestRemoteDAPClientMulti(t *testing.T) { dapclient.ExpectContinueResponse(t) dapclient.ExpectStoppedEvent(t) dapclient.CheckStopLocation(t, 1, "main.main", 5) - closeDAPRemoteMultiClient(t, dapclient) + closeDAPRemoteMultiClient(t, dapclient, "halted") // Client 2 reconnects at main.main and continues to process exit dapclient2 := newDAPRemoteClient(t, listenAddr) @@ -803,13 +808,13 @@ func TestRemoteDAPClientMulti(t *testing.T) { dapclient2.ContinueRequest(1) dapclient2.ExpectContinueResponse(t) dapclient2.ExpectTerminatedEvent(t) - closeDAPRemoteMultiClient(t, dapclient2) + closeDAPRemoteMultiClient(t, dapclient2, "exited") // Attach to exited processs is an error dapclient3 := daptest.NewClient(listenAddr) dapclient3.AttachRequest(map[string]interface{}{"mode": "remote", "stopOnEntry": true}) dapclient3.ExpectErrorResponseWith(t, dap.FailedToAttach, `Process \d+ has exited with status 0`, true) - closeDAPRemoteMultiClient(t, dapclient3) + closeDAPRemoteMultiClient(t, dapclient3, "exited") // But rpc clients can still connect and restart rpcclient := rpc2.NewClient(listenAddr) diff --git a/service/dap/daptest/client.go b/service/dap/daptest/client.go index d3306cdf..42a623ca 100644 --- a/service/dap/daptest/client.go +++ b/service/dap/daptest/client.go @@ -178,9 +178,11 @@ func (c *Client) ExpectOutputEventTerminating(t *testing.T) *dap.OutputEvent { return c.ExpectOutputEventRegex(t, `Terminating process [0-9]+\n`) } -func (c *Client) ExpectOutputEventClosingClient(t *testing.T) *dap.OutputEvent { +const ClosingClient = "Closing client session, but leaving multi-client DAP server at .+:[0-9]+ with debuggee %s\n" + +func (c *Client) ExpectOutputEventClosingClient(t *testing.T, status string) *dap.OutputEvent { t.Helper() - return c.ExpectOutputEventRegex(t, `Closing client session, but leaving multi-client DAP server running at .+:[0-9]+\n`) + return c.ExpectOutputEventRegex(t, fmt.Sprintf(ClosingClient, status)) } func (c *Client) CheckStopLocation(t *testing.T, thread int, name string, line int) { diff --git a/service/dap/server.go b/service/dap/server.go index 6c86ad02..694d03b1 100644 --- a/service/dap/server.go +++ b/service/dap/server.go @@ -491,6 +491,16 @@ func (s *Server) RunWithClient(conn net.Conn) { go s.runSession(conn) } +func (s *Session) address() string { + if s.config.Listener != nil { + return s.config.Listener.Addr().String() + } + if netconn, ok := s.conn.(net.Conn); ok { + return netconn.LocalAddr().String() + } + return "" +} + // ServeDAPCodec reads and decodes requests from the client // until it encounters an error or EOF, when it sends // a disconnect signal and returns. @@ -934,8 +944,8 @@ func cleanExeName(name string) string { func (s *Session) onLaunchRequest(request *dap.LaunchRequest) { var err error if s.debugger != nil { - s.sendShowUserErrorResponse(request.Request, FailedToLaunch, - "Failed to launch", "debugger already started - use remote attach to connect to a server with an active debug session") + s.sendShowUserErrorResponse(request.Request, FailedToLaunch, "Failed to launch", + fmt.Sprintf("debug session already in progress at %s - use remote attach mode to connect to a server with an active debug session", s.address())) return } @@ -1178,7 +1188,13 @@ 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. - s.logToConsole("Closing client session, but leaving multi-client DAP server running at " + s.config.Listener.Addr().String()) + status := "halted" + if s.isRunningCmd() { + status = "running" + } else if s, err := s.debugger.State(false); processExited(s, 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() @@ -1752,7 +1768,8 @@ func (s *Session) onAttachRequest(request *dap.AttachRequest) { if s.debugger != nil { s.sendShowUserErrorResponse( request.Request, FailedToAttach, - "Failed to attach", "debugger already started - use remote mode to connect") + "Failed to attach", + fmt.Sprintf("debug session already in progress at %s - use remote mode to connect to a server with an active debug session", s.address())) return } if args.ProcessID == 0 { diff --git a/service/dap/server_test.go b/service/dap/server_test.go index de7e1066..6f723a4e 100644 --- a/service/dap/server_test.go +++ b/service/dap/server_test.go @@ -6358,10 +6358,10 @@ func TestAttachRemoteToRunningTargetContinueOnEntry(t *testing.T) { }) } -// TestMultiClient tests that that remote attach doesn't take down +// TestAttachRemoteMultiClientDisconnect tests that that remote attach doesn't take down // the server in multi-client mode unless terminateDebugee is explicitely set. -func TestAttachRemoteMultiClient(t *testing.T) { - closingClientSessionOnly := "Closing client session, but leaving multi-client DAP server running at" +func TestAttachRemoteMultiClientDisconnect(t *testing.T) { + closingClientSessionOnly := fmt.Sprintf(daptest.ClosingClient, "halted") detachingAndTerminating := "Detaching and terminating target process" tests := []struct { name string @@ -6441,18 +6441,18 @@ func TestLaunchAttachErrorWhenDebugInProgress(t *testing.T) { // Both launch and attach requests should go through for additional error checking client.AttachRequest(map[string]interface{}{"mode": "local", "processId": 100}) er := client.ExpectVisibleErrorResponse(t) - msg := "Failed to attach: debugger already started - use remote mode to connect" - if er.Body.Error.Id != FailedToAttach || er.Body.Error.Format != msg { - t.Errorf("got %#v, want Id=%d Format=%q", er, FailedToAttach, msg) + msgRe, _ := regexp.Compile("Failed to attach: debug session already in progress at [0-9]+:[0-9]+ - use remote mode to connect to a server with an active debug session") + if er.Body.Error.Id != FailedToAttach || msgRe.MatchString(er.Body.Error.Format) { + t.Errorf("got %#v, want Id=%d Format=%q", er, FailedToAttach, msgRe) } tests := []string{"debug", "test", "exec", "replay", "core"} for _, mode := range tests { t.Run(mode, func(t *testing.T) { client.LaunchRequestWithArgs(map[string]interface{}{"mode": mode}) er := client.ExpectVisibleErrorResponse(t) - msg := "Failed to launch: debugger already started - use remote attach to connect to a server with an active debug session" - if er.Body.Error.Id != FailedToLaunch || er.Body.Error.Format != msg { - t.Errorf("got %#v, want Id=%d Format=%q", er, FailedToLaunch, msg) + msgRe, _ := regexp.Compile("Failed to launch: debug session already in progress at [0-9]+:[0-9]+ - use remote attach mode to connect to a server with an active debug session") + if er.Body.Error.Id != FailedToLaunch || msgRe.MatchString(er.Body.Error.Format) { + t.Errorf("got %#v, want Id=%d Format=%q", er, FailedToLaunch, msgRe) } }) } diff --git a/service/dap/types.go b/service/dap/types.go index 3b442be9..2f3cfc4f 100644 --- a/service/dap/types.go +++ b/service/dap/types.go @@ -195,6 +195,7 @@ func (m *SubstitutePath) UnmarshalJSON(data []byte) error { type AttachConfig struct { // Acceptable values are: // "local": attaches to the local process with the given ProcessID. + // "remote": expects the debugger to already be running to "attach" to an in-progress debug session. // // Default is "local". Mode string `json:"mode"`