diff --git a/.golangci.yml b/.golangci.yml index 221bb82007..e238275bb5 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -100,6 +100,11 @@ issues: # trip this off. path: private/buf/bufcli/env.go text: "G101:" + - linters: + - gosec + # G404 checks for use of the ordinary non-CPRNG. + path: private/buf/buflsp/progress.go + text: "G404:" - linters: - containedctx # Type must implement an interface whose methods do not accept context. But this diff --git a/private/buf/buflsp/buflsp.go b/private/buf/buflsp/buflsp.go index 6b3a464244..cd46a0063a 100644 --- a/private/buf/buflsp/buflsp.go +++ b/private/buf/buflsp/buflsp.go @@ -21,7 +21,6 @@ import ( "context" "fmt" "sync/atomic" - "time" "github.com/bufbuild/buf/private/buf/bufctl" "github.com/bufbuild/buf/private/bufpkg/bufcheck" @@ -38,10 +37,6 @@ import ( "go.uber.org/zap" ) -const ( - handlerTimeout = 3 * time.Second -) - // Serve spawns a new LSP server, listening on the given stream. // // Returns a context for managing the server. @@ -177,34 +172,17 @@ func (l *lsp) newHandler() jsonrpc2.Handler { zap.ByteString("params", req.Params()), ) - // Each request is handled in a separate goroutine, and has a fixed timeout. - // This is to enforce responsiveness on the LSP side: if something is going to take - // a long time, it should be offloaded. - ctx, done := context.WithTimeout(ctx, handlerTimeout) ctx = withRequestID(ctx) - go func() { - defer done() - replier := l.adaptReplier(reply, req) - - // Verify that the server has been initialized if this isn't the initialization - // request. - if req.Method() != protocol.MethodInitialize && l.initParams.Load() == nil { - retErr = replier(ctx, nil, fmt.Errorf("the first call to the server must be the %q method", - protocol.MethodInitialize)) - return - } - - retErr = actual(ctx, replier, req) - }() - - <-ctx.Done() - if ctx.Err() == context.DeadlineExceeded { - // Don't return this error; that will kill the whole server! - l.logger.Sugar().Errorf("timed out while handling %s; this is likely a bug", req.Method()) - return nil + replier := l.adaptReplier(reply, req) + + // Verify that the server has been initialized if this isn't the initialization + // request. + if req.Method() != protocol.MethodInitialize && l.initParams.Load() == nil { + return replier(ctx, nil, fmt.Errorf("the first call to the server must be the %q method", protocol.MethodInitialize)) } - return retErr + + return actual(ctx, replier, req) } } diff --git a/private/buf/buflsp/mutex.go b/private/buf/buflsp/mutex.go index 4d1f2761c7..c3314b0b32 100644 --- a/private/buf/buflsp/mutex.go +++ b/private/buf/buflsp/mutex.go @@ -115,10 +115,6 @@ func (mu *mutex) Lock(ctx context.Context) (unlocker func()) { panic("buflsp/mutex.go: non-reentrant lock locked twice by the same request") } - // Block until we acquire the lock. - mu.lock.Lock() - mu.storeWho(id) - if mu.pool != nil { mu.pool.lock.Lock() defer mu.pool.lock.Unlock() @@ -131,6 +127,10 @@ func (mu *mutex) Lock(ctx context.Context) (unlocker func()) { mu.pool.held[id] = mu } + // Ok, we're definitely not holding a lock, so we can block until we acquire the lock. + mu.lock.Lock() + mu.storeWho(id) + return unlocker }