internal/lsp: decouple client and server debug

This uses log messages to convey information to the debug system, which
has the benefit of logging even if the debug pages are not active and
also not requiring systems to reach into the debug system or require
extra lifetime tracking Not all things are decoupled yet as there are a
couple of places (notably the handshaker) that read information out of
the debug system.

Change-Id: Iec1f81c34ab3b11b3e3d6e6eb39b98ee5ed0d849
Reviewed-on: https://go-review.googlesource.com/c/tools/+/236337
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>

internal/lsp/cache/cache.go

@@ -13,6 +13,7 @@ import (
 	"strconv"
 	"sync/atomic"
 
+	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/lsp/source"
 	"golang.org/x/tools/internal/memoize"
 	"golang.org/x/tools/internal/span"
@@ -80,6 +81,7 @@ func (c *Cache) NewSession(ctx context.Context) *Session {
 		options:  source.DefaultOptions(),
 		overlays: make(map[span.URI]*overlay),
 	}
+	event.Log(ctx, "New session", KeyCreateSession.Of(s))
 	return s
 }
 

internal/lsp/cache/keys.go

@@ -0,0 +1,52 @@
+// Copyright 2020 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package cache
+
+import (
+	"io"
+
+	"golang.org/x/tools/internal/event/label"
+)
+
+var (
+	KeyCreateSession   = NewSessionKey("create_session", "A new session was added")
+	KeyUpdateSession   = NewSessionKey("update_session", "Updated information about a session")
+	KeyShutdownSession = NewSessionKey("shutdown_session", "A session was shut down")
+)
+
+// SessionKey represents an event label key that has a *Session value.
+type SessionKey struct {
+	name        string
+	description string
+}
+
+// NewSessionKey creates a new Key for *Session values.
+func NewSessionKey(name, description string) *SessionKey {
+	return &SessionKey{name: name, description: description}
+}
+
+func (k *SessionKey) Name() string        { return k.name }
+func (k *SessionKey) Description() string { return k.description }
+
+func (k *SessionKey) Format(w io.Writer, buf []byte, l label.Label) {
+	io.WriteString(w, k.From(l).ID())
+}
+
+// Of creates a new Label with this key and the supplied session.
+func (k *SessionKey) Of(v *Session) label.Label { return label.OfValue(k, v) }
+
+// Get can be used to get the session for the key from a label.Map.
+func (k *SessionKey) Get(lm label.Map) *Session {
+	if t := lm.Find(k); t.Valid() {
+		return k.From(t)
+	}
+	return nil
+}
+
+// From can be used to get the session value from a Label.
+func (k *SessionKey) From(t label.Label) *Session {
+	err, _ := t.UnpackValue().(*Session)
+	return err
+}

internal/lsp/cache/session.go

@@ -68,7 +68,8 @@ func (o *overlay) Session() source.Session { return o.session }
 func (o *overlay) Saved() bool             { return o.saved }
 func (o *overlay) Data() []byte            { return o.text }
 
