diff --git a/lsp/cache/graph.go b/lsp/cache/graph.go index 5a65038..ff83cb1 100644 --- a/lsp/cache/graph.go +++ b/lsp/cache/graph.go @@ -56,11 +56,18 @@ func NewIncludeGraph() *IncludeGraph { } } +// Get returns a copy of file's node. The graph's nodes are mutated in place +// by Set and removeWithoutLock, so a live node must never escape the lock. func (g *IncludeGraph) Get(file uri.URI) *IncludeNode { g.mu.RLock() defer g.mu.RUnlock() - return g.mapper[file] + node := g.mapper[file] + if node == nil { + return nil + } + + return node.Clone() } // Set records the include edges of file. resolve maps an include path text diff --git a/lsp/cache/parse.go b/lsp/cache/parse.go index 1b878f6..aefb7b3 100644 --- a/lsp/cache/parse.go +++ b/lsp/cache/parse.go @@ -214,7 +214,8 @@ type ParsedFile struct { errs []syntax.Error // tokens is the identifier set of ast, computed lazily once per parse. - tokens map[string]struct{} + tokensOnce sync.Once + tokens map[string]struct{} // defs and enumValues index the file's definitions, computed lazily // once per parse. A re-parse replaces the whole ParsedFile, so the @@ -241,18 +242,16 @@ func (p *ParsedFile) Errors() []syntax.Error { // reused. A re-parse replaces the whole ParsedFile, so the cache never // goes stale. func (p *ParsedFile) Tokens() map[string]struct{} { - if p.tokens != nil { - return p.tokens - } - - tokens := make(map[string]struct{}) - if p.ast != nil { - collectTokens(p.ast, tokens) - } + p.tokensOnce.Do(func() { + tokens := make(map[string]struct{}) + if p.ast != nil { + collectTokens(p.ast, tokens) + } - p.tokens = tokens + p.tokens = tokens + }) - return tokens + return p.tokens } // Definitions returns the file's top-level definitions indexed by name: diff --git a/lsp/cache/session.go b/lsp/cache/session.go index 85c21d4..117f600 100644 --- a/lsp/cache/session.go +++ b/lsp/cache/session.go @@ -12,11 +12,6 @@ import ( type Session struct { id int64 - // indicates whether initialized with initialize params - // if initialize params doesn't contain any folder. this is false - initializedMu sync.Mutex - initialized bool - // cache is shared global cache *Cache @@ -41,24 +36,6 @@ func NewSession(cache *Cache) *Session { return sess } -func (s *Session) Initialize(fn func()) { - s.initializedMu.Lock() - defer s.initializedMu.Unlock() - - if s.initialized { - return - } - - s.initialized = true - - fn() -} - -func (s *Session) CreateView(folder uri.URI) { - view := NewView(folder.Path(), folder, s.overlayFS, s.cache.IncludePaths) - s.views = append(s.views, view) -} - // AddView registers a view for the workspace folder, returning the // existing view when the folder is already tracked. func (s *Session) AddView(folder uri.URI) *View { diff --git a/lsp/cache/snapshot.go b/lsp/cache/snapshot.go index c1809f6..36c1a6f 100644 --- a/lsp/cache/snapshot.go +++ b/lsp/cache/snapshot.go @@ -188,9 +188,6 @@ func (s *Snapshot) Acquire() func() { return s.refCount.Done } -func (s *Snapshot) Initialize(ctx context.Context) { -} - func (s *Snapshot) Graph() *IncludeGraph { return s.context.graph } diff --git a/lsp/cache/view.go b/lsp/cache/view.go index 007b920..1b5aca6 100644 --- a/lsp/cache/view.go +++ b/lsp/cache/view.go @@ -55,15 +55,6 @@ func NewView(name string, folder uri.URI, fs FileSource, includePaths []string) view.snapshotRelease = view.snapshot.Acquire() - asyncRelease := view.snapshot.Acquire() - go func() { - defer asyncRelease() - - view.snapshotMu.Lock() - view.snapshot.Initialize(context.Background()) - view.snapshotMu.Unlock() - }() - return view } @@ -132,19 +123,22 @@ func (v *View) FileChange(ctx context.Context, changes []*FileChange, postFns .. v.MarkFileKnown(change.URI) } - // Swap in the new snapshot. + // Swap in the new snapshot. The pointer and its release are a single + // unit guarded by snapshotMu: FileChange runs concurrently (request + // handlers, the workspace walk), and an unlocked swap could release the + // same snapshot twice (negative WaitGroup panic) or let a reader see a + // released snapshot. + v.snapshotMu.Lock() newSnapshot, release := v.snapshot.clone() v.snapshotRelease() - v.snapshotMu.Lock() - v.snapshot = newSnapshot for _, change := range changes { - v.snapshot.ForgetFile(change.URI) + newSnapshot.ForgetFile(change.URI) } - v.snapshotMu.Unlock() v.snapshotRelease = release + v.snapshotMu.Unlock() - asyncRelease := v.snapshot.Acquire() + asyncRelease := newSnapshot.Acquire() // Re-parse the changed files so the snapshot's include edges and // parsed caches reflect the change before any request observes it. @@ -158,12 +152,12 @@ func (v *View) FileChange(ctx context.Context, changes []*FileChange, postFns .. slices.Sort(uris) for _, uri := range uris { - if _, err := v.snapshot.Parse(ctx, uri); err != nil { + if _, err := newSnapshot.Parse(ctx, uri); err != nil { slog.Error("parse error", "err", err) } } - affected := v.affectedFiles(changes) + affected := v.affectedFiles(changes, newSnapshot) go func() { defer asyncRelease() @@ -175,10 +169,10 @@ func (v *View) FileChange(ctx context.Context, changes []*FileChange, postFns .. } // affectedFiles returns the changed URIs plus the transitive dependents of -// each, deduped. Dependents are computed on the current snapshot, after the -// changes were applied and re-parsed, so the edges reflect the change. +// each, deduped. Dependents are computed on ss, the snapshot the changes +// were applied to, so the edges reflect the change. // Changes come first, in order; dependents follow, sorted by URI. -func (v *View) affectedFiles(changes []*FileChange) []uri.URI { +func (v *View) affectedFiles(changes []*FileChange, ss *Snapshot) []uri.URI { affected := make([]uri.URI, 0, len(changes)) seen := make(map[uri.URI]struct{}, len(changes)) @@ -190,9 +184,7 @@ func (v *View) affectedFiles(changes []*FileChange) []uri.URI { seen[change.URI] = struct{}{} affected = append(affected, change.URI) - v.snapshotMu.Lock() - deps := v.snapshot.Dependents(change.URI) - v.snapshotMu.Unlock() + deps := ss.Dependents(change.URI) for _, dep := range deps { if _, ok := seen[dep]; ok { diff --git a/lsp/impl.go b/lsp/impl.go index 65d2ba3..3f6dbb8 100644 --- a/lsp/impl.go +++ b/lsp/impl.go @@ -29,7 +29,7 @@ func (s *Server) didOpen(ctx context.Context, params *protocol.DidOpenTextDocume From: cache.FileChangeTypeDidOpen, } - s.session.Initialize(func() { + s.dirWalkOnce.Do(func() { file := change.URI dirPos := strings.LastIndexByte(string(file), '/') @@ -51,14 +51,14 @@ func (s *Server) openFile(ctx context.Context, change *cache.FileChange) error { } } - if _, err := s.session.ViewOf(change.URI); err != nil { - // create view for this folder + // The file's directory becomes the view when no workspace folder + // covers it (single-file mode); AddView dedups and is concurrency-safe. + view, err := s.session.ViewOf(change.URI) + if err != nil { filename := change.URI.Path() - dir := uri.File(path.Dir(filename)) - s.session.CreateView(dir) + view = s.session.AddView(uri.File(path.Dir(filename))) } - view, _ := s.session.ViewOf(change.URI) view.FileChange(ctx, []*cache.FileChange{change}, s.postDiagnostics(ctx, view)) return nil diff --git a/lsp/include_paths_test.go b/lsp/include_paths_test.go index 0bfa092..eb9c6bf 100644 --- a/lsp/include_paths_test.go +++ b/lsp/include_paths_test.go @@ -26,7 +26,7 @@ func TestServerIncludePathsFlow(t *testing.T) { srv := NewServer(c, nil, formatter.DefaultOptions()) // Views are created per workspace folder at initialization. - srv.session.CreateView(uri.File(dir)) + srv.session.AddView(uri.File(dir)) view, err := srv.session.ViewOf(uri.File(filepath.Join(dir, "app.thrift"))) assert.NoError(t, err) diff --git a/lsp/server.go b/lsp/server.go index de1d133..6289cd5 100644 --- a/lsp/server.go +++ b/lsp/server.go @@ -3,6 +3,7 @@ package lsp import ( "context" "log/slog" + "sync" "go.lsp.dev/protocol" "go.lsp.dev/uri" @@ -23,6 +24,14 @@ type Server struct { // walk starts on the Initialized notification so the initialize // handshake never blocks on parsing the workspace. folders []uri.URI + + // workspaceWalkOnce and dirWalkOnce guard the two independent walks: + // the whole workspace on Initialized, and the opened file's directory + // on the first didOpen (single-file mode). They must not share a guard + // — a didOpen racing the Initialized notification would otherwise + // permanently skip the workspace walk. + workspaceWalkOnce sync.Once + dirWalkOnce sync.Once } func NewServer(c *cache.Cache, client protocol.Client, formatOpts formatter.Options) *Server { @@ -45,7 +54,7 @@ func (s *Server) Initialized(ctx context.Context, params *protocol.InitializedPa // The workspace walk parses every thrift file to warm the cache; it // runs once, off the request path, so the initialize handshake and // early requests never block on it. - s.session.Initialize(func() { + s.workspaceWalkOnce.Do(func() { go func() { for _, folder := range s.folders { s.walkFoldersThriftFile(folder)