From 87c45e44d578c0df5c91ebf34effc1a9d3a87b6b Mon Sep 17 00:00:00 2001 From: Edward McFarlane Date: Mon, 24 Mar 2025 15:35:31 +0100 Subject: [PATCH 1/3] LSP ensure checks diagnostics are published This ensures checks diagnostics are always published to the LSP client. We adopt a worker model that runs checks in the background. On updates, the caller invalidates the previous checks. Checks are then re-evaluated and published. This ensures checks are eventually consistent. --- private/buf/buflsp/file.go | 85 ++++++++++++++++++++++++++------------ 1 file changed, 59 insertions(+), 26 deletions(-) diff --git a/private/buf/buflsp/file.go b/private/buf/buflsp/file.go index 4d835474ef..a54cb6717e 100644 --- a/private/buf/buflsp/file.go +++ b/private/buf/buflsp/file.go @@ -45,16 +45,17 @@ import ( ) const ( - descriptorPath = "google/protobuf/descriptor.proto" - refreshCheckStagger = 5 * time.Millisecond + descriptorPath = "google/protobuf/descriptor.proto" + checkRefreshPeriod = 3 * time.Second ) // file is a file that has been opened by the client. // // Mutating a file is thread-safe. type file struct { - lsp *lsp - uri protocol.URI + lsp *lsp + uri protocol.URI + checkWork chan<- struct{} text string // Version is an opaque version identifier given to us by the LSP client. This @@ -134,6 +135,10 @@ func (f *file) Reset(ctx context.Context) { // for this file. func (f *file) Close(ctx context.Context) { f.lsp.fileManager.Close(ctx, f.uri) + if f.checkWork != nil { + close(f.checkWork) + f.checkWork = nil + } } // IsOpenInEditor returns whether this file was opened in the LSP client's @@ -290,28 +295,7 @@ func (f *file) Refresh(ctx context.Context) { f.FindModule(ctx) progress.Report(ctx, "Running Checks", 4.0/6) - // Since checks are a more expensive operation, we do not want to run a check on every - // Refresh call. Instead, we can stagger the checks and only run them periodically by - // spinning them off into a go routine. Then we attempt to lock using the top-level LSP - // lock. It is safe to use because if another LSP call is made, we allow checks to finish - // before resolving a subsequent LSP request. - go func() { - // We stagger the check operation by 5ms and run it for the latest Refresh state. - time.Sleep(refreshCheckStagger) - // Call TryLock, if unnsuccessful, then another thread holds the lock, so we provide a - // debug log and move on. - if !f.lsp.lock.TryLock() { - f.lsp.logger.Debug( - fmt.Sprintf("another thread holds the LSP lock, no new checks started for %v", f.uri), - ) - return - } - // We have successfully obtained the lock, we can now run the checks. - defer f.lsp.lock.Unlock() - f.BuildImages(ctx) - f.RunLints(ctx) - f.RunBreaking(ctx) - }() + f.RunChecks(ctx) progress.Report(ctx, "Indexing Symbols", 5.0/6) f.IndexSymbols(ctx) @@ -324,6 +308,55 @@ func (f *file) Refresh(ctx context.Context) { f.PublishDiagnostics(ctx) } +// RunChecks starts the process of running checks on this file. Returns +// immediately. +// +// Checks are run in a background goroutine to avoid blocking this LSP call. +// Every time RunChecks is called, the current check goroutine is invalidated +// causing checks to be re-run. Checks will lock the LSP mutex. +// If another LSP call is made, we allow checks to finish before resolving a +// subsequent LSP request. +func (f *file) RunChecks(ctx context.Context) { + // If we have not yet started a goroutine to run checks, start one. + // This goroutine will run checks in the background and publish diagnostics. + // We debounce checks to avoid spamming the client. + if f.checkWork == nil { + // We use a buffered channel of length one as the check invalidation mechanism. + work := make(chan struct{}, 1) + f.checkWork = work + runChecks := func(ctx context.Context) { + f.lsp.lock.Lock() + defer f.lsp.lock.Unlock() + if !f.IsOpenInEditor() { + // If the file is not open in the editor, we do not need to run checks. + return + } + f.lsp.logger.Info(fmt.Sprintf("running checks for %v, %v", f.uri, f.version)) + f.BuildImages(ctx) + f.RunLints(ctx) + f.RunBreaking(ctx) + // Publish the latest diagnostics. + f.PublishDiagnostics(ctx) + } + // Start a goroutine to process checks. + go func() { + // Child context may outlive the parent RPC context. + ctx := context.WithoutCancel(ctx) + for range work { + runChecks(ctx) + // Ensure checks are debounced to avoid spamming the client. + time.Sleep(checkRefreshPeriod) + } + }() + } + // Write to the work channel to invalidate the current checks. + select { + case f.checkWork <- struct{}{}: + default: + // Channel is full, already invalidated. + } +} + // RefreshAST reparses the file and generates diagnostics if necessary. // // Returns whether a reparse was necessary. From d4fa517b8bdd47cfe82a2d9dfb6d9cd921720359 Mon Sep 17 00:00:00 2001 From: Edward McFarlane Date: Mon, 24 Mar 2025 23:12:00 +0100 Subject: [PATCH 2/3] Update docs --- private/buf/buflsp/file.go | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/private/buf/buflsp/file.go b/private/buf/buflsp/file.go index a54cb6717e..2919463446 100644 --- a/private/buf/buflsp/file.go +++ b/private/buf/buflsp/file.go @@ -308,14 +308,17 @@ func (f *file) Refresh(ctx context.Context) { f.PublishDiagnostics(ctx) } -// RunChecks starts the process of running checks on this file. Returns -// immediately. +// RunChecks initiates background checks (lint and breaking) on this file and +// returns immediately. // -// Checks are run in a background goroutine to avoid blocking this LSP call. -// Every time RunChecks is called, the current check goroutine is invalidated -// causing checks to be re-run. Checks will lock the LSP mutex. -// If another LSP call is made, we allow checks to finish before resolving a -// subsequent LSP request. +// Checks are executed in a background goroutine to avoid blocking the LSP +// call. Each call to RunChecks invalidates any ongoing checks, triggering a +// fresh run. The checks acquire the LSP mutex. Subsequent LSP calls will wait +// for the current check to complete before proceeding. +// +// Checks are debounce (with the delay defined by checkRefreshPeriod) to avoid +// overwhelming the client with expensive checks. If the file is not open in the +// editor, checks are skipped. Diagnostics are published after checks are run. func (f *file) RunChecks(ctx context.Context) { // If we have not yet started a goroutine to run checks, start one. // This goroutine will run checks in the background and publish diagnostics. @@ -328,32 +331,31 @@ func (f *file) RunChecks(ctx context.Context) { f.lsp.lock.Lock() defer f.lsp.lock.Unlock() if !f.IsOpenInEditor() { - // If the file is not open in the editor, we do not need to run checks. + // Skip checks if the file is not open in the editor. return } f.lsp.logger.Info(fmt.Sprintf("running checks for %v, %v", f.uri, f.version)) f.BuildImages(ctx) f.RunLints(ctx) f.RunBreaking(ctx) - // Publish the latest diagnostics. - f.PublishDiagnostics(ctx) + f.PublishDiagnostics(ctx) // Publish the latest diagnostics. } // Start a goroutine to process checks. go func() { - // Child context may outlive the parent RPC context. + // Detach from the parent RPC context. ctx := context.WithoutCancel(ctx) for range work { runChecks(ctx) - // Ensure checks are debounced to avoid spamming the client. + // Debounce checks to prevent thrashing expensive checks. time.Sleep(checkRefreshPeriod) } }() } - // Write to the work channel to invalidate the current checks. + // Signal the goroutine to invalidate and rerun checks. select { case f.checkWork <- struct{}{}: default: - // Channel is full, already invalidated. + // Channel is full, checks are already invalidated and will be rerun. } } From adec6f17860b768938a7d226d329378fac7dfdd1 Mon Sep 17 00:00:00 2001 From: Edward McFarlane Date: Mon, 24 Mar 2025 23:20:25 +0100 Subject: [PATCH 3/3] Docs --- private/buf/buflsp/file.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/private/buf/buflsp/file.go b/private/buf/buflsp/file.go index 2919463446..e6f3b4f8b2 100644 --- a/private/buf/buflsp/file.go +++ b/private/buf/buflsp/file.go @@ -313,8 +313,9 @@ func (f *file) Refresh(ctx context.Context) { // // Checks are executed in a background goroutine to avoid blocking the LSP // call. Each call to RunChecks invalidates any ongoing checks, triggering a -// fresh run. The checks acquire the LSP mutex. Subsequent LSP calls will wait -// for the current check to complete before proceeding. +// fresh run. However, previous checks are not interrupted. The checks acquire +// the LSP mutex. Subsequent LSP calls will wait for the current check to +// complete before proceeding. // // Checks are debounce (with the delay defined by checkRefreshPeriod) to avoid // overwhelming the client with expensive checks. If the file is not open in the