service/dap: support accept-multiclient shutdown in remote attach mode

service/dap/server.go

@@ -75,11 +75,9 @@ import (
 // error or responding to a (synchronous) DAP disconnect request.
 // Once stop is triggered, the goroutine exits.
 //
-// TODO(polina): add another layer of per-client goroutines to support multiple clients.
-// Note that some requests change the process's environment such
-// as working directory (for example, see DelveCwd of launch configuration).
-// So if we want to reuse this process for multiple debugging sessions
-// we need to address that.
+// Unlike rpccommon, there is not another layer of per-client
+// goroutines here because the dap server does not support
+// multiple clients.
 //
 // (3) Per-request goroutine is started for each asynchronous request
 // that resumes execution. We check if target is running already, so
@@ -248,6 +246,10 @@ func NewServer(config *service.Config) *Server {
 	logger := logflags.DAPLogger()
 	logflags.WriteDAPListeningMessage(config.Listener.Addr())
 	logger.Debug("DAP server pid = ", os.Getpid())
+	if config.AcceptMulti {
+		logger.Warn("DAP server does not support accept-multiclient mode")
+		config.AcceptMulti = false
+	}
 	return &Server{
 		config: &Config{
 			Config:        config,
@@ -430,7 +432,11 @@ func (s *Session) serveDAPCodec() {
 					}
 					s.config.log.Error("DAP error: ", err)
 				}
-				s.config.triggerServerStop()
+				if s.config.AcceptMulti {
+					s.conn.Close()
+				} else {
+					s.config.triggerServerStop()
+				}
 			}
 			return
 		}
@@ -1080,13 +1086,23 @@ func (s *Session) stopNoDebugProcess() {
 // onDisconnectRequest handles the DisconnectRequest. Per the DAP spec,
 // it disconnects the debuggee and signals that the debug adaptor
 // (in our case this TCP server) can be terminated.
-// TODO(polina): differentiate between single- and multi-client
-// server mode when handling requests for debug session shutdown.
 func (s *Session) onDisconnectRequest(request *dap.DisconnectRequest) {
-	defer s.config.triggerServerStop()
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
+	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.
+		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()
+		// TODO(polina): should we always restart the target if it is not running?
+		// There is also suspendDebuggee - TBD if vscode supports it.
+		return
+	}
+
+	defer s.config.triggerServerStop()
 	var err error
 	if s.debugger != nil {
 		// We always kill launched programs.
@@ -1104,9 +1120,7 @@ func (s *Session) onDisconnectRequest(request *dap.DisconnectRequest) {
 		s.send(&dap.DisconnectResponse{Response: *newResponse(request.Request)})
 	}
 	// The debugging session has ended, so we send a terminated event.
-	s.send(&dap.TerminatedEvent{
-		Event: *newEvent("terminated"),
-	})
+	s.send(&dap.TerminatedEvent{Event: *newEvent("terminated")})
 }
 
 // stopDebugSession is called from Stop (main goroutine) and

service/dap/server_test.go

@@ -5617,7 +5617,7 @@ func launchDebuggerWithTargetHalted(t *testing.T, fixture string) *debugger.Debu
 }
 
 // runTestWithDebugger starts the server and sets its debugger, initializes a debug session,
-// runs test, then disconnects. Expects the process at the end of test() to be halted
+// runs test, then disconnects. Expects the process at the end of test() to be halted.
 func runTestWithDebugger(t *testing.T, dbg *debugger.Debugger, test func(c *daptest.Client)) {
 	serverStopped := make(chan struct{})
 	server, _ := startDapServer(t, serverStopped)
@@ -5637,10 +5637,7 @@ func runTestWithDebugger(t *testing.T, dbg *debugger.Debugger, test func(c *dapt
 	test(client)
 
 	client.DisconnectRequest()
-	if server.config.AcceptMulti {
-		// TODO(polina): support multi-client mode
-		t.Fatal("testing of accept-multiclient not yet supporteed")
-	} else if server.config.Debugger.AttachPid == 0 { // launched target
+	if server.config.Debugger.AttachPid == 0 { // launched target
 		client.ExpectOutputEventDetachingKill(t)
 	} else { // attached to target
 		client.ExpectOutputEventDetachingNoKill(t)
@@ -5681,6 +5678,67 @@ func TestAttachRemoteToHaltedTargetContinueOnEntry(t *testing.T) {
 
 // TODO(polina): Running + stop/continue on entry
 
+// 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"
+	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},
+		{"terminate=true", func(c *daptest.Client) { c.DisconnectRequestWithKillOption(true) }, detachingAndTerminating},
+		{"terminate=false", func(c *daptest.Client) { c.DisconnectRequestWithKillOption(false) }, closingClientSession},
+	}
+	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())
+			time.Sleep(100 * time.Millisecond) // Give time for connection to be set as dap.Session
+			server.sessionMu.Lock()
+			if server.session == nil {
+				t.Fatal("dap session is not ready")
+			}
+			// DAP server doesn't support accept-multiclient, but we can use this
+			// hack to test the inner connection logic that can be used by a server that does.
+			server.session.config.AcceptMulti = true
+			// TODO(polina): update once the server interface is refactored to take debugger as arg
+			server.session.debugger = launchDebuggerWithTargetHalted(t, "increment")
+			server.sessionMu.Unlock()
+
+			client.InitializeRequest()
+			client.ExpectInitializeResponseAndCapabilities(t)
+
+			client.AttachRequest(map[string]interface{}{"mode": "remote", "stopOnEntry": true})
+			client.ExpectInitializedEvent(t)
+			client.ExpectAttachResponse(t)
+			client.ConfigurationDoneRequest()
+			client.ExpectStoppedEvent(t)
+			client.ExpectConfigurationDoneResponse(t)
+
+			tc.disconnectRequest(client)
+			e := client.ExpectOutputEvent(t)
+			if matched, _ := regexp.MatchString(tc.expect, e.Body.Output); !matched {
+				t.Errorf("\ngot %#v\nwant Output=%q", e, tc.expect)
+			}
+			client.ExpectDisconnectResponse(t)
+			client.ExpectTerminatedEvent(t)
+			client.Close()
+
+			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.
+				close(forceStop)
+			}
+			<-serverStopped
+		})
+	}
+}
+
 func TestLaunchAttachErrorWhenDebugInProgress(t *testing.T) {
 	runTestWithDebugger(t, launchDebuggerWithTargetHalted(t, "increment"), func(client *daptest.Client) {
 		client.AttachRequest(map[string]interface{}{"mode": "local", "processId": 100})