internal/lsp/regtest: allow sharing memoized results across regtests
Each regtest does a significant amount of extra work re-doing things
like parsing and type-checking the runtime package. We can share this
work across regtests by using a shared cache, significantly speeding
them up at the cost of potentially hiding bugs related to timing.
Sharing this work still retains most of the benefit of the regtests, so
implement this in the default mode (formerly called "singleton" and now
renamed to "default"). In a subsequent CL, modes will be cleaned up so
that "default" is the only mode that runs with -short.
Making this change actually revealed a caching bug: our cached package
stores error messages extracted from go/packages errors, but does not
include these errors in the cache key. Fix this by hashing all metadata
errors into the package cache key.
Updates golang/go#39384
Change-Id: I37ab9604149d34c9a79fc02b0e1bc23fcb17c454
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417587
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
diff --git a/gopls/internal/regtest/bench/bench_test.go b/gopls/internal/regtest/bench/bench_test.go
index 7f0da83..090a51c 100644
--- a/gopls/internal/regtest/bench/bench_test.go
+++ b/gopls/internal/regtest/bench/bench_test.go
@@ -33,7 +33,7 @@
// Skip logs as they buffer up memory unnaturally.
SkipLogs(),
// The Debug server only makes sense if running in singleton mode.
- Modes(Singleton),
+ Modes(Default),
// Remove the default timeout. Individual tests should control their
// own graceful termination.
NoDefaultTimeout(),
diff --git a/gopls/internal/regtest/debug/debug_test.go b/gopls/internal/regtest/debug/debug_test.go
index d01d44e..bae1480 100644
--- a/gopls/internal/regtest/debug/debug_test.go
+++ b/gopls/internal/regtest/debug/debug_test.go
@@ -20,7 +20,7 @@
// Verify that a properly configured session gets notified of a bug on the
// server.
WithOptions(
- Modes(Singleton), // must be in-process to receive the bug report below
+ Modes(Default), // must be in-process to receive the bug report below
Settings{"showBugReports": true},
).Run(t, "", func(t *testing.T, env *Env) {
const desc = "got a bug"
diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go
index 9c9ad36..ac5307e 100644
--- a/gopls/internal/regtest/diagnostics/diagnostics_test.go
+++ b/gopls/internal/regtest/diagnostics/diagnostics_test.go
@@ -298,7 +298,7 @@
t.Run("without workspace module", func(t *testing.T) {
WithOptions(
- Modes(Singleton),
+ Modes(Default),
).Run(t, noMod, func(t *testing.T, env *Env) {
env.Await(
env.DiagnosticAtRegexp("main.go", `"mod.com/bob"`),
@@ -1678,7 +1678,7 @@
WithOptions(
InGOPATH(),
EnvVars{"GO111MODULE": "off"},
- Modes(Singleton),
+ Modes(Default),
).Run(t, mod, func(t *testing.T, env *Env) {
env.Await(
env.DiagnosticAtRegexpWithMessage("main.go", `"nosuchpkg"`, `cannot find package "nosuchpkg" in any of`),
@@ -1705,7 +1705,7 @@
for _, go111module := range []string{"on", "auto"} {
t.Run("GO111MODULE="+go111module, func(t *testing.T) {
WithOptions(
- Modes(Singleton),
+ Modes(Default),
EnvVars{"GO111MODULE": go111module},
).Run(t, modules, func(t *testing.T, env *Env) {
env.OpenFile("a/a.go")
@@ -1722,7 +1722,7 @@
// Expect no warning if GO111MODULE=auto in a directory in GOPATH.
t.Run("GOPATH_GO111MODULE_auto", func(t *testing.T) {
WithOptions(
- Modes(Singleton),
+ Modes(Default),
EnvVars{"GO111MODULE": "auto"},
InGOPATH(),
).Run(t, modules, func(t *testing.T, env *Env) {
@@ -1784,7 +1784,7 @@
`
WithOptions(
ProxyFiles(proxy),
- Modes(Singleton),
+ Modes(Default),
).Run(t, nested, func(t *testing.T, env *Env) {
// Expect a diagnostic in a nested module.
env.OpenFile("nested/hello/hello.go")
@@ -1996,7 +1996,7 @@
`
WithOptions(
Settings{"experimentalUseInvalidMetadata": true},
- Modes(Singleton),
+ Modes(Default),
).Run(t, mod, func(t *testing.T, env *Env) {
env.OpenFile("go.mod")
env.RegexpReplace("go.mod", "module mod.com", "modul mod.com") // break the go.mod file
@@ -2052,7 +2052,7 @@
Settings{"experimentalUseInvalidMetadata": true},
// ExperimentalWorkspaceModule has a different failure mode for this
// case.
- Modes(Singleton),
+ Modes(Default),
).Run(t, mod, func(t *testing.T, env *Env) {
env.Await(
OnceMet(
diff --git a/gopls/internal/regtest/misc/semantictokens_test.go b/gopls/internal/regtest/misc/semantictokens_test.go
index dca2b8e..4437d40 100644
--- a/gopls/internal/regtest/misc/semantictokens_test.go
+++ b/gopls/internal/regtest/misc/semantictokens_test.go
@@ -25,7 +25,7 @@
`
WithOptions(
- Modes(Singleton),
+ Modes(Default),
Settings{"allExperiments": true},
).Run(t, src, func(t *testing.T, env *Env) {
params := &protocol.SemanticTokensParams{}
diff --git a/gopls/internal/regtest/misc/vendor_test.go b/gopls/internal/regtest/misc/vendor_test.go
index 4e02799..b0f507a 100644
--- a/gopls/internal/regtest/misc/vendor_test.go
+++ b/gopls/internal/regtest/misc/vendor_test.go
@@ -49,7 +49,7 @@
}
`
WithOptions(
- Modes(Singleton),
+ Modes(Default),
ProxyFiles(basicProxy),
).Run(t, pkgThatUsesVendoring, func(t *testing.T, env *Env) {
env.OpenFile("a/a1.go")
diff --git a/gopls/internal/regtest/modfile/modfile_test.go b/gopls/internal/regtest/modfile/modfile_test.go
index c0bef83..e6f76d4d 100644
--- a/gopls/internal/regtest/modfile/modfile_test.go
+++ b/gopls/internal/regtest/modfile/modfile_test.go
@@ -742,7 +742,7 @@
WithOptions(
EnvVars{"GOFLAGS": "-mod=readonly"},
ProxyFiles(proxy),
- Modes(Singleton),
+ Modes(Default),
).Run(t, mod, func(t *testing.T, env *Env) {
env.OpenFile("main.go")
original := env.ReadWorkspaceFile("go.mod")
@@ -922,7 +922,7 @@
// TODO(rFindley) this doesn't work in multi-module workspace mode, because
// it keeps around the last parsing modfile. Update this test to also
// exercise the workspace module.
- Modes(Singleton),
+ Modes(Default),
).Run(t, mod, func(t *testing.T, env *Env) {
env.OpenFile("go.mod")
env.Await(env.DoneWithOpen())
@@ -1090,7 +1090,7 @@
`
WithOptions(
ProxyFiles(workspaceProxy),
- Modes(Singleton),
+ Modes(Default),
).Run(t, mod, func(t *testing.T, env *Env) {
env.OpenFile("go.mod")
params := &protocol.PublishDiagnosticsParams{}
@@ -1159,7 +1159,7 @@
`
WithOptions(
ProxyFiles(proxy),
- Modes(Singleton),
+ Modes(Default),
).Run(t, mod, func(t *testing.T, env *Env) {
env.OpenFile("main.go")
d := &protocol.PublishDiagnosticsParams{}
diff --git a/gopls/internal/regtest/workspace/workspace_test.go b/gopls/internal/regtest/workspace/workspace_test.go
index 7eafaf1..e4a1c4b 100644
--- a/gopls/internal/regtest/workspace/workspace_test.go
+++ b/gopls/internal/regtest/workspace/workspace_test.go
@@ -1205,7 +1205,7 @@
`
WithOptions(
EnvVars{"GOPATH": filepath.FromSlash("$SANDBOX_WORKDIR/gopath")},
- Modes(Singleton),
+ Modes(Default),
).Run(t, mod, func(t *testing.T, env *Env) {
env.Await(
// Confirm that the build configuration is seen as valid,
@@ -1236,7 +1236,7 @@
func main() {}
`
WithOptions(
- Modes(Singleton),
+ Modes(Default),
).Run(t, nomod, func(t *testing.T, env *Env) {
env.OpenFile("a/main.go")
env.OpenFile("b/main.go")
diff --git a/gopls/internal/vulncheck/command_test.go b/gopls/internal/vulncheck/command_test.go
index e7bf708..71eaf4a 100644
--- a/gopls/internal/vulncheck/command_test.go
+++ b/gopls/internal/vulncheck/command_test.go
@@ -293,7 +293,7 @@
t.Fatal(err)
}
- cache := cache.New(nil)
+ cache := cache.New(nil, nil, nil)
session := cache.NewSession(ctx)
options := source.DefaultOptions().Clone()
tests.DefaultOptions(options)
diff --git a/internal/lsp/cache/cache.go b/internal/lsp/cache/cache.go
index a59c890..c002850 100644
--- a/internal/lsp/cache/cache.go
+++ b/internal/lsp/cache/cache.go
@@ -28,23 +28,46 @@
"golang.org/x/tools/internal/span"
)
-func New(options func(*source.Options)) *Cache {
+// New Creates a new cache for gopls operation results, using the given file
+// set, shared store, and session options.
+//
+// All of the fset, store and options may be nil, but if store is non-nil so
+// must be fset (and they must always be used together), otherwise it may be
+// possible to get cached data referencing token.Pos values not mapped by the
+// FileSet.
+func New(fset *token.FileSet, store *memoize.Store, options func(*source.Options)) *Cache {
index := atomic.AddInt64(&cacheIndex, 1)
+
+ if store != nil && fset == nil {
+ panic("non-nil store with nil fset")
+ }
+ if fset == nil {
+ fset = token.NewFileSet()
+ }
+ if store == nil {
+ store = &memoize.Store{}
+ }
+
c := &Cache{
id: strconv.FormatInt(index, 10),
- fset: token.NewFileSet(),
+ fset: fset,
options: options,
+ store: store,
fileContent: map[span.URI]*fileHandle{},
}
return c
}
type Cache struct {
- id string
- fset *token.FileSet
+ id string
+ fset *token.FileSet
+
+ // TODO(rfindley): it doesn't make sense that cache accepts LSP options, just
+ // so that it can create a session: the cache does not (and should not)
+ // depend on options. Invert this relationship to remove options from Cache.
options func(*source.Options)
- store memoize.Store
+ store *memoize.Store
fileMu sync.Mutex
fileContent map[span.URI]*fileHandle
diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go
index 4caf4ba..6c02d53 100644
--- a/internal/lsp/cache/check.go
+++ b/internal/lsp/cache/check.go
@@ -249,6 +249,16 @@
for _, file := range files {
b.WriteString(file.FileIdentity().String())
}
+ // Metadata errors are interpreted and memoized on the computed package, so
+ // we must hash them into the key here.
+ //
+ // TODO(rfindley): handle metadata diagnostics independently from
+ // type-checking diagnostics.
+ for _, err := range m.Errors {
+ b.WriteString(err.Msg)
+ b.WriteString(err.Pos)
+ b.WriteRune(rune(err.Kind))
+ }
return packageHandleKey(source.HashOf(b.Bytes()))
}
diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go
index 2374a52..a27efe3 100644
--- a/internal/lsp/cache/session.go
+++ b/internal/lsp/cache/session.go
@@ -231,7 +231,7 @@
backgroundCtx: backgroundCtx,
cancel: cancel,
initializeOnce: &sync.Once{},
- store: &s.cache.store,
+ store: s.cache.store,
packages: persistent.NewMap(packageKeyLessInterface),
meta: &metadataGraph{},
files: newFilesMap(),
diff --git a/internal/lsp/cmd/capabilities_test.go b/internal/lsp/cmd/capabilities_test.go
index 1d01b4b..930621b 100644
--- a/internal/lsp/cmd/capabilities_test.go
+++ b/internal/lsp/cmd/capabilities_test.go
@@ -43,7 +43,7 @@
params.Capabilities.Workspace.Configuration = true
// Send an initialize request to the server.
- c.Server = lsp.NewServer(cache.New(app.options).NewSession(ctx), c.Client)
+ c.Server = lsp.NewServer(cache.New(nil, nil, app.options).NewSession(ctx), c.Client)
result, err := c.Server.Initialize(ctx, params)
if err != nil {
t.Fatal(err)
diff --git a/internal/lsp/cmd/cmd.go b/internal/lsp/cmd/cmd.go
index a81eb83..5911f97 100644
--- a/internal/lsp/cmd/cmd.go
+++ b/internal/lsp/cmd/cmd.go
@@ -286,7 +286,7 @@
switch {
case app.Remote == "":
connection := newConnection(app)
- connection.Server = lsp.NewServer(cache.New(app.options).NewSession(ctx), connection.Client)
+ connection.Server = lsp.NewServer(cache.New(nil, nil, app.options).NewSession(ctx), connection.Client)
ctx = protocol.WithClient(ctx, connection.Client)
return connection, connection.initialize(ctx, app.options)
case strings.HasPrefix(app.Remote, "internal@"):
diff --git a/internal/lsp/cmd/serve.go b/internal/lsp/cmd/serve.go
index 1c229a4..10730fd 100644
--- a/internal/lsp/cmd/serve.go
+++ b/internal/lsp/cmd/serve.go
@@ -101,7 +101,7 @@
return fmt.Errorf("creating forwarder: %w", err)
}
} else {
- ss = lsprpc.NewStreamServer(cache.New(s.app.options), isDaemon)
+ ss = lsprpc.NewStreamServer(cache.New(nil, nil, s.app.options), isDaemon)
}
var network, addr string
diff --git a/internal/lsp/cmd/test/cmdtest.go b/internal/lsp/cmd/test/cmdtest.go
index ff0461b..5342e9b 100644
--- a/internal/lsp/cmd/test/cmdtest.go
+++ b/internal/lsp/cmd/test/cmdtest.go
@@ -50,7 +50,7 @@
func NewTestServer(ctx context.Context, options func(*source.Options)) *servertest.TCPServer {
ctx = debug.WithInstance(ctx, "", "")
- cache := cache.New(options)
+ cache := cache.New(nil, nil, options)
ss := lsprpc.NewStreamServer(cache, false)
return servertest.NewTCPServer(ctx, ss, nil)
}
diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go
index 8e0e628..53890dc 100644
--- a/internal/lsp/lsp_test.go
+++ b/internal/lsp/lsp_test.go
@@ -49,7 +49,7 @@
func testLSP(t *testing.T, datum *tests.Data) {
ctx := tests.Context(t)
- cache := cache.New(nil)
+ cache := cache.New(nil, nil, nil)
session := cache.NewSession(ctx)
options := source.DefaultOptions().Clone()
tests.DefaultOptions(options)
diff --git a/internal/lsp/lsprpc/lsprpc_test.go b/internal/lsp/lsprpc/lsprpc_test.go
index cde641c..498566d 100644
--- a/internal/lsp/lsprpc/lsprpc_test.go
+++ b/internal/lsp/lsprpc/lsprpc_test.go
@@ -58,7 +58,7 @@
client := FakeClient{Logs: make(chan string, 10)}
ctx = debug.WithInstance(ctx, "", "")
- ss := NewStreamServer(cache.New(nil), false)
+ ss := NewStreamServer(cache.New(nil, nil, nil), false)
ss.serverForTest = server
ts := servertest.NewPipeServer(ss, nil)
defer checkClose(t, ts.Close)
@@ -121,7 +121,7 @@
func setupForwarding(ctx context.Context, t *testing.T, s protocol.Server) (direct, forwarded servertest.Connector, cleanup func()) {
t.Helper()
serveCtx := debug.WithInstance(ctx, "", "")
- ss := NewStreamServer(cache.New(nil), false)
+ ss := NewStreamServer(cache.New(nil, nil, nil), false)
ss.serverForTest = s
tsDirect := servertest.NewTCPServer(serveCtx, ss, nil)
@@ -216,7 +216,7 @@
clientCtx := debug.WithInstance(baseCtx, "", "")
serverCtx := debug.WithInstance(baseCtx, "", "")
- cache := cache.New(nil)
+ cache := cache.New(nil, nil, nil)
ss := NewStreamServer(cache, false)
tsBackend := servertest.NewTCPServer(serverCtx, ss, nil)
diff --git a/internal/lsp/mod/mod_test.go b/internal/lsp/mod/mod_test.go
index 09a182d..56af986 100644
--- a/internal/lsp/mod/mod_test.go
+++ b/internal/lsp/mod/mod_test.go
@@ -26,7 +26,7 @@
testenv.NeedsGo1Point(t, 14)
ctx := tests.Context(t)
- cache := cache.New(nil)
+ cache := cache.New(nil, nil, nil)
session := cache.NewSession(ctx)
options := source.DefaultOptions().Clone()
tests.DefaultOptions(options)
diff --git a/internal/lsp/regtest/regtest.go b/internal/lsp/regtest/regtest.go
index 9ebc673..b3a543d 100644
--- a/internal/lsp/regtest/regtest.go
+++ b/internal/lsp/regtest/regtest.go
@@ -8,6 +8,7 @@
"context"
"flag"
"fmt"
+ "go/token"
"io/ioutil"
"os"
"runtime"
@@ -16,6 +17,7 @@
"golang.org/x/tools/internal/lsp/cmd"
"golang.org/x/tools/internal/lsp/source"
+ "golang.org/x/tools/internal/memoize"
"golang.org/x/tools/internal/testenv"
"golang.org/x/tools/internal/tool"
)
@@ -87,9 +89,11 @@
}
func DefaultModes() Mode {
- normal := Singleton | Experimental
+ // TODO(rfindley): these modes should *not* depend on GOOS. Depending on
+ // testing.Short() should be sufficient.
+ normal := Default | Experimental
if slowGOOS[runtime.GOOS] && testing.Short() {
- normal = Singleton
+ normal = Default
}
if *runSubprocessTests {
return normal | SeparateProcess
@@ -116,6 +120,8 @@
PrintGoroutinesOnFailure: *printGoroutinesOnFailure,
SkipCleanup: *skipCleanup,
OptionsHook: hook,
+ fset: token.NewFileSet(),
+ store: memoize.NewStore(memoize.NeverEvict),
}
if *runSubprocessTests {
goplsPath := *goplsBinaryPath
@@ -126,13 +132,13 @@
panic(fmt.Sprintf("finding test binary path: %v", err))
}
}
- runner.GoplsPath = goplsPath
+ runner.goplsPath = goplsPath
}
dir, err := ioutil.TempDir("", "gopls-regtest-")
if err != nil {
panic(fmt.Errorf("creating regtest temp directory: %v", err))
}
- runner.TempDir = dir
+ runner.tempDir = dir
code := m.Run()
if err := runner.Close(); err != nil {
diff --git a/internal/lsp/regtest/runner.go b/internal/lsp/regtest/runner.go
index b2992e9..6c96e61 100644
--- a/internal/lsp/regtest/runner.go
+++ b/internal/lsp/regtest/runner.go
@@ -8,6 +8,7 @@
"bytes"
"context"
"fmt"
+ "go/token"
"io"
"io/ioutil"
"net"
@@ -29,24 +30,58 @@
"golang.org/x/tools/internal/lsp/lsprpc"
"golang.org/x/tools/internal/lsp/protocol"
"golang.org/x/tools/internal/lsp/source"
+ "golang.org/x/tools/internal/memoize"
"golang.org/x/tools/internal/testenv"
"golang.org/x/tools/internal/xcontext"
)
// Mode is a bitmask that defines for which execution modes a test should run.
+//
+// Each mode controls several aspects of gopls' configuration:
+// - Which server options to use for gopls sessions
+// - Whether to use a shared cache
+// - Whether to use a shared server
+// - Whether to run the server in-process or in a separate process
+//
+// The behavior of each mode with respect to these aspects is summarized below.
+// TODO(rfindley, cleanup): rather than using arbitrary names for these modes,
+// we can compose them explicitly out of the features described here, allowing
+// individual tests more freedom in constructing problematic execution modes.
+// For example, a test could assert on a certain behavior when running with
+// experimental options on a separate process. Moreover, we could unify 'Modes'
+// with 'Options', and use RunMultiple rather than a hard-coded loop through
+// modes.
+//
+// Mode | Options | Shared Cache? | Shared Server? | In-process?
+// ---------------------------------------------------------------------------
+// Default | Default | Y | N | Y
+// Forwarded | Default | Y | Y | Y
+// SeparateProcess | Default | Y | Y | N
+// Experimental | Experimental | N | N | Y
type Mode int
const (
- // Singleton mode uses a separate in-process gopls instance for each test,
- // and communicates over pipes to mimic the gopls sidecar execution mode,
- // which communicates over stdin/stderr.
- Singleton Mode = 1 << iota
- // Forwarded forwards connections to a shared in-process gopls instance.
+ // Default mode runs gopls with the default options, communicating over pipes
+ // to emulate the lsp sidecar execution mode, which communicates over
+ // stdin/stdout.
+ //
+ // It uses separate servers for each test, but a shared cache, to avoid
+ // duplicating work when processing GOROOT.
+ Default Mode = 1 << iota
+
+ // Forwarded uses the default options, but forwards connections to a shared
+ // in-process gopls server.
Forwarded
- // SeparateProcess forwards connection to a shared separate gopls process.
+
+ // SeparateProcess uses the default options, but forwards connection to an
+ // external gopls daemon.
SeparateProcess
+
// Experimental enables all of the experimental configurations that are
- // being developed.
+ // being developed, and runs gopls in sidecar mode.
+ //
+ // It uses a separate cache for each test, to exercise races that may only
+ // appear with cache misses.
Experimental
)
@@ -55,14 +90,20 @@
// remote), any tests that execute on the same Runner will share the same
// state.
type Runner struct {
- DefaultModes Mode
- Timeout time.Duration
- GoplsPath string
- PrintGoroutinesOnFailure bool
- TempDir string
- SkipCleanup bool
- OptionsHook func(*source.Options)
+ // Configuration
+ DefaultModes Mode // modes to run for each test
+ Timeout time.Duration // per-test timeout, if set
+ PrintGoroutinesOnFailure bool // whether to dump goroutines on test failure
+ SkipCleanup bool // if set, don't delete test data directories when the test exits
+ OptionsHook func(*source.Options) // if set, use these options when creating gopls sessions
+ // Immutable state shared across test invocations
+ goplsPath string // path to the gopls executable (for SeparateProcess mode)
+ tempDir string // shared parent temp directory
+ fset *token.FileSet // shared FileSet
+ store *memoize.Store // shared store
+
+ // Lazily allocated resources
mu sync.Mutex
ts *servertest.TCPServer
socketDir string
@@ -193,7 +234,7 @@
}
// DebugAddress configures a debug server bound to addr. This option is
-// currently only supported when executing in Singleton mode. It is intended to
+// currently only supported when executing in Default mode. It is intended to
// be used for long-running stress tests.
func DebugAddress(addr string) RunOption {
return optionSetter(func(opts *runConfig) {
@@ -252,10 +293,10 @@
mode Mode
getServer func(*testing.T, func(*source.Options)) jsonrpc2.StreamServer
}{
- {"singleton", Singleton, singletonServer},
+ {"default", Default, r.defaultServer},
{"forwarded", Forwarded, r.forwardedServer},
{"separate_process", SeparateProcess, r.separateProcessServer},
- {"experimental", Experimental, experimentalServer},
+ {"experimental", Experimental, r.experimentalServer},
}
for _, tc := range tests {
@@ -267,10 +308,10 @@
if config.modes&tc.mode == 0 {
continue
}
- if config.debugAddr != "" && tc.mode != Singleton {
+ if config.debugAddr != "" && tc.mode != Default {
// Debugging is useful for running stress tests, but since the daemon has
// likely already been started, it would be too late to debug.
- t.Fatalf("debugging regtest servers only works in Singleton mode, "+
+ t.Fatalf("debugging regtest servers only works in Default mode, "+
"got debug addr %q and mode %v", config.debugAddr, tc.mode)
}
@@ -298,7 +339,7 @@
di.MonitorMemory(ctx)
}
- rootDir := filepath.Join(r.TempDir, filepath.FromSlash(t.Name()))
+ rootDir := filepath.Join(r.tempDir, filepath.FromSlash(t.Name()))
if err := os.MkdirAll(rootDir, 0755); err != nil {
t.Fatal(err)
}
@@ -434,11 +475,13 @@
fmt.Fprintf(os.Stderr, "#### End Gopls Test Logs for %q\n", testname)
}
-func singletonServer(t *testing.T, optsHook func(*source.Options)) jsonrpc2.StreamServer {
- return lsprpc.NewStreamServer(cache.New(optsHook), false)
+// defaultServer handles the Default execution mode.
+func (r *Runner) defaultServer(t *testing.T, optsHook func(*source.Options)) jsonrpc2.StreamServer {
+ return lsprpc.NewStreamServer(cache.New(r.fset, r.store, optsHook), false)
}
-func experimentalServer(t *testing.T, optsHook func(*source.Options)) jsonrpc2.StreamServer {
+// experimentalServer handles the Experimental execution mode.
+func (r *Runner) experimentalServer(t *testing.T, optsHook func(*source.Options)) jsonrpc2.StreamServer {
options := func(o *source.Options) {
optsHook(o)
o.EnableAllExperiments()
@@ -446,28 +489,23 @@
// source.Options.EnableAllExperiments, but we want to test it.
o.ExperimentalWorkspaceModule = true
}
- return lsprpc.NewStreamServer(cache.New(options), false)
+ return lsprpc.NewStreamServer(cache.New(nil, nil, options), false)
}
-func (r *Runner) forwardedServer(t *testing.T, optsHook func(*source.Options)) jsonrpc2.StreamServer {
- ts := r.getTestServer(optsHook)
- return newForwarder("tcp", ts.Addr)
-}
-
-// getTestServer gets the shared test server instance to connect to, or creates
-// one if it doesn't exist.
-func (r *Runner) getTestServer(optsHook func(*source.Options)) *servertest.TCPServer {
- r.mu.Lock()
- defer r.mu.Unlock()
+// forwardedServer handles the Forwarded execution mode.
+func (r *Runner) forwardedServer(_ *testing.T, optsHook func(*source.Options)) jsonrpc2.StreamServer {
if r.ts == nil {
+ r.mu.Lock()
ctx := context.Background()
ctx = debug.WithInstance(ctx, "", "off")
- ss := lsprpc.NewStreamServer(cache.New(optsHook), false)
+ ss := lsprpc.NewStreamServer(cache.New(nil, nil, optsHook), false)
r.ts = servertest.NewTCPServer(ctx, ss, nil)
+ r.mu.Unlock()
}
- return r.ts
+ return newForwarder("tcp", r.ts.Addr)
}
+// separateProcessServer handles the SeparateProcess execution mode.
func (r *Runner) separateProcessServer(t *testing.T, optsHook func(*source.Options)) jsonrpc2.StreamServer {
// TODO(rfindley): can we use the autostart behavior here, instead of
// pre-starting the remote?
@@ -498,21 +536,22 @@
return filepath.Join(r.socketDir, daemonFile)
}
- if r.GoplsPath == "" {
+ if r.goplsPath == "" {
t.Fatal("cannot run tests with a separate process unless a path to a gopls binary is configured")
}
var err error
- r.socketDir, err = ioutil.TempDir(r.TempDir, "gopls-regtest-socket")
+ r.socketDir, err = ioutil.TempDir(r.tempDir, "gopls-regtest-socket")
if err != nil {
t.Fatalf("creating tempdir: %v", err)
}
socket := filepath.Join(r.socketDir, daemonFile)
args := []string{"serve", "-listen", "unix;" + socket, "-listen.timeout", "10s"}
- cmd := exec.Command(r.GoplsPath, args...)
+ cmd := exec.Command(r.goplsPath, args...)
cmd.Env = append(os.Environ(), runTestAsGoplsEnvvar+"=true")
var stderr bytes.Buffer
cmd.Stderr = &stderr
go func() {
+ // TODO(rfindley): this is racy; we're returning before we know that the command is running.
if err := cmd.Run(); err != nil {
panic(fmt.Sprintf("error running external gopls: %v\nstderr:\n%s", err, stderr.String()))
}
@@ -537,7 +576,7 @@
}
}
if !r.SkipCleanup {
- if err := os.RemoveAll(r.TempDir); err != nil {
+ if err := os.RemoveAll(r.tempDir); err != nil {
errmsgs = append(errmsgs, err.Error())
}
}
diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go
index c670bde..5fdcc0f 100644
--- a/internal/lsp/source/source_test.go
+++ b/internal/lsp/source/source_test.go
@@ -49,7 +49,7 @@
func testSource(t *testing.T, datum *tests.Data) {
ctx := tests.Context(t)
- cache := cache.New(nil)
+ cache := cache.New(nil, nil, nil)
session := cache.NewSession(ctx)
options := source.DefaultOptions().Clone()
tests.DefaultOptions(options)
diff --git a/internal/memoize/memoize.go b/internal/memoize/memoize.go
index aa4d58d..150c79a 100644
--- a/internal/memoize/memoize.go
+++ b/internal/memoize/memoize.go
@@ -236,12 +236,34 @@
}
}
+// An EvictionPolicy controls the eviction behavior of keys in a Store when
+// they no longer have any references.
+type EvictionPolicy int
+
+const (
+ // ImmediatelyEvict evicts keys as soon as they no longer have references.
+ ImmediatelyEvict EvictionPolicy = iota
+
+ // NeverEvict does not evict keys.
+ NeverEvict
+)
+
// A Store maps arbitrary keys to reference-counted promises.
+//
+// The zero value is a valid Store, though a store may also be created via
+// NewStore if a custom EvictionPolicy is required.
type Store struct {
+ evictionPolicy EvictionPolicy
+
promisesMu sync.Mutex
promises map[interface{}]*Promise
}
+// NewStore creates a new store with the given eviction policy.
+func NewStore(policy EvictionPolicy) *Store {
+ return &Store{evictionPolicy: policy}
+}
+
// Promise returns a reference-counted promise for the future result of
// calling the specified function.
//
@@ -264,7 +286,9 @@
store.promisesMu.Unlock()
release := func() {
- if atomic.AddInt32(&p.refcount, -1) == 0 {
+ // TODO(rfindley): this looks racy: it's possible that the refcount is
+ // incremented before we grab the lock.
+ if atomic.AddInt32(&p.refcount, -1) == 0 && store.evictionPolicy != NeverEvict {
store.promisesMu.Lock()
delete(store.promises, key)
store.promisesMu.Unlock()
