Skip to content

Commit

Permalink
gopls/internal/test: synchronize notifications during commands
Browse files Browse the repository at this point in the history
It turns out that in the jsonrcp2 package, call responses are
asynchronous to other notifications. Therefore, we must synchronize
tests using progress notifications.

Introduce a DelayMessages test option to reproduce these types of races.
(It worked for reproducing golang/go#70342.)

Fixes golang/go#70342

Change-Id: I4cfcd7675335694a47eaf1a2547be0301fc244c9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/627696
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
  • Loading branch information
findleyr committed Nov 18, 2024
1 parent 254baba commit ed19fc7
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 6 deletions.
9 changes: 9 additions & 0 deletions gopls/internal/server/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,15 @@ func (s *server) ExecuteCommand(ctx context.Context, params *protocol.ExecuteCom
ctx, done := event.Start(ctx, "lsp.Server.executeCommand")
defer done()

// For test synchronization, always create a progress notification.
//
// This may be in addition to user-facing progress notifications created in
// the course of command execution.
if s.Options().VerboseWorkDoneProgress {
work := s.progress.Start(ctx, params.Command, "Verbose: running command...", nil, nil)
defer work.End(ctx, "Done.")
}

var found bool
for _, name := range s.Options().SupportedCommands {
if name == params.Command {
Expand Down
27 changes: 23 additions & 4 deletions gopls/internal/test/integration/fake/editor.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,15 @@ import (
"encoding/json"
"errors"
"fmt"
"math/rand/v2"
"os"
"path"
"path/filepath"
"regexp"
"slices"
"strings"
"sync"
"time"

"golang.org/x/tools/gopls/internal/protocol"
"golang.org/x/tools/gopls/internal/protocol/command"
Expand Down Expand Up @@ -136,6 +138,10 @@ type EditorConfig struct {
// If non-nil, MessageResponder is used to respond to ShowMessageRequest
// messages.
MessageResponder func(params *protocol.ShowMessageRequestParams) (*protocol.MessageActionItem, error)

// MaxMessageDelay is used for fuzzing message delivery to reproduce test
// flakes.
MaxMessageDelay time.Duration
}

// NewEditor creates a new Editor.
Expand All @@ -162,10 +168,11 @@ func (e *Editor) Connect(ctx context.Context, connector servertest.Connector, ho
e.serverConn = conn
e.Server = protocol.ServerDispatcher(conn)
e.client = &Client{editor: e, hooks: hooks}
conn.Go(bgCtx,
protocol.Handlers(
protocol.ClientHandler(e.client,
jsonrpc2.MethodNotFound)))
handler := protocol.ClientHandler(e.client, jsonrpc2.MethodNotFound)
if e.config.MaxMessageDelay > 0 {
handler = DelayedHandler(e.config.MaxMessageDelay, handler)
}
conn.Go(bgCtx, protocol.Handlers(handler))

if err := e.initialize(ctx); err != nil {
return nil, err
Expand All @@ -174,6 +181,18 @@ func (e *Editor) Connect(ctx context.Context, connector servertest.Connector, ho
return e, nil
}

// DelayedHandler waits [0, maxDelay) before handling each message.
func DelayedHandler(maxDelay time.Duration, handler jsonrpc2.Handler) jsonrpc2.Handler {
return func(ctx context.Context, reply jsonrpc2.Replier, req jsonrpc2.Request) error {
delay := time.Duration(rand.Int64N(int64(maxDelay)))
select {
case <-ctx.Done():
case <-time.After(delay):
}
return handler(ctx, reply, req)
}
}

func (e *Editor) Stats() CallCounts {
e.callsMu.Lock()
defer e.callsMu.Unlock()
Expand Down
9 changes: 7 additions & 2 deletions gopls/internal/test/integration/misc/webserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,11 @@ const A = 1
collectMessages := env.Awaiter.ListenToShownMessages()
env.ExecuteCommand(params, &result)

// golang/go#70342: just because the command has finished does not mean
// that we will have received the necessary notifications. Synchronize
// using progress reports.
env.Await(CompletedWork(params.Command, 1, false))

wantDocs, wantMessages := 0, 1
if supported {
wantDocs, wantMessages = 1, 0
Expand All @@ -144,10 +149,10 @@ const A = 1
messages := collectMessages()

if gotDocs := len(docs); gotDocs != wantDocs {
t.Errorf("GoDoc: got %d showDocument requests, want %d", gotDocs, wantDocs)
t.Errorf("gopls.doc: got %d showDocument requests, want %d", gotDocs, wantDocs)
}
if gotMessages := len(messages); gotMessages != wantMessages {
t.Errorf("GoDoc: got %d showMessage requests, want %d", gotMessages, wantMessages)
t.Errorf("gopls.doc: got %d showMessage requests, want %d", gotMessages, wantMessages)
}
})
})
Expand Down
12 changes: 12 additions & 0 deletions gopls/internal/test/integration/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package integration
import (
"strings"
"testing"
"time"

"golang.org/x/tools/gopls/internal/protocol"
"golang.org/x/tools/gopls/internal/test/integration/fake"
Expand Down Expand Up @@ -192,3 +193,14 @@ func MessageResponder(f func(*protocol.ShowMessageRequestParams) (*protocol.Mess
opts.editor.MessageResponder = f
})
}

// DelayMessages can be used to fuzz message delivery delays for the purpose of
// reproducing test flakes.
//
// (Even though this option may be unused, keep it around to aid in debugging
// future flakes.)
func DelayMessages(upto time.Duration) RunOption {
return optionSetter(func(opts *runConfig) {
opts.editor.MaxMessageDelay = upto
})
}

0 comments on commit ed19fc7

Please sign in to comment.