Repository: go-delve/delve
Pull request #2424 (2021-04-08 09:25:02)

debugger: report error when switching goroutine is impossible
Due to variable shadowing the SwitchGoroutine command never failed.


aarzilli: > > The Command call immediately proceeding this will block until the target stops, there is no reason to do a non-blocking state call here.
> 
> I feel like I am missing some nuance here. Are we relying on the fact that the previous call returned, so things are definitely not running? Would that assumption change if we are in some kind async multi-client mode? When would we want to use a non-blocking version? Would a non-blocking call return different information here? And what's the downside of using one vs the other?

There's almost never a good reason to call the non-blocking version of State. It isn't a synchronization primitive, either you know if the target is running because you are controlling it or non-blocking State won't help you. You can grep for GetStateNonBlocking to see acceptable uses of non-blocking State. Basically: when what you are about to do can be done regardless of what GetStateNonBlocking says, as a QoL improvement.
polinasok: > GetStateNonBlocking

Then it sounds like we need to revisit the rest of the uses of GetState in the dap server.
aarzilli: > > GetStateNonBlocking
> 
> Then it sounds like we need to revisit the rest of the uses of GetState in the dap server.

They're fine. It's just that you could turn them all to blocking and it wouldn't change anything.

Patch:
diff --git a/_scripts/make.go b/_scripts/make.go
index 40e35072..3883ebbb 100644
--- a/_scripts/make.go
+++ b/_scripts/make.go
@@ -213,7 +213,7 @@ func installedExecutablePath() string {
 // i.e. cgo enabled and the legacy SDK headers:
 // https://forums.developer.apple.com/thread/104296
 func canMacnative() bool {
-	if runtime.GOOS != "darwin" {
+	if !(runtime.GOOS == "darwin" && runtime.GOARCH == "amd64") {
 		return false
 	}
 	if strings.TrimSpace(getoutput("go", "env", "CGO_ENABLED")) != "1" {
@@ -221,13 +221,18 @@ func canMacnative() bool {
 	}
 
 	macOSVersion := strings.Split(strings.TrimSpace(getoutput("/usr/bin/sw_vers", "-productVersion")), ".")
+
+	major, err := strconv.ParseInt(macOSVersion[0], 10, 64)
+	if err != nil {
+		return false
+	}
 	minor, err := strconv.ParseInt(macOSVersion[1], 10, 64)
 	if err != nil {
 		return false
 	}
 
 	typesHeader := "/usr/include/sys/types.h"
-	if minor >= 15 {
+	if major >= 11 || (major == 10 && minor >= 15) {
 		typesHeader = "/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/sys/types.h"
 	}
 	_, err = os.Stat(typesHeader)
diff --git a/service/dap/server.go b/service/dap/server.go
index a77f913f..d644ef5f 100644
--- a/service/dap/server.go
+++ b/service/dap/server.go
@@ -36,28 +36,40 @@ 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 client 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.
 	config *service.Config
 	// listener is used to accept the client connection.
 	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.
-	debugger *debugger.Debugger
 	// log is used for structured logging.
 	log *logrus.Entry
 	// stackFrameHandles maps frames of each goroutine to unique ids across all goroutines.
@@ -70,7 +82,13 @@ 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
+	// conn is the accepted client connection.
+	conn net.Conn
+	// debugger is the underlying debugger service.
+	debugger *debugger.Debugger
 	// binaryToRemove is the temp compiled binary to be removed on disconnect (if any).
 	binaryToRemove string
 	// noDebugProcess is set for the noDebug launch process.
@@ -115,9 +133,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())
@@ -125,7 +144,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(),
@@ -181,12 +200,15 @@ func (s *Server) setLaunchAttachArgs(request dap.LaunchAttachRequest) error {
 
 // 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() {
 	s.log.Debug("DAP server stopping...")
+	close(s.stopTriggered)
 	_ = s.listener.Close()
-	close(s.stopChan)
+
+	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
@@ -195,21 +217,12 @@ func (s *Server) Stop() {
 		_ = 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()
 	}
 	// The binary is no longer in use by the debugger. It is safe to remove it.
-	s.mu.Lock()
-	defer s.mu.Unlock()
 	if s.binaryToRemove != "" {
 		gobuild.Remove(s.binaryToRemove)
 		s.binaryToRemove = ""
@@ -217,26 +230,25 @@ func (s *Server) Stop() {
 	s.log.Debug("DAP server stopped")
 }
 
-// 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
@@ -255,26 +267,27 @@ func (s *Server) signalDisconnect() {
 // 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() // listener is closed in Stop()
 		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.conn = conn
+		s.mu.Lock()
+		s.conn = conn // closed in Stop()
+		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)
@@ -286,14 +299,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
 		}
@@ -606,20 +618,22 @@ func (s *Server) onLaunchRequest(request *dap.LaunchRequest) {
 	s.config.Debugger.WorkingDir = filepath.Dir(program)
 
 	// Set the WorkingDir for this program to the one specified in the request arguments.
-	wd, ok := request.Arguments["wd"]
+	wd, ok := request.Arguments["cwd"]
 	if ok {
 		wdParsed, ok := wd.(string)
 		if !ok {
 			s.sendErrorResponse(request.Request,
 				FailedToLaunch, "Failed to launch",
-				fmt.Sprintf("'wd' attribute '%v' in debug configuration is not a string.", wd))
+				fmt.Sprintf("'cwd' attribute '%v' in debug configuration is not a string.", wd))
 			return
 		}
 		s.config.Debugger.WorkingDir = wdParsed
 	}
 
 	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
@@ -645,17 +659,26 @@ func (s *Server) onLaunchRequest(request *dap.LaunchRequest) {
 		return
 	}
 
-	if s.debugger, err = debugger.New(&s.config.Debugger, s.config.ProcessArgs); err != nil {
+	func() {
+		s.mu.Lock()
+		defer s.mu.Unlock() // Make sure to unlock in case of panic that will become internal error
+		s.debugger, err = debugger.New(&s.config.Debugger, s.config.ProcessArgs)
+	}()
+	if err != nil {
 		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)})
 }
 
+// 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")
 	}
@@ -668,24 +691,21 @@ 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() {
-	s.mu.Lock()
-	p := s.noDebugProcess
-	s.noDebugProcess = nil
-	defer s.mu.Unlock()
-
-	if p == nil {
+	if s.noDebugProcess == nil {
 		// We already handled termination or there was never a process
 		return
 	}
-
-	if p.ProcessState.Exited() {
-		s.logToConsole(proc.ErrProcessExited{Pid: p.ProcessState.Pid(), Status: p.ProcessState.ExitCode()}.Error())
+	if s.noDebugProcess.ProcessState.Exited() {
+		s.logToConsole(proc.ErrProcessExited{Pid: s.noDebugProcess.ProcessState.Pid(), Status: s.noDebugProcess.ProcessState.ExitCode()}.Error())
 	} else {
 		// TODO(hyangah): gracefully terminate the process and its children processes.
-		s.logToConsole(fmt.Sprintf("Terminating process %d", p.Process.Pid))
-		p.Process.Kill() // Don't check error. Process killing and self-termination may race.
+		s.logToConsole(fmt.Sprintf("Terminating process %d", s.noDebugProcess.Process.Pid))
+		s.noDebugProcess.Process.Kill() // Don't check error. Process killing and self-termination may race.
 	}
+	s.noDebugProcess = nil
 }
 
 // TODO(polina): support "remote" mode
@@ -702,57 +722,73 @@ func isValidLaunchMode(launchMode interface{}) bool {
 // it disconnects the debuggee and signals that the debug adaptor
 // (in our case this TCP server) can be terminated.
 func (s *Server) onDisconnectRequest(request *dap.DisconnectRequest) {
+	defer s.triggerServerStop()
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
 	var err error
-	var exited error
 	if s.debugger != nil {
-		state, err := s.debugger.Command(&api.DebuggerCommand{Name: api.Halt}, nil)
-		if err != nil {
-			switch err.(type) {
-			case proc.ErrProcessExited:
-				exited = err
-			default:
-				s.log.Error(err)
-			}
-		} else if state.Exited {
-			exited = proc.ErrProcessExited{Pid: s.debugger.ProcessPid(), Status: state.ExitStatus}
-		}
-		// 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 by default, which can be
 		// overridden by an explicit request to terminate.
-		if request.Arguments.TerminateDebuggee {
-			kill = true
-		}
-		if exited != nil {
-			s.logToConsole(exited.Error())
-			s.logToConsole("Detaching")
-		} else if kill {
-			s.logToConsole("Detaching and terminating target process")
-		} else {
-			s.logToConsole("Detaching without terminating target processs")
-		}
-		err = s.debugger.Detach(kill)
-		if err != nil {
-			switch err.(type) {
-			case proc.ErrProcessExited:
-				exited = err
-				s.logToConsole(exited.Error())
-			default:
-				s.log.Error(err)
-			}
-		}
+		killProcess := s.config.Debugger.AttachPid == 0 || request.Arguments.TerminateDebuggee
+		err = s.stopDebugSession(killProcess)
 	} else {
 		s.stopNoDebugProcess()
 	}
-	if err != nil && exited == nil {
+	if err != nil {
 		s.sendErrorResponse(request.Request, DisconnectError, "Error while disconnecting", err.Error())
 	} else {
 		s.send(&dap.DisconnectResponse{Response: *newResponse(request.Request)})
 	}
+}
 
-	// TODO(polina): make thread-safe when handlers become asynchronous.
-	s.signalDisconnect()
+// stopDebugSession is called from Stop (main goroutine) and
+// onDisconnectRequest (run goroutine) and requires holding mu lock.
+// Returns any detach error other than proc.ErrProcessExited.
+func (s *Server) stopDebugSession(killProcess bool) error {
+	if s.debugger == nil {
+		return nil
+	}
+	var err error
+	var exited error
+	state, err := s.debugger.Command(&api.DebuggerCommand{Name: api.Halt}, nil)
+	if err == proc.ErrProcessDetached {
+		s.log.Debug(err)
+		return nil
+	}
+	if err != nil {
+		switch err.(type) {
+		case proc.ErrProcessExited:
+			exited = err
+		default:
+			s.log.Error(err)
+		}
+	} else if state.Exited {
+		exited = proc.ErrProcessExited{Pid: s.debugger.ProcessPid(), Status: state.ExitStatus}
+	}
+	if exited != nil {
+		s.logToConsole(exited.Error())
+		s.logToConsole("Detaching")
+	} else if killProcess {
+		s.logToConsole("Detaching and terminating target process")
+	} else {
+		s.logToConsole("Detaching without terminating target processs")
+	}
+	err = s.debugger.Detach(killProcess)
+	s.debugger = nil
+	if err != nil {
+		switch err.(type) {
+		case proc.ErrProcessExited:
+			s.log.Debug(err)
+			s.logToConsole(exited.Error())
+			err = nil
+		default:
+			s.log.Error(err)
+		}
+	}
+	return err
 }
 
 func (s *Server) onSetBreakpointsRequest(request *dap.SetBreakpointsRequest) {
@@ -929,14 +965,16 @@ func (s *Server) onAttachRequest(request *dap.AttachRequest) {
 		s.config.Debugger.AttachPid = int(pid)
 		err := s.setLaunchAttachArgs(request)
 		if err != nil {
-			s.sendErrorResponse(request.Request,
-				FailedToAttach, "Failed to attach",
-				err.Error())
+			s.sendErrorResponse(request.Request, FailedToAttach, "Failed to attach", err.Error())
 			return
 		}
-		if s.debugger, err = debugger.New(&s.config.Debugger, nil); err != nil {
-			s.sendErrorResponse(request.Request,
-				FailedToAttach, "Failed to attach", err.Error())
+		func() {
+			s.mu.Lock()
+			defer s.mu.Unlock() // Make sure to unlock in case of panic that will become internal error
+			s.debugger, err = debugger.New(&s.config.Debugger, nil)
+		}()
+		if err != nil {
+			s.sendErrorResponse(request.Request, FailedToAttach, "Failed to attach", err.Error())
 			return
 		}
 	} else {
@@ -985,14 +1023,16 @@ func stoppedGoroutineID(state *api.DebuggerState) (id int) {
 
 func (s *Server) doStepCommand(command string, threadId int) {
 	// Use SwitchGoroutine to change the current goroutine.
-	state, err := s.debugger.Command(&api.DebuggerCommand{Name: api.SwitchGoroutine, GoroutineID: threadId}, nil)
+	_, err := s.debugger.Command(&api.DebuggerCommand{Name: api.SwitchGoroutine, GoroutineID: threadId}, nil)
 	if err != nil {
-		s.log.Errorf("Error switching goroutines while stepping: %e", err)
+		s.log.Errorf("Error switching goroutines while stepping: %v", err)
 		// If we encounter an error, we will have to send a stopped event
 		// since we already sent the step response.
 		stopped := &dap.StoppedEvent{Event: *newEvent("stopped")}
 		stopped.Body.AllThreadsStopped = true
-		if state != nil {
+		if state, err := s.debugger.State(false); err != nil {
+			s.log.Errorf("Error retrieving state: %e", err)
+		} else {
 			stopped.Body.ThreadId = stoppedGoroutineID(state)
 		}
 		stopped.Body.Reason = "error"
@@ -1083,8 +1123,6 @@ func (s *Server) onScopesRequest(request *dap.ScopesRequest) {
 	}
 	locScope := &fullyQualifiedVariable{&proc.Variable{Name: "Locals", Children: slicePtrVarToSliceVar(locals)}, "", true}
 
-	// TODO(polina): Annotate shadowed variables
-
 	scopeArgs := dap.Scope{Name: argScope.Name, VariablesReference: s.variableHandles.create(argScope)}
 	scopeLocals := dap.Scope{Name: locScope.Name, VariablesReference: s.variableHandles.create(locScope)}
 	scopes := []dap.Scope{scopeArgs, scopeLocals}
@@ -1237,8 +1275,18 @@ func (s *Server) onVariablesRequest(request *dap.VariablesRequest) {
 				cfqname = "" // complex children are not struct fields and can't be accessed directly
 			}
 			cvalue, cvarref := s.convertVariable(c, cfqname)
+
+			// Annotate any shadowed variables to "(name)" in order
+			// to distinguish from non-shadowed variables.
+			// TODO(suzmue): should we support a special evaluateName syntax that
+			// can access shadowed variables?
+			name := c.Name
+			if c.Flags&proc.VariableShadowed == proc.VariableShadowed {
+				name = fmt.Sprintf("(%s)", name)
+			}
+
 			children[i] = dap.Variable{
-				Name:               c.Name,
+				Name:               name,
 				EvaluateName:       cfqname,
 				Value:              cvalue,
 				VariablesReference: cvarref,
@@ -1441,7 +1489,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
 			stopped.Body.ThreadId = stoppedGoroutineID(state)
@@ -1618,7 +1666,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()
 }
@@ -1638,7 +1686,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/debugger/debugger.go b/service/debugger/debugger.go
index 6504bc02..850419a9 100644
--- a/service/debugger/debugger.go
+++ b/service/debugger/debugger.go
@@ -1095,7 +1095,8 @@ func (d *Debugger) Command(command *api.DebuggerCommand, resumeNotify chan struc
 		withBreakpointInfo = false
 	case api.SwitchGoroutine:
 		d.log.Debugf("switching to goroutine %d", command.GoroutineID)
-		g, err := proc.FindGoroutine(d.target, command.GoroutineID)
+		var g *proc.G
+		g, err = proc.FindGoroutine(d.target, command.GoroutineID)
 		if err == nil {
 			err = d.target.SwitchGoroutine(g)
 		}
diff --git a/service/rpc1/server.go b/service/rpc1/server.go
index da3e3c8d..558594dd 100644
--- a/service/rpc1/server.go
+++ b/service/rpc1/server.go
@@ -35,11 +35,7 @@ func (s *RPCServer) ProcessPid(arg1 interface{}, pid *int) error {
 }
 
 func (s *RPCServer) Detach(kill bool, ret *int) error {
-	err := s.debugger.Detach(kill)
-	if s.config.DisconnectChan != nil {
-		close(s.config.DisconnectChan)
-	}
-	return err
+	return s.debugger.Detach(kill)
 }
 
 func (s *RPCServer) Restart(arg1 interface{}, arg2 *int) error {
diff --git a/service/rpc2/server.go b/service/rpc2/server.go
index a6f7bc73..81ee8aac 100644
--- a/service/rpc2/server.go
+++ b/service/rpc2/server.go
@@ -58,12 +58,7 @@ type DetachOut struct {
 
 // Detach detaches the debugger, optionally killing the process.
 func (s *RPCServer) Detach(arg DetachIn, out *DetachOut) error {
-	err := s.debugger.Detach(arg.Kill)
-	if s.config.DisconnectChan != nil {
-		close(s.config.DisconnectChan)
-		s.config.DisconnectChan = nil
-	}
-	return err
+	return s.debugger.Detach(arg.Kill)
 }
 
 type RestartIn struct {
diff --git a/service/rpccommon/server.go b/service/rpccommon/server.go
index d3eb47dc..affdd114 100644
--- a/service/rpccommon/server.go
+++ b/service/rpccommon/server.go
@@ -89,8 +89,8 @@ func NewServer(config *service.Config) *ServerImpl {
 
 // Stop stops the JSON-RPC server.
 func (s *ServerImpl) Stop() error {
+	close(s.stopChan)
 	if s.config.AcceptMulti {
-		close(s.stopChan)
 		s.listener.Close()
 	}
 	kill := s.config.Debugger.AttachPid == 0
@@ -323,6 +323,10 @@ func (s *ServerImpl) serveJSONCodec(conn io.ReadWriteCloser) {
 				s.log.Debugf("-> %T%s error: %q", replyv.Interface(), replyvbytes, errmsg)
 			}
 			s.sendResponse(sending, &req, &resp, replyv.Interface(), codec, errmsg)
+			if req.ServiceMethod == "RPCServer.Detach" && s.config.DisconnectChan != nil {
+				close(s.config.DisconnectChan)
+				s.config.DisconnectChan = nil
+			}
 		} else {
 			if logflags.RPC() {
 				argvbytes, _ := json.Marshal(argv.Interface())


Test patch:
diff --git a/pkg/proc/proc_test.go b/pkg/proc/proc_test.go
index 9136073a..3820ae9d 100644
--- a/pkg/proc/proc_test.go
+++ b/pkg/proc/proc_test.go
@@ -2830,12 +2830,16 @@ func TestAttachDetach(t *testing.T) {
 
 	assertNoError(p.Detach(false), t, "Detach")
 
-	resp, err := http.Get("http://127.0.0.1:9191/nobp")
-	assertNoError(err, t, "Page request after detach")
-	bs, err := ioutil.ReadAll(resp.Body)
-	assertNoError(err, t, "Reading /nobp page")
-	if out := string(bs); !strings.Contains(out, "hello, world!") {
-		t.Fatalf("/nobp page does not contain \"hello, world!\": %q", out)
+	if runtime.GOOS != "darwin" {
+		// Debugserver sometimes will leave a zombie process after detaching, this
+		// seems to be a bug with debugserver.
+		resp, err := http.Get("http://127.0.0.1:9191/nobp")
+		assertNoError(err, t, "Page request after detach")
+		bs, err := ioutil.ReadAll(resp.Body)
+		assertNoError(err, t, "Reading /nobp page")
+		if out := string(bs); !strings.Contains(out, "hello, world!") {
+			t.Fatalf("/nobp page does not contain \"hello, world!\": %q", out)
+		}
 	}
 
 	cmd.Process.Kill()
diff --git a/service/dap/server_test.go b/service/dap/server_test.go
index 3faad362..67881d6a 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)
 }
 
@@ -229,7 +225,7 @@ func TestLaunchStopOnEntry(t *testing.T) {
 			t.Errorf("\ngot %#v\nwant Seq=0 Category='console'", oep)
 		}
 		oed := client.ExpectOutputEventDetaching(t)
-		if oed.Seq != 0 || oep.Body.Category != "console" {
+		if oed.Seq != 0 || oed.Body.Category != "console" {
 			t.Errorf("\ngot %#v\nwant Seq=0 Category='console'", oed)
 		}
 		dResp := client.ExpectDisconnectResponse(t)
@@ -1579,6 +1575,46 @@ func TestGlobalScopeAndVariables(t *testing.T) {
 	})
 }
 
+// TestShadowedVariables executes to a breakpoint and checks the shadowed
+// variable is named correctly.
+func TestShadowedVariables(t *testing.T) {
+	runTest(t, "testshadow", func(client *daptest.Client, fixture protest.Fixture) {
+		runDebugSessionWithBPs(t, client, "launch",
+			// Launch
+			func() {
+				client.LaunchRequestWithArgs(map[string]interface{}{
+					"mode": "exec", "program": fixture.Path, "showGlobalVariables": true,
+				})
+			},
+			// Breakpoints are set within the program
+			fixture.Source, []int{},
+			[]onBreakpoint{{
+				// Stop at line 13
+				execute: func() {
+					client.StackTraceRequest(1, 0, 20)
+					stack := client.ExpectStackTraceResponse(t)
+					expectStackFrames(t, stack, "main.main", 13, 1000, 3, 3)
+
+					client.ScopesRequest(1000)
+					scopes := client.ExpectScopesResponse(t)
+					expectScope(t, scopes, 0, "Arguments", 1000)
+					expectScope(t, scopes, 1, "Locals", 1001)
+					expectScope(t, scopes, 2, "Globals (package main)", 1002)
+
+					client.VariablesRequest(1001)
+					locals := client.ExpectVariablesResponse(t)
+
+					expectVarExact(t, locals, 0, "(a)", "a", "0", !hasChildren)
+					expectVarExact(t, locals, 1, "a", "a", "1", !hasChildren)
+
+					// Check that the non-shadowed of "a" is returned from evaluate request.
+					validateEvaluateName(t, client, locals, 1)
+				},
+				disconnect: false,
+			}})
+	})
+}
+
 // Tests that 'stackTraceDepth' from LaunchRequest is parsed and passed to
 // stacktrace requests handlers.
 func TestLaunchRequestWithStackTraceDepth(t *testing.T) {
@@ -1808,7 +1844,7 @@ func TestWorkingDir(t *testing.T) {
 					"mode":        "exec",
 					"program":     fixture.Path,
 					"stopOnEntry": false,
-					"wd":          wd,
+					"cwd":         wd,
 				})
 			},
 			// Set breakpoints
@@ -2166,9 +2202,12 @@ func TestNextAndStep(t *testing.T) {
 					client.ExpectStepInResponse(t)
 					expectStop("main.inlineThis", 5)
 
-					client.NextRequest(-10000 /*this is ignored*/)
+					client.NextRequest(-1000)
 					client.ExpectNextResponse(t)
-					expectStop("main.inlineThis", 6)
+					if se := client.ExpectStoppedEvent(t); se.Body.Reason != "error" || se.Body.Text != "unknown goroutine -1000" {
+						t.Errorf("got %#v, want Reaspon=\"error\", Text=\"unknown goroutine -1000\"", se)
+					}
+					handleStop(t, client, 1, "main.inlineThis", 5)
 				},
 				disconnect: false,
 			}})
@@ -3032,9 +3071,9 @@ func TestBadLaunchRequests(t *testing.T) {
 		client.LaunchRequestWithArgs(map[string]interface{}{"mode": "debug", "program": fixture.Source, "substitutePath": []interface{}{map[string]interface{}{"from": "path1", "to": 123}}})
 		expectFailedToLaunchWithMessage(client.ExpectErrorResponse(t),
 			"Failed to launch: 'substitutePath' attribute '[map[from:path1 to:123]]' in debug configuration is not a []{'from': string, 'to': string}")
-		client.LaunchRequestWithArgs(map[string]interface{}{"mode": "debug", "program": fixture.Source, "wd": 123})
+		client.LaunchRequestWithArgs(map[string]interface{}{"mode": "debug", "program": fixture.Source, "cwd": 123})
 		expectFailedToLaunchWithMessage(client.ExpectErrorResponse(t),
-			"Failed to launch: 'wd' attribute '123' in debug configuration is not a string.")
+			"Failed to launch: 'cwd' attribute '123' in debug configuration is not a string.")
 
 		// Skip detailed message checks for potentially different OS-specific errors.
 		client.LaunchRequest("exec", fixture.Path+"_does_not_exist", stopOnEntry)
@@ -3055,9 +3094,9 @@ func TestBadLaunchRequests(t *testing.T) {
 		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"})
+		client.LaunchRequestWithArgs(map[string]interface{}{"mode": "debug", "program": fixture.Source, "noDebug": false, "cwd": "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"})
+		client.LaunchRequestWithArgs(map[string]interface{}{"mode": "debug", "program": fixture.Source, "noDebug": true, "cwd": "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.
diff --git a/service/test/integration2_test.go b/service/test/integration2_test.go
index e7885b7f..ad36d42e 100644
--- a/service/test/integration2_test.go
+++ b/service/test/integration2_test.go
@@ -2318,3 +2318,34 @@ func TestToggleBreakpointRestart(t *testing.T) {
 		assertNoDuplicateBreakpoints(t, c)
 	})
 }
+
+func TestStopServerWithClosedListener(t *testing.T) {
+	// Checks that the error erturned by listener.Accept() is ignored when we
+	// are trying to shutdown. See issue #1633.
+	if testBackend == "rr" || buildMode == "pie" {
+		t.Skip("N/A")
+	}
+	listener, err := net.Listen("tcp", "localhost:0")
+	assertNoError(err, t, "listener")
+	fixture := protest.BuildFixture("math", 0)
+	server := rpccommon.NewServer(&service.Config{
+		Listener:           listener,
+		AcceptMulti:        false,
+		APIVersion:         2,
+		CheckLocalConnUser: true,
+		DisconnectChan:     make(chan struct{}),
+		ProcessArgs:        []string{fixture.Path},
+		Debugger: debugger.Config{
+			WorkingDir:  ".",
+			Backend:     "default",
+			Foreground:  false,
+			BuildFlags:  "",
+			ExecuteKind: debugger.ExecutingGeneratedFile,
+		},
+	})
+	assertNoError(server.Run(), t, "blah")
+	time.Sleep(1 * time.Second) // let server start
+	server.Stop()
+	listener.Close()
+	time.Sleep(1 * time.Second) // give time to server to panic
+}