-func (s *Session) ID() string { return s.id }
+func (s *Session) ID() string     { return s.id }
+func (s *Session) String() string { return s.id }
 
 func (s *Session) Options() source.Options {
 	return s.options
@@ -86,6 +87,7 @@ func (s *Session) Shutdown(ctx context.Context) {
 	}
 	s.views = nil
 	s.viewMap = nil
+	event.Log(ctx, "Shutdown session", KeyShutdownSession.Of(s))
 }
 
 func (s *Session) Cache() source.Cache {

internal/lsp/debug/serve.go

@@ -35,6 +35,7 @@ import (
 	"golang.org/x/tools/internal/lsp/cache"
 	"golang.org/x/tools/internal/lsp/debug/tag"
 	"golang.org/x/tools/internal/lsp/protocol"
+	"golang.org/x/tools/internal/lsp/source"
 	"golang.org/x/xerrors"
 )
 
@@ -149,6 +150,16 @@ func (st *State) Clients() []*Client {
 	return clients
 }
 
+// View returns the View that matches the supplied id.
+func (st *State) Client(id string) *Client {
+	for _, c := range st.Clients() {
+		if c.Session.ID() == id {
+			return c
+		}
+	}
+	return nil
+}
+
 // Servers returns the set of Servers the instance is currently connected to.
 func (st *State) Servers() []*Server {
 	st.mu.Lock()
@@ -160,7 +171,6 @@ func (st *State) Servers() []*Server {
 
 // A Client is an incoming connection from a remote client.
 type Client struct {
-	ID           string
 	Session      *cache.Session
 	DebugAddress string
 	Logfile      string
@@ -178,18 +188,18 @@ type Server struct {
 }
 
 // AddClient adds a client to the set being served.
-func (st *State) AddClient(client *Client) {
+func (st *State) addClient(session *cache.Session) {
 	st.mu.Lock()
 	defer st.mu.Unlock()
-	st.clients = append(st.clients, client)
+	st.clients = append(st.clients, &Client{Session: session})
 }
 
 // DropClient removes a client from the set being served.
-func (st *State) DropClient(id string) {
+func (st *State) dropClient(session source.Session) {
 	st.mu.Lock()
 	defer st.mu.Unlock()
 	for i, c := range st.clients {
-		if c.ID == id {
+		if c.Session == session {
 			copy(st.clients[i:], st.clients[i+1:])
 			st.clients[len(st.clients)-1] = nil
 			st.clients = st.clients[:len(st.clients)-1]
@@ -200,14 +210,14 @@ func (st *State) DropClient(id string) {
 
 // AddServer adds a server to the set being queried. In practice, there should
 // be at most one remote server.
-func (st *State) AddServer(server *Server) {
+func (st *State) addServer(server *Server) {
 	st.mu.Lock()
 	defer st.mu.Unlock()
 	st.servers = append(st.servers, server)
 }
 
 // DropServer drops a server from the set being queried.
-func (st *State) DropServer(id string) {
+func (st *State) dropServer(id string) {
 	st.mu.Lock()
 	defer st.mu.Unlock()
 	for i, s := range st.servers {
@@ -229,15 +239,7 @@ func (i *Instance) getSession(r *http.Request) interface{} {
 }
 
 func (i Instance) getClient(r *http.Request) interface{} {
-	i.State.mu.Lock()
+	return i.State.Client(path.Base(r.URL.Path))
-	defer i.State.mu.Unlock()
-	id := path.Base(r.URL.Path)
-	for _, c := range i.State.clients {
-		if c.ID == id {
-			return c
-		}
-	}
-	return nil
 }
 
 func (i Instance) getServer(r *http.Request) interface{} {
@@ -480,6 +482,34 @@ func makeInstanceExporter(i *Instance) event.Exporter {
 		if i.traces != nil {
 			ctx = i.traces.ProcessEvent(ctx, ev, lm)
 		}
+		if event.IsLog(ev) {
+			if s := cache.KeyCreateSession.Get(ev); s != nil {
+				i.State.addClient(s)
+			}
+			if sid := tag.NewServer.Get(ev); sid != "" {
+				i.State.addServer(&Server{
+					ID:           sid,
+					Logfile:      tag.Logfile.Get(ev),
+					DebugAddress: tag.DebugAddress.Get(ev),
+					GoplsPath:    tag.GoplsPath.Get(ev),
+					ClientID:     tag.ClientID.Get(ev),
+				})
+			}
+			if s := cache.KeyShutdownSession.Get(ev); s != nil {
+				i.State.dropClient(s)
+			}
+			if sid := tag.EndServer.Get(ev); sid != "" {
+				i.State.dropServer(sid)
+			}
+			if s := cache.KeyUpdateSession.Get(ev); s != nil {
+				if c := i.State.Client(s.ID()); c != nil {
+					c.DebugAddress = tag.DebugAddress.Get(ev)
+					c.Logfile = tag.Logfile.Get(ev)
+					c.ServerID = tag.ServerID.Get(ev)
+					c.GoplsPath = tag.GoplsPath.Get(ev)
+				}
+			}
+		}
 		return ctx
 	}
 	metrics := metric.Config{}
@@ -601,7 +631,7 @@ var mainTmpl = template.Must(template.Must(baseTemplate.Clone()).Parse(`
 <h2>Views</h2>
 <ul>{{range .State.Views}}<li>{{.Name}} is {{template "viewlink" .ID}} from {{template "sessionlink" .Session.ID}} in {{.Folder}}</li>{{end}}</ul>
 <h2>Clients</h2>
-<ul>{{range .State.Clients}}<li>{{template "clientlink" .ID}}</li>{{end}}</ul>
+<ul>{{range .State.Clients}}<li>{{template "clientlink" .Session.ID}}</li>{{end}}</ul>
 <h2>Servers</h2>
 <ul>{{range .State.Servers}}<li>{{template "serverlink" .ID}}</li>{{end}}</ul>
 {{end}}
@@ -662,7 +692,7 @@ var cacheTmpl = template.Must(template.Must(baseTemplate.Clone()).Parse(`
 `))
 
 var clientTmpl = template.Must(template.Must(baseTemplate.Clone()).Parse(`
-{{define "title"}}Client {{.ID}}{{end}}
+{{define "title"}}Client {{.Session.ID}}{{end}}
 {{define "body"}}
 Using session: <b>{{template "sessionlink" .Session.ID}}</b><br>
 {{if .DebugAddress}}Debug this client at: <a href="http://{{localAddress .DebugAddress}}">{{localAddress .DebugAddress}}</a><br>{{end}}

internal/lsp/debug/tag/tag.go

@@ -32,6 +32,15 @@ var (
 	Port         = keys.NewInt("port", "")
 	Type         = keys.New("type", "")
 	HoverKind    = keys.NewString("hoverkind", "")
+
+	NewServer = keys.NewString("new_server", "A new server was added")
+	EndServer = keys.NewString("end_server", "A server was shut down")
+
+	ServerID     = keys.NewString("server", "The server ID an event is related to")
+	Logfile      = keys.NewString("logfile", "")
+	DebugAddress = keys.NewString("debug_address", "")
+	GoplsPath    = keys.NewString("gopls_path", "")
+	ClientID     = keys.NewString("client_id", "")
 )
 
 var (

internal/lsp/lsprpc/lsprpc.go

@@ -22,6 +22,7 @@ import (
 	"golang.org/x/tools/internal/lsp"
 	"golang.org/x/tools/internal/lsp/cache"
 	"golang.org/x/tools/internal/lsp/debug"
+	"golang.org/x/tools/internal/lsp/debug/tag"
 	"golang.org/x/tools/internal/lsp/protocol"
 )
 
@@ -30,7 +31,7 @@ import (
 const AutoNetwork = "auto"
 
 // Unique identifiers for client/server.
-var clientIndex, serverIndex int64
+var serverIndex int64
 
 // The StreamServer type is a jsonrpc2.StreamServer that handles incoming
 // streams as a new LSP session, using a shared cache.
@@ -51,15 +52,8 @@ func NewStreamServer(cache *cache.Cache) *StreamServer {
 // ServeStream implements the jsonrpc2.StreamServer interface, by handling
 // incoming streams using a new lsp server.
 func (s *StreamServer) ServeStream(ctx context.Context, conn jsonrpc2.Conn) error {
-	id := strconv.FormatInt(atomic.AddInt64(&clientIndex, 1), 10)
-
 	client := protocol.ClientDispatcher(conn)
 	session := s.cache.NewSession(ctx)
-	dc := &debug.Client{ID: id, Session: session}
-	if di := debug.GetInstance(ctx); di != nil {
-		di.State.AddClient(dc)
-		defer di.State.DropClient(id)
-	}
 	server := s.serverForTest
 	if server == nil {
 		server = lsp.NewServer(session, client)
@@ -80,7 +74,7 @@ func (s *StreamServer) ServeStream(ctx context.Context, conn jsonrpc2.Conn) erro
 	ctx = protocol.WithClient(ctx, client)
 	conn.Go(ctx,
 		protocol.Handlers(
-			handshaker(dc, executable,
+			handshaker(session, executable,
 				protocol.ServerHandler(server,
 					jsonrpc2.MethodNotFound))))
 	<-conn.Done()
@@ -205,7 +199,6 @@ func (f *Forwarder) ServeStream(ctx context.Context, clientConn jsonrpc2.Conn) e
 	// Do a handshake with the server instance to exchange debug information.
 	index := atomic.AddInt64(&serverIndex, 1)
 	serverID := strconv.FormatInt(index, 10)
-	di := debug.GetInstance(ctx)
 	var (
 		hreq = handshakeRequest{
 			ServerID:  serverID,
@@ -213,7 +206,7 @@ func (f *Forwarder) ServeStream(ctx context.Context, clientConn jsonrpc2.Conn) e
 		}
 		hresp handshakeResponse
 	)
-	if di != nil {
+	if di := debug.GetInstance(ctx); di != nil {
 		hreq.Logfile = di.Logfile
 		hreq.DebugAddr = di.ListenedDebugAddress
 	}
@@ -223,15 +216,13 @@ func (f *Forwarder) ServeStream(ctx context.Context, clientConn jsonrpc2.Conn) e
 	if hresp.GoplsPath != f.goplsPath {
 		event.Error(ctx, "", fmt.Errorf("forwarder: gopls path mismatch: forwarder is %q, remote is %q", f.goplsPath, hresp.GoplsPath))
 	}
-	if di != nil {
+	event.Log(ctx, "New server",
-		di.State.AddServer(&debug.Server{
+		tag.NewServer.Of(serverID),
-			ID:           serverID,
+		tag.Logfile.Of(hresp.Logfile),
-			Logfile:      hresp.Logfile,
+		tag.DebugAddress.Of(hresp.DebugAddr),
-			DebugAddress: hresp.DebugAddr,
+		tag.GoplsPath.Of(hresp.GoplsPath),
-			GoplsPath:    hresp.GoplsPath,
+		tag.ClientID.Of(hresp.SessionID),
-			ClientID:     hresp.ClientID,
+	)
-		})
-	}
 	clientConn.Go(ctx,
 		protocol.Handlers(
 			protocol.ServerHandler(server,
@@ -346,8 +337,6 @@ type handshakeRequest struct {
 // A handshakeResponse is returned by the LSP server to tell the LSP client
 // information about its session.
 type handshakeResponse struct {
-	// ClientID is the ID of the client as seen on the server.
-	ClientID string `json:"clientID"`
 	// SessionID is the server session associated with the client.
 	SessionID string `json:"sessionID"`
 	// Logfile is the location of the server logs.
@@ -363,7 +352,6 @@ type handshakeResponse struct {
 // that it looks similar to handshakeResposne, but in fact 'Logfile' and
 // 'DebugAddr' now refer to the client.
 type ClientSession struct {
-	ClientID  string `json:"clientID"`
 	SessionID string `json:"sessionID"`
 	Logfile   string `json:"logfile"`
 	DebugAddr string `json:"debugAddr"`
@@ -385,7 +373,7 @@ const (
 	sessionsMethod  = "gopls/sessions"
 )
 
-func handshaker(client *debug.Client, goplsPath string, handler jsonrpc2.Handler) jsonrpc2.Handler {
+func handshaker(session *cache.Session, goplsPath string, handler jsonrpc2.Handler) jsonrpc2.Handler {
 	return func(ctx context.Context, reply jsonrpc2.Replier, r jsonrpc2.Request) error {
 		switch r.Method() {
 		case handshakeMethod:
@@ -394,13 +382,15 @@ func handshaker(client *debug.Client, goplsPath string, handler jsonrpc2.Handler
 				sendError(ctx, reply, err)
 				return nil
 			}
-			client.DebugAddress = req.DebugAddr
+			event.Log(ctx, "Handshake session update",
-			client.Logfile = req.Logfile
+				cache.KeyUpdateSession.Of(session),
-			client.ServerID = req.ServerID
+				tag.DebugAddress.Of(req.DebugAddr),
-			client.GoplsPath = req.GoplsPath
+				tag.Logfile.Of(req.Logfile),
+				tag.ServerID.Of(req.ServerID),
+				tag.GoplsPath.Of(req.GoplsPath),
+			)
 			resp := handshakeResponse{
-				ClientID:  client.ID,
+				SessionID: session.ID(),
-				SessionID: client.Session.ID(),
 				GoplsPath: goplsPath,
 			}
 			if di := debug.GetInstance(ctx); di != nil {
@@ -412,15 +402,13 @@ func handshaker(client *debug.Client, goplsPath string, handler jsonrpc2.Handler
 		case sessionsMethod:
 			resp := ServerState{
 				GoplsPath:       goplsPath,
-				CurrentClientID: client.ID,
+				CurrentClientID: session.ID(),
 			}
-			//TODO: this should not need access to the debug information
 			if di := debug.GetInstance(ctx); di != nil {
 				resp.Logfile = di.Logfile
 				resp.DebugAddr = di.ListenedDebugAddress
 				for _, c := range di.State.Clients() {
 					resp.Clients = append(resp.Clients, ClientSession{
-						ClientID:  c.ID,
 						SessionID: c.Session.ID(),
 						Logfile:   c.Logfile,
 						DebugAddr: c.DebugAddress,