PenaDevops/delve
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

service/dap: improve shutdown logic and test coverage (#2749)

Browse Source 
This high-level goal is to push more shutdown logic into Session and not rely Server.Stop() because dap's version of Stop() is different from rpccommon. That means triggering the same shutdown steps in multiple places, so additional safeguards are added to make some of the duplicate steps no-ops to avoid errors, extra logging, etc.

This includes the following changes:

close client conn when request loop in serveDAPCodec exits (to match rpccommon)
exit request loop after processing a disconnect request (instead of relying on next read to fail because conn got closed by triggered Stop(), which is skipped in case of accept-multiclient and is not closed in Stop() in rpccommon)
reset debugger to nil to avoid shutting it down more than once and causing duplicate logging (in case stopDebugSession is called from onDisconnectRequest and Server.Stop or Session.Close as things shut down)
reset binaryToRemove to "" upon removal to avoid duplicate error (in case Close() is called more than once outside of Stop(), which technically is not the case right now)
expand testing for all possible server shutdown triggers
testing for Session-only shutdown as it will be integrated into rpccommon without dap.Server wrapper
updates golang/vscode-go#1830
updates #2328

Co-authored-by: Polina Sokolova <polinasok@users.noreply.github.com>
This commit is contained in:
polinasok 2021-10-26 10:11:55 -07:00 committed by GitHub
parent b31565d8aa
commit b48ceec161
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 223 additions and 54 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

52
service/dap/server.go
View File

@ -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
}

225
service/dap/server_test.go
View File

@ -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