diff --git a/gopls/internal/regtest/bench/bench_test.go b/gopls/internal/regtest/bench/bench_test.go index 7f0da83fb37..090a51cc968 100644 --- a/gopls/internal/regtest/bench/bench_test.go +++ b/gopls/internal/regtest/bench/bench_test.go @@ -33,7 +33,7 @@ func benchmarkOptions(dir string) []RunOption { // 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 d01d44ed980..bae14802eef 100644 --- a/gopls/internal/regtest/debug/debug_test.go +++ b/gopls/internal/regtest/debug/debug_test.go @@ -20,7 +20,7 @@ func TestBugNotification(t *testing.T) { // 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 9c9ad368cf3..ac5307e5534 100644 --- a/gopls/internal/regtest/diagnostics/diagnostics_test.go +++ b/gopls/internal/regtest/diagnostics/diagnostics_test.go @@ -298,7 +298,7 @@ func Hello() { 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 @@ import ( 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 @@ package b 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 @@ package b // 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 @@ func helloHelper() {} ` 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 @@ func Hello() {} ` 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 @@ func _() {} 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 dca2b8e7514..4437d402d46 100644 --- a/gopls/internal/regtest/misc/semantictokens_test.go +++ b/gopls/internal/regtest/misc/semantictokens_test.go @@ -25,7 +25,7 @@ func main() {} ` 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 4e02799b47a..b0f507aaf32 100644 --- a/gopls/internal/regtest/misc/vendor_test.go +++ b/gopls/internal/regtest/misc/vendor_test.go @@ -49,7 +49,7 @@ func _() { } ` 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 c0bef833f44..e6f76d4d514 100644 --- a/gopls/internal/regtest/modfile/modfile_test.go +++ b/gopls/internal/regtest/modfile/modfile_test.go @@ -742,7 +742,7 @@ func main() { 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 @@ func hello() {} // 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 @@ func main() { ` 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 @@ func main() { ` 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 7eafaf191dd..e4a1c4b4494 100644 --- a/gopls/internal/regtest/workspace/workspace_test.go +++ b/gopls/internal/regtest/workspace/workspace_test.go @@ -1205,7 +1205,7 @@ package main ` 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 @@ package main 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 e7bf7085f88..71eaf4a580f 100644 --- a/gopls/internal/vulncheck/command_test.go +++ b/gopls/internal/vulncheck/command_test.go @@ -293,7 +293,7 @@ func runTest(t *testing.T, workspaceData, proxyData string, test func(context.Co 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 a59c8908d5a..c002850653b 100644 --- a/internal/lsp/cache/cache.go +++ b/internal/lsp/cache/cache.go @@ -28,23 +28,46 @@ import ( "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 4caf4ba6fa7..6c02d5348f8 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -249,6 +249,16 @@ func computePackageKey(id PackageID, files []source.FileHandle, m *KnownMetadata 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 2374a528757..a27efe334fd 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -231,7 +231,7 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI, 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 1d01b4bd0d7..930621bf307 100644 --- a/internal/lsp/cmd/capabilities_test.go +++ b/internal/lsp/cmd/capabilities_test.go @@ -43,7 +43,7 @@ func TestCapabilities(t *testing.T) { 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 a81eb839535..5911f97d1c1 100644 --- a/internal/lsp/cmd/cmd.go +++ b/internal/lsp/cmd/cmd.go @@ -286,7 +286,7 @@ func (app *Application) connect(ctx context.Context) (*connection, error) { 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 1c229a422b4..10730fd89cd 100644 --- a/internal/lsp/cmd/serve.go +++ b/internal/lsp/cmd/serve.go @@ -101,7 +101,7 @@ func (s *Serve) Run(ctx context.Context, args ...string) error { 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 ff0461b333f..5342e9b7faf 100644 --- a/internal/lsp/cmd/test/cmdtest.go +++ b/internal/lsp/cmd/test/cmdtest.go @@ -50,7 +50,7 @@ func TestCommandLine(t *testing.T, testdata string, options func(*source.Options 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 8e0e628dfea..53890dc616b 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -49,7 +49,7 @@ type runner struct { 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 cde641c920b..498566d1bbd 100644 --- a/internal/lsp/lsprpc/lsprpc_test.go +++ b/internal/lsp/lsprpc/lsprpc_test.go @@ -58,7 +58,7 @@ func TestClientLogging(t *testing.T) { 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 checkClose(t *testing.T, closer func() error) { 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 @@ func TestDebugInfoLifecycle(t *testing.T) { 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 09a182d16d7..56af9860b9d 100644 --- a/internal/lsp/mod/mod_test.go +++ b/internal/lsp/mod/mod_test.go @@ -26,7 +26,7 @@ func TestModfileRemainsUnchanged(t *testing.T) { 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 9ebc673f8c0..b3a543d531e 100644 --- a/internal/lsp/regtest/regtest.go +++ b/internal/lsp/regtest/regtest.go @@ -8,6 +8,7 @@ import ( "context" "flag" "fmt" + "go/token" "io/ioutil" "os" "runtime" @@ -16,6 +17,7 @@ import ( "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 @@ var slowGOOS = map[string]bool{ } 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 @@ func Main(m *testing.M, hook func(*source.Options)) { PrintGoroutinesOnFailure: *printGoroutinesOnFailure, SkipCleanup: *skipCleanup, OptionsHook: hook, + fset: token.NewFileSet(), + store: memoize.NewStore(memoize.NeverEvict), } if *runSubprocessTests { goplsPath := *goplsBinaryPath @@ -126,13 +132,13 @@ func Main(m *testing.M, hook func(*source.Options)) { 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 b2992e99392..6c96e61dbc6 100644 --- a/internal/lsp/regtest/runner.go +++ b/internal/lsp/regtest/runner.go @@ -8,6 +8,7 @@ import ( "bytes" "context" "fmt" + "go/token" "io" "io/ioutil" "net" @@ -29,24 +30,58 @@ import ( "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 @@ const ( // 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 @@ func InGOPATH() RunOption { } // 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 @@ func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOptio 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 @@ func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOptio 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 @@ func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOptio 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 @@ func (s *loggingFramer) printBuffers(testname string, w io.Writer) { 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 @@ func experimentalServer(t *testing.T, optsHook func(*source.Options)) jsonrpc2.S // source.Options.EnableAllExperiments, but we want to test it. o.ExperimentalWorkspaceModule = true } - return lsprpc.NewStreamServer(cache.New(options), false) -} - -func (r *Runner) forwardedServer(t *testing.T, optsHook func(*source.Options)) jsonrpc2.StreamServer { - ts := r.getTestServer(optsHook) - return newForwarder("tcp", ts.Addr) + return lsprpc.NewStreamServer(cache.New(nil, nil, options), false) } -// 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 @@ func (r *Runner) getRemoteSocket(t *testing.T) string { 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 @@ func (r *Runner) Close() error { } } 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 c670bdeb0b2..5fdcc0f86ea 100644 --- a/internal/lsp/source/source_test.go +++ b/internal/lsp/source/source_test.go @@ -49,7 +49,7 @@ type runner struct { 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 aa4d58d2f26..150c79af852 100644 --- a/internal/memoize/memoize.go +++ b/internal/memoize/memoize.go @@ -236,12 +236,34 @@ func (p *Promise) wait(ctx context.Context) (interface{}, error) { } } +// 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 @@ func (store *Store) Promise(key interface{}, function Function) (*Promise, func( 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()