diff --git a/service/dap/server.go b/service/dap/server.go index b7c1c7c1..46ddfc16 100644 --- a/service/dap/server.go +++ b/service/dap/server.go @@ -291,6 +291,9 @@ func NewSession(conn io.ReadWriteCloser, config *Config, debugger *debugger.Debu config.log = logflags.DAPLogger() } config.log.Debug("DAP connection started") + if config.StopTriggered == nil { + config.log.Fatal("Session must be configured with StopTriggered") + } return &Session{ config: config, conn: conn, @@ -350,6 +353,7 @@ func (s *Server) Stop() { } // Close closes the underlying debugger/process and connection. +// May be called more than once. func (s *Session) Close() { s.mu.Lock() defer s.mu.Unlock() @@ -357,12 +361,13 @@ func (s *Session) Close() { if s.debugger != nil { killProcess := s.config.Debugger.AttachPid == 0 s.stopDebugSession(killProcess) - } else { + } else if s.noDebugProcess != nil { s.stopNoDebugProcess() } // The binary is no longer in use by the debugger. It is safe to remove it. if s.binaryToRemove != "" { gobuild.Remove(s.binaryToRemove) + s.binaryToRemove = "" // avoid error printed on duplicate removal } // Close client connection last, so other shutdown stages // can send client notifications. @@ -370,6 +375,11 @@ func (s *Session) Close() { // returned, this will result in a closed connection error // on next read, breaking out the read loop andd // allowing the run goroutinee to exit. + // This connection is closed here and in serveDAPCodec(). + // If this was a forced shutdown, external stop logic can close this first. + // If this was a client loop exit (on error or disconnect), serveDAPCodec() + // will be first. + // Duplicate close calls return an error, but are not fatal. _ = s.conn.Close() } @@ -382,14 +392,8 @@ func (s *Session) Close() { // and is currently only called from the run goroutine. func (c *Config) triggerServerStop() { // Avoid accidentally closing the channel twice and causing a panic, when - // this function is called more than once. For example, we could have the - // following sequence of events: - // -- run goroutine: calls onDisconnectRequest() - // -- run goroutine: calls triggerServerStop() - // -- main goroutine: calls Stop() - // -- main goroutine: Stop() closes client connection (or client closed it) - // -- run goroutine: ServeDAPCodec() gets "closed network connection" - // -- run goroutine: ServeDAPCodec() returns and calls triggerServerStop() + // this function is called more than once because stop was triggered + // by multiple conditions simultenously. if c.DisconnectChan != nil { close(c.DisconnectChan) c.DisconnectChan = nil @@ -461,7 +465,9 @@ func (s *Server) RunWithClient(conn net.Conn) { // until it encounters an error or EOF, when it sends // a disconnect signal and returns. func (s *Session) ServeDAPCodec() { - // TODO(polina): defer-close conn/session like in serveJSONCodec + // Close conn, but not the debugger in case we are in AcceptMuli mode. + // If not, debugger will be shut down in Stop(). + defer s.conn.Close() reader := bufio.NewReader(s.conn) for { request, err := dap.ReadProtocolMessage(reader) @@ -478,6 +484,9 @@ func (s *Session) ServeDAPCodec() { select { case <-s.config.StopTriggered: default: + if !s.config.AcceptMulti { + defer s.config.triggerServerStop() + } if err != io.EOF { // EOF means client closed connection if decodeErr, ok := err.(*dap.DecodeProtocolMessageFieldError); ok { // Send an error response to the users if we were unable to process the message. @@ -486,15 +495,15 @@ func (s *Session) ServeDAPCodec() { } s.config.log.Error("DAP error: ", err) } - if s.config.AcceptMulti { - s.conn.Close() - } else { - s.config.triggerServerStop() - } } return } s.handleRequest(request) + + if _, ok := request.(*dap.DisconnectRequest); ok { + // disconnect already shut things down and triggered stopping + return + } } } @@ -1137,11 +1146,13 @@ func (s *Session) onDisconnectRequest(request *dap.DisconnectRequest) { if s.debugger != nil && s.config.AcceptMulti && !request.Arguments.TerminateDebuggee { // This is a multi-use server/debugger, so a disconnect request that doesn't - // terminate the debuggee should clean up only the client connection, but not the server. + // 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()) s.send(&dap.DisconnectResponse{Response: *newResponse(request.Request)}) s.send(&dap.TerminatedEvent{Event: *newEvent("terminated")}) s.conn.Close() + s.debugger = nil // The target is left in whatever state it is already in - halted or running. // The users therefore have the flexibility to choose the appropriate state // for their case before disconnecting. This is also desirable in case of @@ -1162,7 +1173,7 @@ func (s *Session) onDisconnectRequest(request *dap.DisconnectRequest) { // overridden by an explicit request to terminate. killProcess := s.config.Debugger.AttachPid == 0 || request.Arguments.TerminateDebuggee err = s.stopDebugSession(killProcess) - } else { + } else if s.noDebugProcess != nil { s.stopNoDebugProcess() } if err != nil { @@ -1179,7 +1190,12 @@ func (s *Session) onDisconnectRequest(request *dap.DisconnectRequest) { // Returns any detach error other than proc.ErrProcessExited. func (s *Session) stopDebugSession(killProcess bool) error { s.changeStateMu.Lock() - defer s.changeStateMu.Unlock() + defer func() { + // Avoid running stop sequence twice. + // It's not fatal, but will result in duplicate logging. + s.debugger = nil + s.changeStateMu.Unlock() + }() if s.debugger == nil { return nil } diff --git a/service/dap/server_test.go b/service/dap/server_test.go index 7a36679c..1eb4da36 100644 --- a/service/dap/server_test.go +++ b/service/dap/server_test.go @@ -111,39 +111,189 @@ func startDAPServer(t *testing.T, serverStopped chan struct{}) (server *Server, return server, forceStop } -func TestForceStopNoClient(t *testing.T) { - serverStopped := make(chan struct{}) - _, forceStop := startDAPServer(t, serverStopped) - close(forceStop) - <-serverStopped +func verifyServerStopped(t *testing.T, server *Server) { + t.Helper() + if server.listener != nil { + if server.listener.Close() == nil { + t.Error("server should have closed listener after shutdown") + } + } + verifySessionStopped(t, server.session) } -func TestForceStopNoTarget(t *testing.T) { - serverStopped := make(chan struct{}) - server, forceStop := startDAPServer(t, serverStopped) - client := daptest.NewClient(server.config.Listener.Addr().String()) - defer client.Close() - - client.InitializeRequest() - client.ExpectInitializeResponseAndCapabilities(t) - close(forceStop) - <-serverStopped +func verifySessionStopped(t *testing.T, session *Session) { + t.Helper() + if session == nil { + return + } + if session.conn == nil { + t.Error("session must always have a set connection") + } + verifyConnStopped(t, session.conn) + if session.debugger != nil { + t.Error("session should have no pointer to debugger after shutdown") + } + if session.binaryToRemove != "" { + t.Error("session should have no binary to remove after shutdown") + } } -func TestForceStopWithTarget(t *testing.T) { - serverStopped := make(chan struct{}) - server, forceStop := startDAPServer(t, serverStopped) - client := daptest.NewClient(server.config.Listener.Addr().String()) - defer client.Close() +func verifyConnStopped(t *testing.T, conn io.ReadWriteCloser) { + t.Helper() + if conn.Close() == nil { + t.Error("client connection should be closed after shutdown") + } +} - client.InitializeRequest() - client.ExpectInitializeResponseAndCapabilities(t) - fixture := protest.BuildFixture("increment", protest.AllNonOptimized) - client.LaunchRequest("exec", fixture.Path, stopOnEntry) - client.ExpectInitializedEvent(t) - client.ExpectLaunchResponse(t) - close(forceStop) - <-serverStopped +func TestStopNoClient(t *testing.T) { + for name, triggerStop := range map[string]func(s *Server, forceStop chan struct{}){ + "force": func(s *Server, forceStop chan struct{}) { close(forceStop) }, + "accept error": func(s *Server, forceStop chan struct{}) { s.config.Listener.Close() }, + } { + t.Run(name, func(t *testing.T) { + serverStopped := make(chan struct{}) + server, forceStop := startDAPServer(t, serverStopped) + triggerStop(server, forceStop) + <-serverStopped + verifyServerStopped(t, server) + }) + } +} + +func TestStopNoTarget(t *testing.T) { + for name, triggerStop := range map[string]func(c *daptest.Client, forceStop chan struct{}){ + "force": func(c *daptest.Client, forceStop chan struct{}) { close(forceStop) }, + "read error": func(c *daptest.Client, forceStop chan struct{}) { c.Close() }, + "disconnect": func(c *daptest.Client, forceStop chan struct{}) { c.DisconnectRequest() }, + } { + t.Run(name, func(t *testing.T) { + serverStopped := make(chan struct{}) + server, forceStop := startDAPServer(t, serverStopped) + client := daptest.NewClient(server.config.Listener.Addr().String()) + defer client.Close() + + client.InitializeRequest() + client.ExpectInitializeResponseAndCapabilities(t) + triggerStop(client, forceStop) + <-serverStopped + verifyServerStopped(t, server) + }) + } +} + +func TestStopWithTarget(t *testing.T) { + for name, triggerStop := range map[string]func(c *daptest.Client, forceStop chan struct{}){ + "force": func(c *daptest.Client, forceStop chan struct{}) { close(forceStop) }, + "read error": func(c *daptest.Client, forceStop chan struct{}) { c.Close() }, + "disconnect before exit": func(c *daptest.Client, forceStop chan struct{}) { c.DisconnectRequest() }, + "disconnect after exit": func(c *daptest.Client, forceStop chan struct{}) { + c.ContinueRequest(1) + c.ExpectContinueResponse(t) + c.ExpectTerminatedEvent(t) + c.DisconnectRequest() + }, + } { + t.Run(name, func(t *testing.T) { + serverStopped := make(chan struct{}) + server, forceStop := startDAPServer(t, serverStopped) + client := daptest.NewClient(server.config.Listener.Addr().String()) + defer client.Close() + + client.InitializeRequest() + client.ExpectInitializeResponseAndCapabilities(t) + fixture := protest.BuildFixture("increment", protest.AllNonOptimized) + client.LaunchRequest("debug", fixture.Source, stopOnEntry) + client.ExpectInitializedEvent(t) + client.ExpectLaunchResponse(t) + triggerStop(client, forceStop) + <-serverStopped + verifyServerStopped(t, server) + }) + } +} + +func TestSessionStop(t *testing.T) { + verifySessionState := func(t *testing.T, s *Session, binaryToRemoveSet bool, debuggerSet bool, disconnectChanSet bool) { + t.Helper() + if binaryToRemoveSet && s.binaryToRemove == "" || !binaryToRemoveSet && s.binaryToRemove != "" { + t.Errorf("binaryToRemove: got %s, want set=%v", s.binaryToRemove, binaryToRemoveSet) + } + if debuggerSet && s.debugger == nil || !debuggerSet && s.debugger != nil { + t.Errorf("debugger: got %v, want set=%v", s.debugger, debuggerSet) + } + if disconnectChanSet && s.config.DisconnectChan == nil || !disconnectChanSet && s.config.DisconnectChan != nil { + t.Errorf("disconnectChan: got %v, want set=%v", s.config.DisconnectChan, disconnectChanSet) + } + } + for name, stopSession := range map[string]func(s *Session, c *daptest.Client, serveDone chan struct{}){ + "force": func(s *Session, c *daptest.Client, serveDone chan struct{}) { + s.Close() + <-serveDone + verifySessionState(t, s, false /*binaryToRemoveSet*/, false /*debuggerSet*/, false /*disconnectChanSet*/) + }, + "read error": func(s *Session, c *daptest.Client, serveDone chan struct{}) { + c.Close() + <-serveDone + verifyConnStopped(t, s.conn) + verifySessionState(t, s, true /*binaryToRemoveSet*/, true /*debuggerSet*/, false /*disconnectChanSet*/) + s.Close() + }, + "disconnect before exit": func(s *Session, c *daptest.Client, serveDone chan struct{}) { + c.DisconnectRequest() + <-serveDone + verifyConnStopped(t, s.conn) + verifySessionState(t, s, true /*binaryToRemoveSet*/, false /*debuggerSet*/, false /*disconnectChanSet*/) + s.Close() + }, + "disconnect after exit": func(s *Session, c *daptest.Client, serveDone chan struct{}) { + c.ContinueRequest(1) + c.ExpectContinueResponse(t) + c.ExpectTerminatedEvent(t) + c.DisconnectRequest() + <-serveDone + verifyConnStopped(t, s.conn) + verifySessionState(t, s, true /*binaryToRemoveSet*/, false /*debuggerSet*/, false /*disconnectChanSet*/) + s.Close() + }, + } { + t.Run(name, func(t *testing.T) { + listener, err := net.Listen("tcp", ":0") + if err != nil { + t.Fatalf("cannot setup listener required for testing: %v", err) + } + defer listener.Close() + acceptDone := make(chan struct{}) + var conn net.Conn + go func() { + conn, err = listener.Accept() + close(acceptDone) + }() + time.Sleep(10 * time.Millisecond) // give time to start listening + client := daptest.NewClient(listener.Addr().String()) + defer client.Close() + <-acceptDone + if err != nil { + t.Fatalf("cannot accept client requireed for testing: %v", err) + } + session := NewSession(conn, &Config{ + Config: &service.Config{DisconnectChan: make(chan struct{})}, + StopTriggered: make(chan struct{})}, nil) + serveDAPCodecDone := make(chan struct{}) + go func() { + session.ServeDAPCodec() + close(serveDAPCodecDone) + }() + time.Sleep(10 * time.Millisecond) // give time to start reading + client.InitializeRequest() + client.ExpectInitializeResponseAndCapabilities(t) + fixture := protest.BuildFixture("increment", protest.AllNonOptimized) + client.LaunchRequest("debug", fixture.Source, stopOnEntry) + client.ExpectInitializedEvent(t) + client.ExpectLaunchResponse(t) + stopSession(session, client, serveDAPCodecDone) + verifySessionStopped(t, session) + }) + } } func TestForceStopWhileStopping(t *testing.T) { @@ -160,6 +310,7 @@ func TestForceStopWhileStopping(t *testing.T) { time.Sleep(time.Microsecond) close(forceStop) // depending on timing may trigger Stop() <-serverStopped + verifyServerStopped(t, server) } // TestLaunchStopOnEntry emulates the message exchange that can be observed with @@ -6043,22 +6194,23 @@ func TestAttachRemoteToRunningTargetContinueOnEntry(t *testing.T) { // TestMultiClient 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) { - closingClientSession := "Closing client session, but leaving multi-client DAP server running at" + closingClientSessionOnly := "Closing client session, but leaving multi-client DAP server running at" detachingAndTerminating := "Detaching and terminating target process" tests := []struct { name string disconnectRequest func(client *daptest.Client) expect string }{ - {"default", func(c *daptest.Client) { c.DisconnectRequest() }, closingClientSession}, + {"default", func(c *daptest.Client) { c.DisconnectRequest() }, closingClientSessionOnly}, {"terminate=true", func(c *daptest.Client) { c.DisconnectRequestWithKillOption(true) }, detachingAndTerminating}, - {"terminate=false", func(c *daptest.Client) { c.DisconnectRequestWithKillOption(false) }, closingClientSession}, + {"terminate=false", func(c *daptest.Client) { c.DisconnectRequestWithKillOption(false) }, closingClientSessionOnly}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { serverStopped := make(chan struct{}) server, forceStop := startDAPServer(t, serverStopped) client := daptest.NewClient(server.listener.Addr().String()) + defer client.Close() time.Sleep(100 * time.Millisecond) // Give time for connection to be set as dap.Session server.sessionMu.Lock() if server.session == nil { @@ -6087,12 +6239,13 @@ func TestAttachRemoteMultiClient(t *testing.T) { } client.ExpectDisconnectResponse(t) client.ExpectTerminatedEvent(t) - client.Close() + time.Sleep(10 * time.Millisecond) // give time for things to shut down - if tc.expect == closingClientSession { - // At this point a multi-client server is still running, but since - // it is a dap server, it cannot accept another client, so the only - // way to take down the server is too force kill it. + if tc.expect == closingClientSessionOnly { + // At this point a multi-client server is still running. + verifySessionStopped(t, server.session) + // Since it is a dap server, it cannot accept another client, so the only + // way to take down the server is to force-kill it. close(forceStop) } <-serverStopped