From 19fb30d1d24fd3c7ab2f8e46efa08d1bf17e5397 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Fri, 18 Nov 2022 16:29:11 -0500 Subject: [PATCH] gopls/internal/lsp/cache: fix data race in Symbols We forgot to acquire a lock while accessing snapshot.files. Also, upgrade errgroup and use its new SetLimit feature. Change-Id: Iff1449fd727f0766f6c81391acccaa21951f59be Reviewed-on: https://go-review.googlesource.com/c/tools/+/452058 Auto-Submit: Alan Donovan Run-TryBot: Alan Donovan Reviewed-by: Robert Findley TryBot-Result: Gopher Robot --- gopls/internal/lsp/cache/snapshot.go | 29 ++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go index ae5fe28825..c2cb946afa 100644 --- a/gopls/internal/lsp/cache/snapshot.go +++ b/gopls/internal/lsp/cache/snapshot.go @@ -1094,32 +1094,37 @@ func (s *snapshot) activePackageHandles(ctx context.Context) ([]*packageHandle, // Symbols extracts and returns the symbols for each file in all the snapshot's views. func (s *snapshot) Symbols(ctx context.Context) map[span.URI][]source.Symbol { + // Read the set of Go files out of the snapshot. + var goFiles []source.VersionedFileHandle + s.mu.Lock() + s.files.Range(func(uri span.URI, f source.VersionedFileHandle) { + if s.View().FileKind(f) == source.Go { + goFiles = append(goFiles, f) + } + }) + s.mu.Unlock() + + // Symbolize them in parallel. var ( group errgroup.Group - nprocs = 2 * runtime.GOMAXPROCS(-1) // symbolize is a mix of I/O and CPU - iolimit = make(chan struct{}, nprocs) // I/O limiting counting semaphore + nprocs = 2 * runtime.GOMAXPROCS(-1) // symbolize is a mix of I/O and CPU resultMu sync.Mutex result = make(map[span.URI][]source.Symbol) ) - s.files.Range(func(uri span.URI, f source.VersionedFileHandle) { - if s.View().FileKind(f) != source.Go { - return // workspace symbols currently supports only Go files. - } - - // TODO(adonovan): upgrade errgroup and use group.SetLimit(nprocs). - iolimit <- struct{}{} // acquire token + group.SetLimit(nprocs) + for _, f := range goFiles { + f := f group.Go(func() error { - defer func() { <-iolimit }() // release token symbols, err := s.symbolize(ctx, f) if err != nil { return err } resultMu.Lock() - result[uri] = symbols + result[f.URI()] = symbols resultMu.Unlock() return nil }) - }) + } // Keep going on errors, but log the first failure. // Partial results are better than no symbol results. if err := group.Wait(); err != nil {