From 3abb7241ac5eeef4760f420f60d8fc17f051746f Mon Sep 17 00:00:00 2001 From: Polina Sokolova Date: Sat, 3 Apr 2021 23:21:49 -0700 Subject: [PATCH 1/7] service/dap: refine teardown logic --- service/dap/server.go | 162 ++++++++++++++++++++----------------- service/dap/server_test.go | 10 +-- 2 files changed, 92 insertions(+), 80 deletions(-) diff --git a/service/dap/server.go b/service/dap/server.go index c43828b4ce..d00e1072b1 100644 --- a/service/dap/server.go +++ b/service/dap/server.go @@ -35,13 +35,29 @@ import ( ) // Server implements a DAP server that can accept a single client for -// a single debug session. It does not support restarting. -// The server operates via two goroutines: +// a single debug session (for now). It does yet not support restarting. +// That means that in addition to explicit shutdown requests, +// program termination and failed or closed client connection +// would also result in stopping this single-use server. +// +// The DAP server operates via the following goroutines: +// // (1) Main goroutine where the server is created via NewServer(), -// started via Run() and stopped via Stop(). -// (2) Run goroutine started from Run() that accepts a client connection, +// started via Run() and stopped via Stop(). Once the server is +// started, this goroutine blocks until it receives a stop-server +// signal that can come from an OS interrupt (such as Ctrl-C) or +// config.DisconnectChan (passed to NewServer()) as a result of +// client connection failure or closure or a DAP disconnect request. +// +// (2) Run goroutine started from Run() that serves as both +// a listener and a client goroutine. It accepts a client connection, // reads, decodes and processes each request, issuing commands to the // underlying debugger and sending back events and responses. +// This gorouitne sends a stop-server signal via config.DisconnecChan +// when encounering a clientt connection error or responding to +// a DAP disconnect request. +// +// TODO(polina): add another layer of per-client goroutines to support multiple clients // TODO(polina): make it asynchronous (i.e. launch goroutine per request) type Server struct { // config is all the information necessary to start the debugger and server. @@ -50,9 +66,9 @@ type Server struct { listener net.Listener // conn is the accepted client connection. conn net.Conn - // stopChan is closed when the server is Stop()-ed. This can be used to signal + // stopTriggered is closed when the server is Stop()-ed. This can be used to signal // to goroutines run by the server that it's time to quit. - stopChan chan struct{} + stopTriggered chan struct{} // reader is used to read requests from the connection. reader *bufio.Reader // debugger is the underlying debugger service. @@ -71,6 +87,8 @@ type Server struct { // args tracks special settings for handling debug session requests. args launchAttachArgs + // mu synchronizes access to objects set on start-up (from run goroutine) + // and stopped on teardown (from main goroutine) mu sync.Mutex // noDebugProcess is set for the noDebug launch process. @@ -107,9 +125,10 @@ var DefaultLoadConfig = proc.LoadConfig{ } // NewServer creates a new DAP Server. It takes an opened Listener -// via config and assumes its ownership. config.disconnectChan has to be set; -// it will be closed by the server when the client disconnects or requests -// shutdown. Once disconnectChan is closed, Server.Stop() must be called. +// via config and assumes its ownership. config.DisconnectChan has to be set; +// it will be closed by the server when the client fails to connect, +// disconnects or requests shutdown. Once config.DisconnectChan is closed, +// Server.Stop() must be called to shutdown this single-user server. func NewServer(config *service.Config) *Server { logger := logflags.DAPLogger() logflags.WriteDAPListeningMessage(config.Listener.Addr().String()) @@ -117,7 +136,7 @@ func NewServer(config *service.Config) *Server { return &Server{ config: config, listener: config.Listener, - stopChan: make(chan struct{}), + stopTriggered: make(chan struct{}), log: logger, stackFrameHandles: newHandlesMap(), variableHandles: newVariablesHandlesMap(), @@ -144,11 +163,13 @@ func (s *Server) setLaunchAttachArgs(request dap.LaunchAttachRequest) { // Stop stops the DAP debugger service, closes the listener and the client // connection. It shuts down the underlying debugger and kills the target -// process if it was launched by it. This method mustn't be called more than -// once. +// process if it was launched by it or stops the noDebug process. +// This method mustn't be called more than once. func (s *Server) Stop() { + close(s.stopTriggered) _ = s.listener.Close() - close(s.stopChan) + + s.mu.Lock() if s.conn != nil { // Unless Stop() was called after serveDAPCodec() // returned, this will result in closed connection error @@ -156,39 +177,35 @@ func (s *Server) Stop() { // allowing the run goroutine to exit. _ = s.conn.Close() } + if s.debugger != nil { - kill := s.config.Debugger.AttachPid == 0 - if err := s.debugger.Detach(kill); err != nil { - switch err.(type) { - case *proc.ErrProcessExited: - s.log.Debug(err) - default: - s.log.Error(err) - } - } + killProcess := s.config.Debugger.AttachPid == 0 + s.stopDebugSession(killProcess) + } else { + s.stopNoDebugProcess() } + s.mu.Unlock() } -// signalDisconnect closes config.DisconnectChan if not nil, which -// signals that the client disconnected or there was a client -// connection failure. Since the server currently services only one -// client, this can be used as a signal to the entire server via -// Stop(). The function safeguards agaist closing the channel more +// triggerServerStop closes config.DisconnectChan if not nil, which +// signals that client sent a disconnect request or there was connection +// failure or closure. Since the server currently services only one +// client, this is used as a signal to stop the entire server. +// The function safeguards agaist closing the channel more // than once and can be called multiple times. It is not thread-safe // and is currently only called from the run goroutine. // TODO(polina): lock this when we add more goroutines that could call // this when we support asynchronous request-response communication. -func (s *Server) signalDisconnect() { +func (s *Server) 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 signalDisconnect() + // -- run goroutine: calls triggerServerStop() // -- main goroutine: calls Stop() - // -- main goroutine: Stop() closes client connection + // -- main goroutine: Stop() closes client connection (or client closed it) // -- run goroutine: serveDAPCodec() gets "closed network connection" - // -- run goroutine: serveDAPCodec() returns - // -- run goroutine: serveDAPCodec calls signalDisconnect() + // -- run goroutine: serveDAPCodec() returns and calls triggerServerStop() if s.config.DisconnectChan != nil { close(s.config.DisconnectChan) s.config.DisconnectChan = nil @@ -211,23 +228,24 @@ func (s *Server) Run() { conn, err := s.listener.Accept() if err != nil { select { - case <-s.stopChan: + case <-s.stopTriggered: default: s.log.Errorf("Error accepting client connection: %s\n", err) + s.triggerServerStop() } - s.signalDisconnect() return } + s.mu.Lock() s.conn = conn + s.mu.Unlock() s.serveDAPCodec() }() } // serveDAPCodec reads and decodes requests from the client // until it encounters an error or EOF, when it sends -// the disconnect signal and returns. +// a disconnect signal and returns. func (s *Server) serveDAPCodec() { - defer s.signalDisconnect() s.reader = bufio.NewReader(s.conn) for { request, err := dap.ReadProtocolMessage(s.reader) @@ -239,14 +257,13 @@ func (s *Server) serveDAPCodec() { // TODO(polina): to support this add Seq to // dap.DecodeProtocolMessageFieldError. if err != nil { - stopRequested := false select { - case <-s.stopChan: - stopRequested = true + case <-s.stopTriggered: default: - } - if err != io.EOF && !stopRequested { - s.log.Error("DAP error: ", err) + if err != io.EOF { + s.log.Error("DAP error: ", err) + } + s.triggerServerStop() } return } @@ -566,6 +583,8 @@ func (s *Server) onLaunchRequest(request *dap.LaunchRequest) { } } var err error + s.mu.Lock() + defer s.mu.Unlock() if s.debugger, err = debugger.New(&s.config.Debugger, s.config.ProcessArgs); err != nil { s.sendErrorResponse(request.Request, FailedToLaunch, "Failed to launch", err.Error()) @@ -613,8 +632,6 @@ func (s *Server) runWithoutDebug(program string, targetArgs []string, wd string) } func (s *Server) stopNoDebugProcess() { - s.mu.Lock() - defer s.mu.Unlock() if s.noDebugProcess == nil { return } @@ -640,38 +657,37 @@ func isValidLaunchMode(launchMode interface{}) bool { // (in our case this TCP server) can be terminated. func (s *Server) onDisconnectRequest(request *dap.DisconnectRequest) { s.send(&dap.DisconnectResponse{Response: *newResponse(request.Request)}) + s.mu.Lock() if s.debugger != nil { - _, err := s.debugger.Command(&api.DebuggerCommand{Name: api.Halt}, nil) - if err != nil { - switch err.(type) { - case *proc.ErrProcessExited: - s.log.Debug(err) - default: - s.log.Error(err) - } - } - // We always kill launched programs - kill := s.config.Debugger.AttachPid == 0 + // We always kill launched programs. // In case of attach, we leave the program - // running but default, which can be + // running by default, which can be // overridden by an explicit request to terminate. - if request.Arguments.TerminateDebuggee { - kill = true - } - err = s.debugger.Detach(kill) - if err != nil { - switch err.(type) { - case *proc.ErrProcessExited: - s.log.Debug(err) - default: - s.log.Error(err) - } - } + killProcess := s.config.Debugger.AttachPid == 0 || request.Arguments.TerminateDebuggee + s.stopDebugSession(killProcess) } else { s.stopNoDebugProcess() } - // TODO(polina): make thread-safe when handlers become asynchronous. - s.signalDisconnect() + s.mu.Unlock() + s.triggerServerStop() +} + +func (s *Server) stopDebugSession(killProcess bool) { + _, err := s.debugger.Command(&api.DebuggerCommand{Name: api.Halt}, nil) + if err == proc.ErrProcessDetached { + s.log.Debug(err) + return + } else if err == nil { + err = s.debugger.Detach(killProcess) + } + if err != nil { + switch err.(type) { + case *proc.ErrProcessExited: + s.log.Debug(err) + default: + s.log.Error(err) + } + } } func (s *Server) onSetBreakpointsRequest(request *dap.SetBreakpointsRequest) { @@ -1340,7 +1356,7 @@ func (s *Server) onEvaluateRequest(request *dap.EvaluateRequest) { if retVars == nil { // The call got interrupted by a stop (e.g. breakpoint in injected // function call or in another goroutine) - s.resetHandlesForStop() + s.resetHandlesForStoppedEvent() stopped := &dap.StoppedEvent{Event: *newEvent("stopped")} stopped.Body.AllThreadsStopped = true if state.SelectedGoroutine != nil { @@ -1519,7 +1535,7 @@ func newEvent(event string) *dap.Event { const BetterBadAccessError = `invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation] Unable to propagate EXC_BAD_ACCESS signal to target process and panic (see https://github.com/go-delve/delve/issues/852)` -func (s *Server) resetHandlesForStop() { +func (s *Server) resetHandlesForStoppedEvent() { s.stackFrameHandles.reset() s.variableHandles.reset() } @@ -1539,7 +1555,7 @@ func (s *Server) doCommand(command string) { return } - s.resetHandlesForStop() + s.resetHandlesForStoppedEvent() stopped := &dap.StoppedEvent{Event: *newEvent("stopped")} stopped.Body.AllThreadsStopped = true diff --git a/service/dap/server_test.go b/service/dap/server_test.go index 261416a8de..3785439c7d 100644 --- a/service/dap/server_test.go +++ b/service/dap/server_test.go @@ -14,7 +14,6 @@ import ( "regexp" "runtime" "strings" - "sync" "testing" "time" @@ -64,22 +63,19 @@ func runTest(t *testing.T, name string, test func(c *daptest.Client, f protest.F // Give server time to start listening for clients time.Sleep(100 * time.Millisecond) - var stopOnce sync.Once // Run a goroutine that stops the server when disconnectChan is signaled. // This helps us test that certain events cause the server to stop as // expected. go func() { <-disconnectChan - stopOnce.Do(func() { server.Stop() }) + server.Stop() }() client := daptest.NewClient(listener.Addr().String()) + // This will close the client connectinon, which will cause a connection error + // on the server side and signal disconnect to unblock Stop() above. defer client.Close() - defer func() { - stopOnce.Do(func() { server.Stop() }) - }() - test(client, fixture) } From 49b85a8218135f3cbccd29a3bcf80253ee92a997 Mon Sep 17 00:00:00 2001 From: Polina Sokolova Date: Mon, 5 Apr 2021 16:04:48 -0700 Subject: [PATCH 2/7] Address review comments + add missing lock/unlock --- service/dap/server.go | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/service/dap/server.go b/service/dap/server.go index e578fc8187..e6407a86fb 100644 --- a/service/dap/server.go +++ b/service/dap/server.go @@ -64,15 +64,11 @@ type Server struct { config *service.Config // listener is used to accept the client connection. listener net.Listener - // conn is the accepted client connection. - conn net.Conn // stopTriggered is closed when the server is Stop()-ed. This can be used to signal // to goroutines run by the server that it's time to quit. stopTriggered chan struct{} // reader is used to read requests from the connection. reader *bufio.Reader - // debugger is the underlying debugger service. - debugger *debugger.Debugger // log is used for structured logging. log *logrus.Entry // binaryToRemove is the compiled binary to be removed on disconnect. @@ -91,6 +87,10 @@ type Server struct { // and stopped on teardown (from main goroutine) mu sync.Mutex + // conn is the accepted client connection. + conn net.Conn + // debugger is the underlying debugger service. + debugger *debugger.Debugger // noDebugProcess is set for the noDebug launch process. noDebugProcess *exec.Cmd } @@ -170,6 +170,7 @@ func (s *Server) Stop() { _ = s.listener.Close() s.mu.Lock() + defer s.mu.Unlock() if s.conn != nil { // Unless Stop() was called after serveDAPCodec() // returned, this will result in closed connection error @@ -184,7 +185,6 @@ func (s *Server) Stop() { } else { s.stopNoDebugProcess() } - s.mu.Unlock() } // triggerServerStop closes config.DisconnectChan if not nil, which @@ -628,6 +628,8 @@ func (s *Server) startNoDebugProcess(program string, targetArgs []string, wd str return cmd, nil } +// stopNoDebugProcess is called from Stop() (main goroutine) and +// onDisconnectRequest (run goroutine) and requires holding mu lock. func (s *Server) stopNoDebugProcess() { p := s.noDebugProcess s.noDebugProcess = nil @@ -653,7 +655,9 @@ func isValidLaunchMode(launchMode interface{}) bool { // (in our case this TCP server) can be terminated. func (s *Server) onDisconnectRequest(request *dap.DisconnectRequest) { s.send(&dap.DisconnectResponse{Response: *newResponse(request.Request)}) + defer s.triggerServerStop() s.mu.Lock() + defer s.mu.Unlock() if s.debugger != nil { // We always kill launched programs. // In case of attach, we leave the program @@ -664,18 +668,22 @@ func (s *Server) onDisconnectRequest(request *dap.DisconnectRequest) { } else { s.stopNoDebugProcess() } - s.mu.Unlock() - s.triggerServerStop() } +// stopDebugSession is called from Stop() (main goroutine) and +// onDisconnectRequest (run goroutine) and requires holding mu lock. func (s *Server) stopDebugSession(killProcess bool) { + if s.debugger == nil { + return + } _, err := s.debugger.Command(&api.DebuggerCommand{Name: api.Halt}, nil) - if err == proc.ErrProcessDetached { + if err != nil { s.log.Debug(err) + } + if err == proc.ErrProcessDetached { return - } else if err == nil { - err = s.debugger.Detach(killProcess) } + err = s.debugger.Detach(killProcess) if err != nil { switch err.(type) { case *proc.ErrProcessExited: @@ -857,6 +865,8 @@ func (s *Server) onAttachRequest(request *dap.AttachRequest) { s.config.Debugger.AttachPid = int(pid) s.setLaunchAttachArgs(request) var err error + s.mu.Lock() + defer s.mu.Unlock() if s.debugger, err = debugger.New(&s.config.Debugger, nil); err != nil { s.sendErrorResponse(request.Request, FailedToAttach, "Failed to attach", err.Error()) return From e8a28ede7d64e050c54e10329a880d0a924ba6cc Mon Sep 17 00:00:00 2001 From: Polina Sokolova Date: Mon, 5 Apr 2021 20:46:17 -0700 Subject: [PATCH 3/7] Narrow lock scope --- service/dap/server.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/service/dap/server.go b/service/dap/server.go index e6407a86fb..0c7dd16d37 100644 --- a/service/dap/server.go +++ b/service/dap/server.go @@ -572,7 +572,9 @@ func (s *Server) onLaunchRequest(request *dap.LaunchRequest) { } if noDebug, ok := request.Arguments["noDebug"].(bool); ok && noDebug { + s.mu.Lock() cmd, err := s.startNoDebugProcess(program, targetArgs, s.config.Debugger.WorkingDir) + s.mu.Unlock() if err != nil { s.sendErrorResponse(request.Request, FailedToLaunch, "Failed to launch", err.Error()) return @@ -600,8 +602,9 @@ func (s *Server) onLaunchRequest(request *dap.LaunchRequest) { var err error s.mu.Lock() - defer s.mu.Unlock() - if s.debugger, err = debugger.New(&s.config.Debugger, s.config.ProcessArgs); err != nil { + s.debugger, err = debugger.New(&s.config.Debugger, s.config.ProcessArgs) + s.mu.Unlock() + if err != nil { s.sendErrorResponse(request.Request, FailedToLaunch, "Failed to launch", err.Error()) return } @@ -613,9 +616,9 @@ func (s *Server) onLaunchRequest(request *dap.LaunchRequest) { s.send(&dap.LaunchResponse{Response: *newResponse(request.Request)}) } +// startNoDebugProcess is called from onLaunchRequest (run goroutine) and +// requires holding mu lock. 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 nil, fmt.Errorf("another launch request is in progress") } @@ -866,8 +869,9 @@ func (s *Server) onAttachRequest(request *dap.AttachRequest) { s.setLaunchAttachArgs(request) var err error s.mu.Lock() - defer s.mu.Unlock() - if s.debugger, err = debugger.New(&s.config.Debugger, nil); err != nil { + s.debugger, err = debugger.New(&s.config.Debugger, nil) + s.mu.Unlock() + if err != nil { s.sendErrorResponse(request.Request, FailedToAttach, "Failed to attach", err.Error()) return } From 13243f9a3169f0dc54afeb7ed00e6ce41d7f2df1 Mon Sep 17 00:00:00 2001 From: Polina Sokolova Date: Tue, 6 Apr 2021 15:51:03 -0700 Subject: [PATCH 4/7] Update comments only --- service/dap/server.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/service/dap/server.go b/service/dap/server.go index 0c7dd16d37..362a225673 100644 --- a/service/dap/server.go +++ b/service/dap/server.go @@ -225,7 +225,7 @@ func (s *Server) triggerServerStop() { // so the editor needs to launch delve only once? func (s *Server) Run() { go func() { - conn, err := s.listener.Accept() + conn, err := s.listener.Accept() // closed in Stop() if err != nil { select { case <-s.stopTriggered: @@ -236,7 +236,7 @@ func (s *Server) Run() { return } s.mu.Lock() - s.conn = conn + s.conn = conn // closed in Stop() s.mu.Unlock() s.serveDAPCodec() }() @@ -631,7 +631,7 @@ func (s *Server) startNoDebugProcess(program string, targetArgs []string, wd str return cmd, nil } -// stopNoDebugProcess is called from Stop() (main goroutine) and +// stopNoDebugProcess is called from Stop (main goroutine) and // onDisconnectRequest (run goroutine) and requires holding mu lock. func (s *Server) stopNoDebugProcess() { p := s.noDebugProcess @@ -673,7 +673,7 @@ func (s *Server) onDisconnectRequest(request *dap.DisconnectRequest) { } } -// stopDebugSession is called from Stop() (main goroutine) and +// stopDebugSession is called from Stop (main goroutine) and // onDisconnectRequest (run goroutine) and requires holding mu lock. func (s *Server) stopDebugSession(killProcess bool) { if s.debugger == nil { From ae989b94cdd0592bab58c9d624acdb707d162adc Mon Sep 17 00:00:00 2001 From: Polina Sokolova Date: Tue, 6 Apr 2021 15:56:09 -0700 Subject: [PATCH 5/7] Remove redundan temp var from stopNoDebugProcess --- service/dap/server.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/service/dap/server.go b/service/dap/server.go index 362a225673..7dd951032d 100644 --- a/service/dap/server.go +++ b/service/dap/server.go @@ -634,13 +634,11 @@ func (s *Server) startNoDebugProcess(program string, targetArgs []string, wd str // stopNoDebugProcess is called from Stop (main goroutine) and // onDisconnectRequest (run goroutine) and requires holding mu lock. func (s *Server) stopNoDebugProcess() { - p := s.noDebugProcess - s.noDebugProcess = nil - // TODO(hyangah): gracefully terminate the process and its children processes. - if p != nil && !p.ProcessState.Exited() { - p.Process.Kill() // Don't check error. Process killing and self-termination may race. + if s.noDebugProcess != nil && !s.noDebugProcess.ProcessState.Exited() { + s.noDebugProcess.Process.Kill() // Don't check error. Process killing and self-termination may race. } + s.noDebugProcess = nil } // TODO(polina): support "remote" mode From ca5e7eab1c69fbb07ace041e9c8f098ee1de168d Mon Sep 17 00:00:00 2001 From: Polina Sokolova Date: Tue, 6 Apr 2021 17:28:10 -0700 Subject: [PATCH 6/7] Clarify comment --- service/dap/server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/service/dap/server.go b/service/dap/server.go index 7dd951032d..af75e1bed8 100644 --- a/service/dap/server.go +++ b/service/dap/server.go @@ -225,7 +225,7 @@ func (s *Server) triggerServerStop() { // so the editor needs to launch delve only once? func (s *Server) Run() { go func() { - conn, err := s.listener.Accept() // closed in Stop() + conn, err := s.listener.Accept() // listener is closed in Stop() if err != nil { select { case <-s.stopTriggered: From a3048c428d27df555b47889c1a9d5f2a25f7c6d1 Mon Sep 17 00:00:00 2001 From: Polina Sokolova Date: Tue, 13 Apr 2021 16:30:27 -0700 Subject: [PATCH 7/7] Set debugger to nil after detach to prevent dup teardown in Stop() --- service/dap/server.go | 1 + 1 file changed, 1 insertion(+) diff --git a/service/dap/server.go b/service/dap/server.go index 04939c2a99..101c9d9d75 100644 --- a/service/dap/server.go +++ b/service/dap/server.go @@ -729,6 +729,7 @@ func (s *Server) stopDebugSession(killProcess bool) error { s.logToConsole("Detaching without terminating target processs") } err = s.debugger.Detach(killProcess) + s.debugger = nil if err != nil { switch err.(type) { case proc.ErrProcessExited: