diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewLocalLanguageFeatures.test.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewLocalLanguageFeatures.test.ts new file mode 100644 index 000000000..f0886b759 --- /dev/null +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewLocalLanguageFeatures.test.ts @@ -0,0 +1,191 @@ +import assert from "node:assert/strict"; +import test from "node:test"; +import { createRequire, registerHooks } from "node:module"; +import { Disposable } from "../../base/common/lifecycle.js"; +import { Position } from "../../editor/common/core/position.js"; +import { URI } from "../../base/common/uri.js"; + +const { JSDOM } = createRequire(import.meta.url)("jsdom"); +const dom = new JSDOM(""); +for (const key of ["window", "document", "HTMLElement", "HTMLCanvasElement", "Node", "MutationObserver", "Element", "navigator", "customElements", "UIEvent", "MouseEvent", "KeyboardEvent", "FocusEvent"] as const) { + Object.defineProperty(globalThis, key, { configurable: true, value: dom.window[key] }); +} +dom.window.matchMedia = () => ({ matches: false, addEventListener() { }, removeEventListener() { } }) as never; +registerHooks({ load(url, context, next) { + return url.endsWith(".css") ? { format: "module", source: "", shortCircuit: true } : next(url, context); +} }); +const { ReviewLocalLanguageFeatures } = await import("./reviewLocalLanguageFeatures.js"); + +function event() { + const listeners = new Set<(value: T) => void>(); + return { + event: (listener: (value: T) => void) => { + listeners.add(listener); + return { dispose: () => listeners.delete(listener) }; + }, + fire(value: T) { for (const listener of [...listeners]) listener(value); }, + }; +} + +function deferred() { + let resolve!: (value: T) => void; + const promise = new Promise(done => { resolve = done; }); + return { promise, resolve }; +} + +function model(attached = false) { + const changed = event(); + const disposed = event(); + let isDisposed = false; + const text = "same pinned source"; + return { + uri: URI.parse("review-api-source://review/src/file.ts?version=1"), + isAttachedToEditor: () => attached, + isDisposed: () => isDisposed, + getVersionId: () => 1, + getTextBuffer: () => text, + getEOL: () => "\n", + equalsTextBuffer: (other: string) => other === text, + onDidChangeAttached: changed.event, + onWillDispose: disposed.event, + setAttached(value: boolean) { attached = value; changed.fire(); }, + dispose() { isDisposed = true; disposed.fire(); }, + } as any; +} + +function setup(input: ReturnType | ReturnType[]) { + const sourceModels = Array.isArray(input) ? input : [input]; + const added = event(); + const modelService = { getModels: () => sourceModels, onModelAdded: added.event, createModelReference: async () => { throw new Error("unexpected model reference"); } }; + const languages = Object.fromEntries(["hoverProvider", "definitionProvider", "typeDefinitionProvider", "implementationProvider", "referenceProvider"].map(key => [key, { + register: () => Disposable.None, + ordered: () => [], + }])) as any; + const local = { + uri: URI.file("/project/src/file.ts"), + isDisposed: () => false, + getVersionId: () => 1, + getTextBuffer: () => "same pinned source", + getEOL: () => "\n", + equalsTextBuffer: (other: string) => other === "same pinned source", + }; + const service = new ReviewLocalLanguageFeatures( + { onDidChangeConnection: () => Disposable.None } as any, + { createModelReference: async (uri: URI) => ({ object: { textEditorModel: { ...local, uri } }, dispose() { } }) } as any, + modelService as any, + languages, + { activateByEvent: async () => undefined } as any, + {} as any, + { files: { models: [], resolve: async () => undefined } } as any, + { onDidFilesChange: () => Disposable.None, exists: async () => true } as any, + { debug() { } } as any, + ); + return { service, sourceModel: sourceModels[0], sourceModels, local }; +} + +function source(local: any) { + let references = 1; + let disposed = false; + const disposal = deferred(); + const release = () => { + if (!disposed && --references === 0) { disposed = true; disposal.resolve(); } + }; + const source = { + identity: "identity", + root: URI.file("/project"), + reference: { object: { textEditorModel: local }, dispose() { } }, + retain() { + if (disposed) return undefined; + references++; + let released = false; + return { dispose() { if (!released) { released = true; release(); } } }; + }, + dispose: release, + isDisposed: () => disposed, + waitDisposed: () => disposal.promise, + }; + return source; +} + +test("cancellation while acquiring a review source releases the newly acquired native model", async (t) => { + const setupResult = setup(model()); + t.after(() => setupResult.service.dispose()); + const acquisition = deferred(); + const started = deferred(); + const actualSource = source(setupResult.local); + const internal = setupResult.service as any; + internal.environment = async () => ({ rootPath: "/project", identity: "identity" }); + internal.acquire = async () => { started.resolve(); return acquisition.promise; }; + const token = { isCancellationRequested: false }; + const pending = internal.withSource(setupResult.sourceModel, new Position(1, 1), token, async () => "should not run"); + await started.promise; + token.isCancellationRequested = true; + acquisition.resolve(actualSource); + assert.equal(await pending, undefined); + assert.equal(actualSource.isDisposed(), true, "the canceled request drops both its temporary retain and detached owner"); +}); + +test("detaching during an in-flight definition keeps its captured source until mapping completes", async (t) => { + const setupResult = setup(model()); + t.after(() => setupResult.service.dispose()); + const acquisition = deferred(); + const started = deferred(); + const entered = deferred(); + const acquired = source(setupResult.local); + const internal = setupResult.service as any; + internal.environment = async () => ({ rootPath: "/project", identity: "identity" }); + internal.acquire = async () => { started.resolve(); return acquisition.promise; }; + setupResult.sourceModel.setAttached(true); + await started.promise; + const mapping = deferred(); + let capturedSource: unknown; + const request = internal.withSource(setupResult.sourceModel, new Position(1, 1), { isCancellationRequested: false }, async (_local: unknown, _at: unknown, _review: unknown, captured: unknown) => { + assert.equal(captured, acquired); + capturedSource = captured; + entered.resolve(); + return mapping.promise; + }); + acquisition.resolve(acquired); + await entered.promise; + setupResult.sourceModel.setAttached(false); + assert.equal(acquired.isDisposed(), false, "the request retain survives detachment and source-map eviction"); + const locations = await internal.reviewLocations(setupResult.sourceModel, [{ uri: URI.file("/project/src/target.ts") }], { isCancellationRequested: false }, capturedSource); + assert.equal(locations[0].uri.scheme, "review-api-source", "definition mapping uses the source retained by this request after map eviction"); + mapping.resolve("mapped result"); + assert.equal(await request, "mapped result"); + assert.equal(acquired.isDisposed(), true, "mapping completion releases the final source reference"); +}); + +test("hundreds of unattached pinned models allocate no native source models; detaching releases the visible model", async (t) => { + const setupResult = setup(Array.from({ length: 800 }, () => model())); + t.after(() => setupResult.service.dispose()); + const acquired = source(setupResult.local); + const internal = setupResult.service as any; + internal.environment = async () => ({ rootPath: "/project", identity: "identity" }); + let acquireCount = 0; + internal.acquire = async () => { acquireCount++; return acquired; }; + assert.equal(acquireCount, 0, "the pinned comparison is not eagerly resolved into 800 native working copies"); + setupResult.sourceModel.setAttached(true); + await internal.localSource(setupResult.sourceModel, true); + assert.equal(acquireCount, 1); + setupResult.sourceModel.setAttached(false); + await acquired.waitDisposed(); + assert.equal(acquired.isDisposed(), true); +}); + +test("disposing a review model or the service releases its warm native source", async (t) => { + for (const disposeOwner of ["model", "service"] as const) { + const result = setup(model()); + const acquired = source(result.local); + const internal = result.service as any; + internal.environment = async () => ({ rootPath: "/project", identity: "identity" }); + internal.acquire = async () => acquired; + result.sourceModel.setAttached(true); + await internal.localSource(result.sourceModel, true); + if (disposeOwner === "model") result.sourceModel.dispose(); + else result.service.dispose(); + await acquired.waitDisposed(); + assert.equal(acquired.isDisposed(), true, `${disposeOwner} disposal releases the working copy`); + result.service.dispose(); + } +}); diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewLocalLanguageFeatures.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewLocalLanguageFeatures.ts index 696932240..1b8f9ca09 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewLocalLanguageFeatures.ts +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewLocalLanguageFeatures.ts @@ -1,5 +1,5 @@ import type { CancellationToken } from "../../base/common/cancellation.js"; -import { Disposable, DisposableStore, type IReference } from "../../base/common/lifecycle.js"; +import { Disposable, DisposableStore, RefCountedDisposable, toDisposable, type IDisposable, type IReference } from "../../base/common/lifecycle.js"; import { URI } from "../../base/common/uri.js"; import { Position } from "../../editor/common/core/position.js"; import type { Hover, LocationLink } from "../../editor/common/languages.js"; @@ -23,11 +23,13 @@ import { IReviewDesktopConnectionService } from "./reviewDesktopConnectionServic import { withCurrentLocalContext } from "./reviewLocalRequest.js"; import { acquireReviewLanguageRoot } from "./reviewLocalWorkspace.js"; import { ReviewLanguageEnvironmentRequests } from "./reviewLanguageEnvironmentRequests.js"; +import { watchAttachedReviewModels, withRetainedSource } from "./reviewSourceModelLifecycle.js"; interface LocalSource { identity: string; root: URI; reference: IReference; + retain(): IDisposable | undefined; dispose(): void; } @@ -64,12 +66,10 @@ export class ReviewLocalLanguageFeatures extends Disposable { this._register(files.onDidFilesChange(event => { if ([...this.roots.keys()].some(root => event.affects(URI.parse(root)))) this.generation++; })); - // Warm language servers while source is being displayed, not at the first click. - const warm = (model: ITextModel) => { - if ([REVIEW_API_SOURCE_SCHEME, REVIEW_LANGUAGE_SOURCE_SCHEME].includes(model.uri.scheme)) void this.localSource(model); - }; - this._register(modelService.onModelAdded(warm)); - modelService.getModels().forEach(warm); + // Diff models include off-screen files; only attached editors warm native models. + this._register(watchAttachedReviewModels(modelService, [REVIEW_API_SOURCE_SCHEME, REVIEW_LANGUAGE_SOURCE_SCHEME], + model => { void this.localSource(model, true); }, + model => this.releaseSource(model))); for (const scheme of [REVIEW_API_SOURCE_SCHEME, REVIEW_LANGUAGE_SOURCE_SCHEME]) { const selector = { scheme, exclusive: true }; this._register(languages.hoverProvider.register(selector, { provideHover: (model, position, token) => this.hover(model, position, token) })); @@ -81,9 +81,9 @@ export class ReviewLocalLanguageFeatures extends Disposable { this._register(languages.typeDefinitionProvider.register(target, { provideTypeDefinition: (model, position, token) => this.locations(model, position, token, "type") })); this._register(languages.implementationProvider.register(target, { provideImplementation: (model, position, token) => this.locations(model, position, token, "implementation") })); this._register(languages.referenceProvider.register(target, { - provideReferences: (model, position, context, token) => this.withSource(model, position, token, async (local, at, pinned) => { + provideReferences: (model, position, context, token) => this.withSource(model, position, token, async (local, at, pinned, source) => { const results = await Promise.all(languages.referenceProvider.ordered(local).map(provider => provider.provideReferences(local, at, context, token))); - return this.reviewLocations(pinned, results.flatMap(result => result ?? []), token); + return this.reviewLocations(pinned, results.flatMap(result => result ?? []), token, source); }), })); } @@ -104,12 +104,19 @@ export class ReviewLocalLanguageFeatures extends Disposable { }, validate); } - private async localSource(model: ITextModel): Promise { - if (new URLSearchParams(model.uri.query).has("empty")) return undefined; + private releaseSource(model: ITextModel): void { + const entry = this.sources.get(model); + if (!entry) return; + this.sources.delete(model); + void entry.pending.then(source => source?.dispose()); + } + + private async localSource(model: ITextModel, warming = false): Promise { + if (this._store.isDisposed || model.isDisposed() || new URLSearchParams(model.uri.query).has("empty")) return undefined; try { const epoch = this.environments.generation; const context = await this.environment(model); - if (epoch !== this.environments.generation || model.isDisposed()) return undefined; + if (epoch !== this.environments.generation || this._store.isDisposed || model.isDisposed() || (warming && !model.isAttachedToEditor())) return undefined; const cached = this.sources.get(model); if (cached && cached.identity === context?.identity && cached.rootPath === context.rootPath) return cached.pending; if (cached) { @@ -156,49 +163,67 @@ export class ReviewLocalLanguageFeatures extends Disposable { }); const reference = owned.add(await this.models.createModelReference(resource)); owned.add(reference.object.textEditorModel.onDidChangeContent(() => this.generation++)); - owned.add(model.onWillDispose(() => { this.sources.delete(model); owned.dispose(); })); - if (model.isDisposed()) { owned.dispose(); return undefined; } + if (model.isDisposed()) { owned.dispose(); return undefined; } await this.extensions.activateByEvent(`onLanguage:${reference.object.textEditorModel.getLanguageId()}`); if (model.isDisposed()) { owned.dispose(); return undefined; } - return { root, reference, identity: context.identity, dispose: () => owned.dispose() }; + const lifetime = new RefCountedDisposable(owned); + const owner = toDisposable(() => lifetime.release()); + return { + root, reference, identity: context.identity, + retain: () => { + if (owned.isDisposed) return undefined; + lifetime.acquire(); + return toDisposable(() => lifetime.release()); + }, + dispose: () => owner.dispose(), + }; } catch (error) { owned.dispose(); throw error; } } - private async withSource(model: ITextModel, position: Position, token: CancellationToken, run: (local: ITextModel, at: Position, review: ITextModel) => Promise): Promise { + private async withSource(model: ITextModel, position: Position, token: CancellationToken, run: (local: ITextModel, at: Position, review: ITextModel, source: LocalSource | undefined) => Promise): Promise { if (token.isCancellationRequested || model.isDisposed()) return undefined; - if (model.uri.scheme === "file") return withCurrentLocalContext([model], token, () => this.generation, async () => run(model, position, model)); + if (model.uri.scheme === "file") return withCurrentLocalContext([model], token, () => this.generation, async () => run(model, position, model, undefined)); const epoch = this.environments.generation; const source = await this.localSource(model); - if (!source || token.isCancellationRequested || model.isDisposed()) return undefined; - const local = source.reference.object.textEditorModel; - if (!await this.files.exists(local.uri)) { this.uncertainRoots.add(source.root.toString()); return undefined; } - if (this.uncertainRoots.has(source.root.toString())) { - // An open dependency can outlive a removed checkout and miss subsequent - // watcher updates. Until watcher readiness is authoritative, revalidate - // clean native buffers before querying; never replace a dirty buffer. - const prefix = source.root.path.replace(/\/$/, "") + "/"; - await Promise.all(this.textFiles.files.models.filter(file => !file.isDirty() && file.resource.scheme === "file" && file.resource.path.startsWith(prefix)) - .map(file => this.textFiles.files.resolve(file.resource, { reload: { async: false } }))); + if (!source) return undefined; + // Detaching the editor can release its warm owner while a request awaits + // disk or an extension provider. Keep that request's model/root alive. + try { + return await withRetainedSource(source, async () => { + if (token.isCancellationRequested || model.isDisposed()) return undefined; + const local = source.reference.object.textEditorModel; + if (!await this.files.exists(local.uri)) { this.uncertainRoots.add(source.root.toString()); return undefined; } + if (this.uncertainRoots.has(source.root.toString())) { + // An open dependency can outlive a removed checkout and miss subsequent + // watcher updates. Until watcher readiness is authoritative, revalidate + // clean native buffers before querying; never replace a dirty buffer. + const prefix = source.root.path.replace(/\/$/, "") + "/"; + await Promise.all(this.textFiles.files.models.filter(file => !file.isDirty() && file.resource.scheme === "file" && file.resource.path.startsWith(prefix)) + .map(file => this.textFiles.files.resolve(file.resource, { reload: { async: false } }))); + } + // Resolve current disk contents, preserving any unsaved local editor buffer. + await this.textFiles.files.resolve(local.uri, { reload: { async: false } }); + return await withCurrentLocalContext([model, local], token, () => this.generation, async () => { + // The language server must see exactly the source displayed in the review. + if (!sameSource(model, local)) return undefined; + const result = await run(local, position, model, source); + const current = await this.environment(model, true).catch(() => undefined); + if (epoch === this.environments.generation && isSameRoot(current?.rootPath, source.root) && current?.identity === source.identity) return result; + // Failed validation must also release the old workspace/watchers. Keeping + // them after deletion can leave the language server blind to later edits. + const cached = this.sources.get(model); + if (cached?.identity === source.identity && isSameRoot(cached.rootPath, source.root)) { + this.uncertainRoots.add(source.root.toString()); + this.sources.delete(model); + void cached.pending.then(value => value?.dispose()); + this.generation++; + } + return undefined; + }); + }); + } finally { + if (!model.isAttachedToEditor()) this.releaseSource(model); } - // Resolve current disk contents, preserving any unsaved local editor buffer. - await this.textFiles.files.resolve(local.uri, { reload: { async: false } }); - return withCurrentLocalContext([model, local], token, () => this.generation, async () => { - // The language server must see exactly the source displayed in the review. - if (!sameSource(model, local)) return undefined; - const result = await run(local, position, model); - const current = await this.environment(model, true).catch(() => undefined); - if (epoch === this.environments.generation && isSameRoot(current?.rootPath, source.root) && current?.identity === source.identity) return result; - // Failed validation must also release the old workspace/watchers. Keeping - // them after deletion can leave the language server blind to later edits. - const cached = this.sources.get(model); - if (cached?.identity === source.identity && isSameRoot(cached.rootPath, source.root)) { - this.uncertainRoots.add(source.root.toString()); - this.sources.delete(model); - void cached.pending.then(value => value?.dispose()); - this.generation++; - } - return undefined; - }); } private hover(model: ITextModel, position: Position, token: CancellationToken): Promise { @@ -212,18 +237,17 @@ export class ReviewLocalLanguageFeatures extends Disposable { } private async locations(model: ITextModel, position: Position, token: CancellationToken, kind: "definition" | "type" | "implementation"): Promise { - return this.withSource(model, position, token, async (local, at, pinned) => { + return this.withSource(model, position, token, async (local, at, pinned, source) => { const results = kind === "definition" ? await getDefinitionsAtPosition(this.languages.definitionProvider, local, at, false, token) : kind === "type" ? await getTypeDefinitionsAtPosition(this.languages.typeDefinitionProvider, local, at, false, token) : await getImplementationsAtPosition(this.languages.implementationProvider, local, at, false, token); - return this.reviewLocations(pinned, results, token); + return this.reviewLocations(pinned, results, token, source); }); } /** Keep navigation in the same saved version/side only when destination contents match. */ - private async reviewLocations(pinned: ITextModel, locations: T[], token: CancellationToken): Promise { + private async reviewLocations(pinned: ITextModel, locations: T[], token: CancellationToken, source?: LocalSource): Promise { if (![REVIEW_API_SOURCE_SCHEME, REVIEW_LANGUAGE_SOURCE_SCHEME].includes(pinned.uri.scheme)) return locations; - const source = await this.sources.get(pinned)?.pending; if (!source) return []; const prefix = source.root.path.replace(/\/$/, "") + "/"; // References often share a file. Resolve and compare each destination once per request. diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewSourceModelLifecycle.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewSourceModelLifecycle.ts new file mode 100644 index 000000000..0f0ce081e --- /dev/null +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewSourceModelLifecycle.ts @@ -0,0 +1,47 @@ +import { DisposableStore, toDisposable, type IDisposable } from "../../base/common/lifecycle.js"; +import type { ITextModel } from "../../editor/common/model.js"; +import type { IModelService } from "../../editor/common/services/model.js"; + +/** Tracks only review models currently attached to an editor. */ +export function watchAttachedReviewModels( + models: Pick, + schemes: readonly string[], + attached: (model: ITextModel) => void, + detached: (model: ITextModel) => void, +): IDisposable { + const store = new DisposableStore(); + const active = new Set(); + store.add(toDisposable(() => { + for (const model of active) detached(model); + active.clear(); + })); + const watch = (model: ITextModel) => { + if (!schemes.includes(model.uri.scheme)) return; + const owned = store.add(new DisposableStore()); + const update = () => { + if (model.isAttachedToEditor()) { + if (!active.has(model)) { active.add(model); attached(model); } + } else if (active.delete(model)) detached(model); + }; + owned.add(model.onDidChangeAttached(update)); + owned.add(model.onWillDispose(() => { + if (active.delete(model)) detached(model); + store.delete(owned); + owned.dispose(); + })); + update(); + }; + store.add(models.onModelAdded(watch)); + models.getModels().forEach(watch); + return store; +} + +export async function withRetainedSource(source: { retain(): IDisposable | undefined }, run: () => Promise): Promise { + const reference = source.retain(); + if (!reference) return undefined; + try { + return await run(); + } finally { + reference.dispose(); + } +}