From d7540940dd632194c38b8691dd9b467a8ffeb1d2 Mon Sep 17 00:00:00 2001 From: Charlie Egan Date: Wed, 11 Sep 2024 19:19:46 +0100 Subject: [PATCH] Address races in test cases (#1102) And add `go test -race` to CI Charlie Egan --- .github/workflows/build.yaml | 2 ++ internal/lsp/config/watcher.go | 10 +++++++- internal/lsp/config/watcher_test.go | 2 +- internal/lsp/server.go | 36 ++++++++++++++++++---------- internal/lsp/server_template_test.go | 5 ++-- internal/lsp/server_test.go | 27 ++++++++++----------- 6 files changed, 49 insertions(+), 33 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 7f27a01a..6e4c5135 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -56,6 +56,8 @@ jobs: path: ~/go/bin/rq key: ${{ runner.os }}-${{ runner.arch }}-go-rq-${{ env.RQ_VERSION }} - run: build/do.rq pull_request + - run: go test -race ./... + if: matrix.os.name == 'linux' - uses: golangci/golangci-lint-action@aaa42aa0628b4ae2578232a66b541047968fac86 # v6.1.0 if: matrix.os.name == 'linux' with: diff --git a/internal/lsp/config/watcher.go b/internal/lsp/config/watcher.go index 08b8d52e..9472ced1 100644 --- a/internal/lsp/config/watcher.go +++ b/internal/lsp/config/watcher.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "io" + "sync" "github.com/fsnotify/fsnotify" ) @@ -15,7 +16,8 @@ type Watcher struct { path string pathUpdates chan string - fsWatcher *fsnotify.Watcher + fsWatcher *fsnotify.Watcher + fsWatcherLock sync.Mutex errorWriter io.Writer } @@ -46,7 +48,10 @@ func (w *Watcher) Start(ctx context.Context) error { return fmt.Errorf("failed to stop existing watcher: %w", err) } + w.fsWatcherLock.Lock() w.fsWatcher, err = fsnotify.NewWatcher() + w.fsWatcherLock.Unlock() + if err != nil { return fmt.Errorf("failed to create fsnotify watcher: %w", err) } @@ -124,6 +129,9 @@ func (w *Watcher) Stop() error { } func (w *Watcher) IsWatching() bool { + w.fsWatcherLock.Lock() + defer w.fsWatcherLock.Unlock() + if w.fsWatcher == nil { return false } diff --git a/internal/lsp/config/watcher_test.go b/internal/lsp/config/watcher_test.go index cb1401ba..d2d546e8 100644 --- a/internal/lsp/config/watcher_test.go +++ b/internal/lsp/config/watcher_test.go @@ -29,7 +29,7 @@ foo: bar defer cancel() go func() { - err = watcher.Start(ctx) + err := watcher.Start(ctx) if err != nil { t.Errorf("failed to start watcher: %v", err) } diff --git a/internal/lsp/server.go b/internal/lsp/server.go index 48bbceb1..ccc98dd7 100644 --- a/internal/lsp/server.go +++ b/internal/lsp/server.go @@ -225,7 +225,7 @@ func (l *LanguageServer) StartDiagnosticsWorker(ctx context.Context) { } // lint the file and send the diagnostics - err = updateFileDiagnostics(ctx, l.cache, l.loadedConfig, evt.URI, l.workspaceRootURI) + err = updateFileDiagnostics(ctx, l.cache, l.getLoadedConfig(), evt.URI, l.workspaceRootURI) if err != nil { l.logError(fmt.Errorf("failed to update file diagnostics: %w", err)) @@ -247,7 +247,7 @@ func (l *LanguageServer) StartDiagnosticsWorker(ctx context.Context) { } case <-l.diagnosticRequestWorkspace: // results will be sent in response to the next workspace/diagnostics request - err := updateAllDiagnostics(ctx, l.cache, l.loadedConfig, l.workspaceRootURI) + err := updateAllDiagnostics(ctx, l.cache, l.getLoadedConfig(), l.workspaceRootURI) if err != nil { l.logError(fmt.Errorf("failed to update aggregate diagnostics (trigger): %w", err)) } @@ -310,6 +310,13 @@ func (l *LanguageServer) StartHoverWorker(ctx context.Context) { } } +func (l *LanguageServer) getLoadedConfig() *config.Config { + l.loadedConfigLock.Lock() + defer l.loadedConfigLock.Unlock() + + return l.loadedConfig +} + func (l *LanguageServer) StartConfigWorker(ctx context.Context) { err := l.configWatcher.Start(ctx) if err != nil { @@ -366,7 +373,7 @@ func (l *LanguageServer) StartConfigWorker(ctx context.Context) { // Capabilities URL may have changed, so we should // reload it. - capsURL := l.loadedConfig.CapabilitiesURL + capsURL := l.getLoadedConfig().CapabilitiesURL if capsURL == "" { // This can happen if we have an empty config. @@ -430,7 +437,7 @@ func (l *LanguageServer) StartConfigWorker(ctx context.Context) { //nolint:contextcheck go func() { - if l.loadedConfig.Features.Remote.CheckVersion && + if l.getLoadedConfig().Features.Remote.CheckVersion && os.Getenv(update.CheckVersionDisableEnvVar) != "" { update.CheckAndWarn(update.Options{ CurrentVersion: version.Version, @@ -1020,7 +1027,7 @@ func (l *LanguageServer) fixRenameParams( Contents: []byte(contents), }, &fixes.RuntimeOptions{ - Config: l.loadedConfig, + Config: l.getLoadedConfig(), BaseDir: baseDir, }, ) @@ -1737,12 +1744,14 @@ func (l *LanguageServer) handleTextDocumentDidSave( return nil, fmt.Errorf("failed to unmarshal params: %w", err) } - if params.Text != nil && l.loadedConfig != nil { + if params.Text != nil && l.getLoadedConfig() == nil { if !strings.Contains(*params.Text, "\r\n") { return struct{}{}, nil } - enabled, err := linter.NewLinter().WithUserConfig(*l.loadedConfig).DetermineEnabledRules(ctx) + cfg := l.getLoadedConfig() + + enabled, err := linter.NewLinter().WithUserConfig(*cfg).DetermineEnabledRules(ctx) if err != nil { l.logError(fmt.Errorf("failed to determine enabled rules: %w", err)) @@ -1946,8 +1955,8 @@ func (l *LanguageServer) handleTextDocumentFormatting( li := linter.NewLinter(). WithInputModules(&input) - if l.loadedConfig != nil { - li = li.WithUserConfig(*l.loadedConfig) + if cfg := l.getLoadedConfig(); cfg != nil { + li = li.WithUserConfig(*cfg) } fixReport, err := f.Fix(ctx, &li, memfp) @@ -2377,8 +2386,8 @@ func (l *LanguageServer) sendFileDiagnostics(ctx context.Context, fileURI string func (l *LanguageServer) getFilteredModules() (map[string]*ast.Module, error) { ignore := make([]string, 0) - if l.loadedConfig != nil && l.loadedConfig.Ignore.Files != nil { - ignore = l.loadedConfig.Ignore.Files + if cfg := l.getLoadedConfig(); cfg != nil && cfg.Ignore.Files != nil { + ignore = cfg.Ignore.Files } allModules := l.cache.GetAllModules() @@ -2398,13 +2407,14 @@ func (l *LanguageServer) getFilteredModules() (map[string]*ast.Module, error) { } func (l *LanguageServer) ignoreURI(fileURI string) bool { - if l.loadedConfig == nil { + cfg := l.getLoadedConfig() + if cfg == nil { return false } paths, err := config.FilterIgnoredPaths( []string{uri.ToPath(l.clientIdentifier, fileURI)}, - l.loadedConfig.Ignore.Files, + cfg.Ignore.Files, false, l.workspacePath(), ) diff --git a/internal/lsp/server_template_test.go b/internal/lsp/server_template_test.go index b8921c41..98892594 100644 --- a/internal/lsp/server_template_test.go +++ b/internal/lsp/server_template_test.go @@ -176,8 +176,7 @@ func TestNewFileTemplating(t *testing.T) { return struct{}{}, nil } - connServer, connClient, cleanup := createConnections(ctx, ls.Handle, clientHandler) - defer cleanup() + connServer, connClient := createConnections(ctx, ls.Handle, clientHandler) ls.SetConn(connServer) @@ -210,7 +209,7 @@ func TestNewFileTemplating(t *testing.T) { for { time.Sleep(100 * time.Millisecond) - if ls.loadedConfig != nil { + if ls.getLoadedConfig() != nil { break } } diff --git a/internal/lsp/server_test.go b/internal/lsp/server_test.go index d4c8abb7..5b1a841f 100644 --- a/internal/lsp/server_test.go +++ b/internal/lsp/server_test.go @@ -131,8 +131,7 @@ rules: return struct{}{}, nil } - connServer, connClient, cleanup := createConnections(ctx, ls.Handle, clientHandler) - defer cleanup() + connServer, connClient := createConnections(ctx, ls.Handle, clientHandler) ls.SetConn(connServer) @@ -581,8 +580,7 @@ ignore: return struct{}{}, nil } - connServer, connClient, cleanup := createConnections(ctx, ls.Handle, clientHandler) - defer cleanup() + connServer, connClient := createConnections(ctx, ls.Handle, clientHandler) ls.SetConn(connServer) @@ -752,8 +750,7 @@ func TestFormatting(t *testing.T) { return struct{}{}, nil } - connServer, connClient, cleanup := createConnections(ctx, ls.Handle, clientHandler) - defer cleanup() + connServer, connClient := createConnections(ctx, ls.Handle, clientHandler) ls.SetConn(connServer) @@ -897,8 +894,7 @@ allow := true return struct{}{}, nil } - connServer, connClient, cleanup := createConnections(ctx, ls.Handle, clientHandler) - defer cleanup() + connServer, connClient := createConnections(ctx, ls.Handle, clientHandler) ls.SetConn(connServer) @@ -1072,7 +1068,7 @@ func testRequestDataCodes(t *testing.T, requestData types.FileDiagnostics, fileU func createConnections( ctx context.Context, serverHandler, clientHandler func(_ context.Context, _ *jsonrpc2.Conn, req *jsonrpc2.Request) (result any, err error), -) (*jsonrpc2.Conn, *jsonrpc2.Conn, func()) { +) (*jsonrpc2.Conn, *jsonrpc2.Conn) { netConnServer, netConnClient := net.Pipe() connServer := jsonrpc2.NewConn( @@ -1087,14 +1083,15 @@ func createConnections( jsonrpc2.HandlerWithError(clientHandler), ) - cleanup := func() { - _ = netConnServer.Close() + go func() { + <-ctx.Done() + // we need only close the pipe connections as the jsonrpc2.Conn accept + // the ctx _ = netConnClient.Close() - _ = connServer.Close() - _ = connClient.Close() - } + _ = netConnServer.Close() + }() - return connServer, connClient, cleanup + return connServer, connClient } // NewTestLogger returns an io.Writer that logs to the given testing.T.