Address races in test cases (#1102)

And add `go test -race` to CI

Charlie Egan <[email protected]>

.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:

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
 	}

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

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(),
 	)

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

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.