diff --git a/tsc/internal/project/project.go b/tsc/internal/project/project.go index 80cacf0b7b834..9fe6a5e16135f 100644 --- a/tsc/internal/project/project.go +++ b/tsc/internal/project/project.go @@ -415,20 +415,33 @@ func (p *Project) CreateProgram() CreateProgramResult { // Use pointer identity: dirtyFile is the exact instance UpdateProgram acquired, // and it is the only file whose refcount is already accounted for. if file != dirtyFile && !file.IsContentMapperFailureStub() && !file.IsContentMapperSupplemental() { - // UpdateProgram acquired the changed file only, so we need to ref everything else + // UpdateProgram acquired the changed file only, so we need to ref everything else. + // We already hold file itself, so RefOrAcquire (rather than Ref) tolerates losing + // a benign race against a concurrent, independent snapshot build that drops the + // last other claim on this cache entry between our lookup and our lock (e.g. a + // normal edit racing a speculative auto-import clone sharing this file's old + // Program, see GetLanguageServiceWithAutoImports): recreating the entry from a + // value we already possess is always correct. if file.ContentMapper() != "" { - p.host.builder.contentMappedParseCache.Ref(contentMappedParseCacheKeyForFile(file)) + p.host.builder.contentMappedParseCache.RefOrAcquire( + contentMappedParseCacheKeyForFile(file), + contentmapper.SourceFiles{Canonical: file, Supplemental: file.SupplementalSourceFiles()}, + ) } else { - p.host.builder.parseCache.Ref(parseCacheKeyForFile(file)) + p.host.builder.parseCache.RefOrAcquire(parseCacheKeyForFile(file), file) } } } for _, file := range newProgram.DuplicateSourceFiles() { if !file.IsContentMapperFailureStub { + // Duplicates are pure bookkeeping refs on an entry acquired elsewhere: we have no + // value to recreate it with, so RefIfPresent no-ops if it loses the same race + // described above. That's safe because the matching Deref issued when this + // bookkeeping owner is later released already tolerates a missing entry. if file.ContentMapper != "" { - p.host.builder.contentMappedParseCache.Ref(contentMappedParseCacheKeyForDuplicate(file)) + p.host.builder.contentMappedParseCache.RefIfPresent(contentMappedParseCacheKeyForDuplicate(file)) } else { - p.host.builder.parseCache.Ref(parseCacheKeyForDuplicate(file)) + p.host.builder.parseCache.RefIfPresent(parseCacheKeyForDuplicate(file)) } } } diff --git a/tsc/internal/project/refcountcache.go b/tsc/internal/project/refcountcache.go index 7a3c01d0f2ed5..a4072e876499e 100644 --- a/tsc/internal/project/refcountcache.go +++ b/tsc/internal/project/refcountcache.go @@ -80,23 +80,51 @@ func (c *RefCountCache[K, V, AcquireArgs]) AcquireOrError(identity K, produce fu return value, nil } -// Ref increments the reference count for an existing entry. -// Panics if the entry does not exist. -func (c *RefCountCache[K, V, AcquireArgs]) Ref(identity K) { +// RefOrAcquire increments the reference count for an existing entry, or +// installs value as a fresh entry with refCount 1 if none exists. +// +// It never panics on a missing entry. It exists for callers that already +// hold value from elsewhere (e.g. a *ast.SourceFile reused from +// an old Program while cloning a new one) and are re-establishing their own +// claim on it. Such callers can legitimately race with a concurrent Deref of +// the last other claim on the same identity: two independent snapshot builds +// (for example a normal edit and a speculative auto-import clone, see +// GetLanguageServiceWithAutoImports) can each be cloning from the same +// shared Program concurrently, and the moment the file's last other owner +// releases it can fall between this call's initial lookup and its lock +// acquisition. Since the caller already possesses a valid value for +// identity, recreating the entry is always safe: it never returns a value +// the caller didn't already have. +func (c *RefCountCache[K, V, AcquireArgs]) RefOrAcquire(identity K, value V) { + entry, loaded := c.loadOrStoreNewLockedEntry(identity) + if !loaded { + entry.value = value + } + entry.mu.Unlock() +} + +// RefIfPresent increments the reference count for an existing entry and +// reports true, or does nothing and reports false if no entry exists. +// +// It exists for callers that are recording an additional owner of an entry +// they do not themselves have a value for (e.g. a duplicate source file, +// which is only ever a bookkeeping reference to a canonical entry acquired +// elsewhere). Skipping the ref when the entry is already gone is safe: the +// corresponding Deref for this same identity, issued later when the +// bookkeeping owner is released, is itself a no-op against a missing entry. +func (c *RefCountCache[K, V, AcquireArgs]) RefIfPresent(identity K) bool { entry, ok := c.entries.Load(identity) if !ok { - panic("cache entry not found") + return false } entry.mu.Lock() defer entry.mu.Unlock() if entry.refCount <= 0 && !c.Options.DisableDeletion { - // Entry was deleted while we were acquiring the lock - newEntry, _ := c.loadOrStoreNewLockedEntry(identity) - defer newEntry.mu.Unlock() - newEntry.value = entry.value - return + // Entry was deleted while we were acquiring the lock. + return false } entry.refCount++ + return true } // Deref decrements the reference count for an entry. diff --git a/tsc/internal/project/refcountcache_test.go b/tsc/internal/project/refcountcache_test.go index 9af1a6eab689d..f6552d1cb7dd6 100644 --- a/tsc/internal/project/refcountcache_test.go +++ b/tsc/internal/project/refcountcache_test.go @@ -89,6 +89,70 @@ func TestParseCacheBindsBeforePublishing(t *testing.T) { assert.Assert(t, file.CommonJSModuleIndicator != nil) } +func TestRefOrAcquireRecreatesConcurrentlyDeletedEntry(t *testing.T) { + t.Parallel() + + cache := NewParseCache(RefCountCacheOptions{}) + key := NewParseCacheKey(ast.SourceFileParseOptions{FileName: "/a.ts", Path: "/a.ts"}, xxh3.Hash128([]byte("a")), core.ScriptKindTS) + file := &ast.SourceFile{} + + // A caller holding file (e.g. reused, by pointer, from an old Program while + // cloning a new one) can lose a benign race: some other, independent owner + // derefs the entry to zero and it's deleted from the map entirely before + // this caller gets a chance to record its own claim. Plain Ref would panic + // in that situation (see refcountcache.go); RefOrAcquire must instead + // recreate the entry from the value the caller already has. + assert.Assert(t, !cache.Has(key)) + cache.RefOrAcquire(key, file) + assert.Assert(t, cache.Has(key)) + entry, ok := cache.entries.Load(key) + assert.Assert(t, ok) + assert.Equal(t, entry.refCount, 1) + assert.Assert(t, entry.value == file) + + // A second RefOrAcquire for a live entry behaves like Ref: it bumps the + // existing entry rather than replacing its value. + other := &ast.SourceFile{} + cache.RefOrAcquire(key, other) + entry, ok = cache.entries.Load(key) + assert.Assert(t, ok) + assert.Equal(t, entry.refCount, 2) + assert.Assert(t, entry.value == file) + + cache.Deref(key) + cache.Deref(key) + assert.Assert(t, !cache.Has(key)) +} + +func TestRefIfPresentSkipsMissingEntry(t *testing.T) { + t.Parallel() + + cache := NewParseCache(RefCountCacheOptions{}) + key := NewParseCacheKey(ast.SourceFileParseOptions{FileName: "/a.ts", Path: "/a.ts"}, xxh3.Hash128([]byte("a")), core.ScriptKindTS) + + // Duplicates are bookkeeping-only refs on an entry owned elsewhere: there's + // no value on hand to recreate it with, so a missing entry must be a no-op + // (never a panic) rather than fabricating a zero-value entry. + assert.Equal(t, cache.RefIfPresent(key), false) + assert.Assert(t, !cache.Has(key)) + + file := &ast.SourceFile{} + cache.RefOrAcquire(key, file) + assert.Equal(t, cache.RefIfPresent(key), true) + entry, ok := cache.entries.Load(key) + assert.Assert(t, ok) + assert.Equal(t, entry.refCount, 2) + + cache.Deref(key) + cache.Deref(key) + assert.Assert(t, !cache.Has(key)) + + // The corresponding Deref for a duplicate whose RefIfPresent no-op'd must + // also no-op rather than panicking or corrupting an unrelated entry. + cache.Deref(key) + assert.Assert(t, !cache.Has(key)) +} + func TestRefCountingCaches(t *testing.T) { t.Parallel() diff --git a/tsc/internal/project/snapshot_stress_test.go b/tsc/internal/project/snapshot_stress_test.go new file mode 100644 index 0000000000000..1ad52f5d36680 --- /dev/null +++ b/tsc/internal/project/snapshot_stress_test.go @@ -0,0 +1,137 @@ +package project + +import ( + "context" + "fmt" + "sync" + "sync/atomic" + "testing" + + "github.com/microsoft/TypeScript/tsc/internal/bundled" + "github.com/microsoft/TypeScript/tsc/internal/lsp/lsproto" + "github.com/microsoft/TypeScript/tsc/internal/vfs/vfstest" + "gotest.tools/v3/assert" +) + +// TestSnapshotConcurrentAutoImportCloneDoesNotPanic reproduces +// https://github.com/microsoft/TypeScript/issues/63844: a "cache entry not +// found" panic in RefCountCache.Ref hit by real monorepo users of the +// language server. +// +// Two entry points can build a new Snapshot from a shared base: the normal, +// serialized edit path (getSnapshot/updateSnapshot, under snapshotUpdateMu) +// and the speculative auto-import clone used by completions needing +// auto-imports (CloneSnapshotWithAutoImports, used by +// GetLanguageServiceWithAutoImports and warmAutoImportCache), which does NOT +// go through snapshotUpdateMu. Both read and mutate the same host-level, +// ref-counted parseCache/contentMappedParseCache. When a project's Program is +// unchanged across several edits, it (and its files) stay shared across many +// snapshot generations; a concurrent auto-import clone that reuses one of +// those files via Project.CreateProgram's clone path can lose a benign race +// against a concurrent edit's disposal of an older generation, such that the +// file's cache entry is gone by the time the clone tries to Ref it. +// +// Neither concurrent edits alone nor concurrent auto-import clones alone +// (against an otherwise idle session) are sufficient to reproduce this; it +// takes both running at once, which is what this test drives. +func TestSnapshotConcurrentAutoImportCloneDoesNotPanic(t *testing.T) { + if !bundled.Embedded { + t.Skip("bundled files are not embedded") + } + + const numProjects = 6 + + files := map[string]any{} + for i := range numProjects { + files[fmt.Sprintf("/home/projects/TS/p%d/tsconfig.json", i)] = "{}" + files[fmt.Sprintf("/home/projects/TS/p%d/index.ts", i)] = "import { foo } from './foo'; export const value = foo;" + files[fmt.Sprintf("/home/projects/TS/p%d/foo.ts", i)] = "export const foo = 1;" + } + + fs := bundled.WrapFS(vfstest.FromMap(files, false /*useCaseSensitiveFileNames*/)) + session := NewSession(&SessionInit{ + BackgroundCtx: context.Background(), + Options: &SessionOptions{ + CurrentDirectory: "/", + DefaultLibraryPath: bundled.LibPath(), + TypingsLocation: "/home/src/Library/Caches/typescript", + PositionEncoding: lsproto.PositionEncodingKindUTF8, + WatchEnabled: false, + LoggingEnabled: false, + }, + FS: fs, + }) + defer session.Close() + + ctx := context.Background() + uris := make([]lsproto.DocumentUri, numProjects) + for i := range numProjects { + uri := lsproto.DocumentUri(fmt.Sprintf("file:///home/projects/TS/p%d/index.ts", i)) + uris[i] = uri + session.DidOpenFile(ctx, uri, 1, files[fmt.Sprintf("/home/projects/TS/p%d/index.ts", i)].(string), lsproto.LanguageKindTypeScript) + _, err := session.GetLanguageService(ctx, uri) + assert.NilError(t, err) + } + + var version int32 = 1 + var wg sync.WaitGroup + stop := make(chan struct{}) + + // Goroutines that keep editing files, forcing a steady stream of new + // snapshot generations (and disposal of old ones) via the normal, + // snapshotUpdateMu-serialized path. + for i := range numProjects { + wg.Add(1) + go func(i int) { + defer wg.Done() + uri := uris[i] + for { + select { + case <-stop: + return + default: + } + v := atomic.AddInt32(&version, 1) + session.DidChangeFile(ctx, uri, v, []lsproto.TextDocumentContentChangePartialOrWholeDocument{ + { + WholeDocument: &lsproto.TextDocumentContentChangeWholeDocument{ + Text: fmt.Sprintf("import { foo } from './foo'; export const value = foo; export const v = %d;", v), + }, + }, + }) + _, _ = session.GetLanguageService(ctx, uri) + } + }(i) + } + + // Goroutines that repeatedly take a speculative auto-import clone off of + // whatever the current snapshot happens to be, mimicking the + // ErrNeedsAutoImports path used by completions (ctrl+space), which does + // NOT go through snapshotUpdateMu. + for i := range numProjects { + wg.Add(1) + go func(i int) { + defer wg.Done() + uri := uris[i] + for { + select { + case <-stop: + return + default: + } + baseSnapshot := session.Snapshot() + preparedSnapshot := session.SnapshotHost.CloneSnapshotWithAutoImports(ctx, baseSnapshot, uri, nil) + session.TryAdoptSnapshotInBackground(baseSnapshot, preparedSnapshot) + preparedSnapshot.Deref() + } + }(i) + } + + // Let the race run for a bounded number of edit cycles rather than wall time. + for n := 0; n < 400; n++ { + _, _ = session.GetLanguageService(ctx, uris[n%numProjects]) + } + close(stop) + wg.Wait() + session.WaitForBackgroundTasks() +}