diff --git a/src/vs/sessions/contrib/layout/browser/singlePane/singlePaneDockedTabsCoordinator.ts b/src/vs/sessions/contrib/layout/browser/singlePane/singlePaneDockedTabsCoordinator.ts index 9e9f433c5ece08..1818eaa77a0491 100644 --- a/src/vs/sessions/contrib/layout/browser/singlePane/singlePaneDockedTabsCoordinator.ts +++ b/src/vs/sessions/contrib/layout/browser/singlePane/singlePaneDockedTabsCoordinator.ts @@ -207,7 +207,7 @@ export class SinglePaneDockedTabsCoordinator extends Disposable { if (!group || group.contains(e.editor)) { return; } - void this._sequencer.queue(() => this._removeFilesTab(this._editorGroupsService.mainPart.activeGroup)).catch(onUnexpectedError); + this._queue(() => this._removeFilesTab(this._editorGroupsService.mainPart.activeGroup)); })); this._register(this._editorService.onDidCloseEditor(e => { if (e.editor instanceof EmptyFileEditorInput @@ -245,7 +245,7 @@ export class SinglePaneDockedTabsCoordinator extends Disposable { } if (visible) { - void this._sequencer.queue(() => this._restoreCollapsedTabs()).catch(onUnexpectedError); + this._queue(() => this._restoreCollapsedTabs()); return; } @@ -254,7 +254,7 @@ export class SinglePaneDockedTabsCoordinator extends Disposable { return; } if (this._layoutService.isVisible(Parts.AUXILIARYBAR_PART)) { - void this._sequencer.queue(() => this._collapseNonManagedTabs()).catch(onUnexpectedError); + this._queue(() => this._collapseNonManagedTabs()); } })); @@ -294,7 +294,7 @@ export class SinglePaneDockedTabsCoordinator extends Disposable { : trigger; this._pending = { sessionKey, target, trigger: mergedTrigger }; const generation = ++this._generation; - void this._sequencer.queue(() => this._reconcile(generation)).catch(onUnexpectedError); + this._queue(() => this._reconcile(generation)); } private _readTarget(reader: IReader | undefined): IManagedTabsTarget { @@ -310,8 +310,24 @@ export class SinglePaneDockedTabsCoordinator extends Disposable { // --- Reconcile -------------------------------------------------------- + override dispose(): void { + // Bump the generation before super.dispose() so queued/in-flight reconciles bail at their next checkpoint. + this._generation++; + this._pending = undefined; + super.dispose(); + } + + /** Queues coordinator-owned work, dropping tasks and failures that outlive disposal. */ + private _queue(task: () => Promise): void { + void this._sequencer.queue(() => this._store.isDisposed ? Promise.resolve() : task()).catch(error => { + if (!this._store.isDisposed) { + onUnexpectedError(error); + } + }); + } + private async _reconcile(generation: number): Promise { - if (generation !== this._generation || !this._pending) { + if (this._store.isDisposed || generation !== this._generation || !this._pending) { return; } @@ -334,6 +350,9 @@ export class SinglePaneDockedTabsCoordinator extends Disposable { private async _reconcileCore(target: IManagedTabsTarget, trigger: IReconcileTrigger, generation: number): Promise { const group = this._editorGroupsService.mainPart.activeGroup; + let groupDisposed = false; + const groupDisposeListener = Event.once(group.onWillDispose)(() => groupDisposed = true); + const isCancelled = () => this._store.isDisposed || groupDisposed || generation !== this._generation; this._resetCollapsedEditorsOnSessionChange(); const changesResource = target.changesSessionResource ? this._sessionChangesService.getChangesEditorResource(target.changesSessionResource) : undefined; @@ -345,8 +364,8 @@ export class SinglePaneDockedTabsCoordinator extends Disposable { try { // [1] Replace an outgoing session's Changes tab in place when the incoming // session also wants Changes; close only additional stale tabs. - await this._reconcileForeignChangesEditors(group, changesResource); - if (generation !== this._generation) { + await this._reconcileForeignChangesEditors(group, changesResource, isCancelled); + if (isCancelled()) { return; } this._updateFilesEditors(group, target.workspace); @@ -354,7 +373,7 @@ export class SinglePaneDockedTabsCoordinator extends Disposable { const preserveMissingFiles = !!trigger.workingSetRestored && this._preserveMissingFilesForSessionKey === sessionKey; if (preserveMissingFiles) { await this._removeFilesTab(group); - if (generation !== this._generation) { + if (isCancelled()) { return; } } @@ -376,14 +395,14 @@ export class SinglePaneDockedTabsCoordinator extends Disposable { // [3] Keep Files active by default for a new-session view. if (openFilesFirst) { await this._openFilesTab(group, target.workspace); - if (generation !== this._generation) { + if (isCancelled()) { return; } } // [4] Open Changes (active on submit so the detail panel maps to it). if (openChanges && changesResource) { - if (!await this._openChangesTab(target.changesSessionResource!, changesResource, group, generation, activateChanges)) { + if (!await this._openChangesTab(target.changesSessionResource!, changesResource, group, activateChanges, isCancelled)) { return; } } @@ -391,13 +410,18 @@ export class SinglePaneDockedTabsCoordinator extends Disposable { // [5] Open the Files placeholder after Changes for created sessions. if (openFiles && !openFilesFirst) { await this._openFilesTab(group, target.workspace); - if (generation !== this._generation) { + if (isCancelled()) { return; } } + } catch (error) { + if (!this._store.isDisposed && !groupDisposed) { + throw error; + } } finally { suppression.dispose(); - if (generation === this._generation) { + groupDisposeListener.dispose(); + if (!isCancelled()) { if (trigger.workingSetRestored) { this._preserveMissingFilesForSessionKey = undefined; } @@ -417,11 +441,11 @@ export class SinglePaneDockedTabsCoordinator extends Disposable { // --- Tab operations --------------------------------------------------- - /** Opens the Changes editor pinned first (active on submit). Returns `false` if a newer reconcile superseded this one mid-open. */ - private async _openChangesTab(sessionResource: URI, changesResource: URI, group: IEditorGroup, generation: number, active: boolean): Promise { + /** Opens the Changes editor pinned first (active on submit). Returns `false` if the reconcile is cancelled mid-open. */ + private async _openChangesTab(sessionResource: URI, changesResource: URI, group: IEditorGroup, active: boolean, isCancelled: () => boolean): Promise { this._changesViewService.setChangesetId(undefined); await this._sessionChangesService.openChangesEditor(sessionResource, active ? CHANGES_TAB_ACTIVE_OPTIONS : CHANGES_TAB_OPTIONS, group); - if (generation !== this._generation) { + if (isCancelled()) { return false; } const changesEditor = this._findChangesEditor(group, changesResource); @@ -455,7 +479,7 @@ export class SinglePaneDockedTabsCoordinator extends Disposable { } } - private async _reconcileForeignChangesEditors(group: IEditorGroup, activeChangesResource: URI | undefined): Promise { + private async _reconcileForeignChangesEditors(group: IEditorGroup, activeChangesResource: URI | undefined, isCancelled: () => boolean): Promise { const foreign = group.editors.filter(editor => { const resource = this.getChangesEditorResource(editor); return resource && (!activeChangesResource || !isEqual(resource, activeChangesResource)); @@ -476,6 +500,9 @@ export class SinglePaneDockedTabsCoordinator extends Disposable { replacement: this._instantiationService.createInstance(SessionChangesEditorInput, activeChangesResource), options: wasActive ? CHANGES_TAB_ACTIVE_OPTIONS : CHANGES_TAB_OPTIONS, }]); + if (isCancelled()) { + return; + } if (editorsToClose.length > 0) { await this._closeManagedEditors(group, editorsToClose); } @@ -536,7 +563,7 @@ export class SinglePaneDockedTabsCoordinator extends Disposable { private _queueCollapseIfDetailsOnly(): void { if (!this._layoutService.isVisible(Parts.EDITOR_PART, mainWindow) && this._layoutService.isVisible(Parts.AUXILIARYBAR_PART)) { - void this._sequencer.queue(() => this._collapseNonManagedTabs()).catch(onUnexpectedError); + this._queue(() => this._collapseNonManagedTabs()); } } diff --git a/src/vs/sessions/contrib/layout/test/browser/desktopSessionLayoutController.test.ts b/src/vs/sessions/contrib/layout/test/browser/desktopSessionLayoutController.test.ts index 0f9776b969993f..320ecc7f425c5f 100644 --- a/src/vs/sessions/contrib/layout/test/browser/desktopSessionLayoutController.test.ts +++ b/src/vs/sessions/contrib/layout/test/browser/desktopSessionLayoutController.test.ts @@ -5,6 +5,7 @@ import assert from 'assert'; import { timeout } from '../../../../../base/common/async.js'; +import { errorHandler } from '../../../../../base/common/errors.js'; import { isEqual } from '../../../../../base/common/resources.js'; import { DisposableStore } from '../../../../../base/common/lifecycle.js'; import { ISettableObservable, transaction } from '../../../../../base/common/observable.js'; @@ -2981,6 +2982,126 @@ suite('LayoutController (desktop)', () => { assert.deepStrictEqual(publishedWorkspaces, ['c']); }); + test('[managed tabs / dispose] a reconcile stalled mid-open opens no further editors once the controller is disposed', async () => { + const controller = createSinglePaneController({ activateAux: true }); + await settle(); + + // Pause the reconcile at the first Changes open so it stalls before the Files tab opens. + let releaseChangesOpen!: () => void; + const changesOpenGate = new Promise(resolve => { releaseChangesOpen = resolve; }); + let gateArmed = true; + harness.onOpenChangesEditor = () => { + if (gateArmed) { + gateArmed = false; + return changesOpenGate; + } + return undefined; + }; + + // The created session's reconcile stalls awaiting the gated Changes open before the Files tab. + harness.activeSessionObs.set(makeSession(URI.parse('session:1'), { isCreated: true, changes: [makeChange('/file.ts')] }), undefined); + await settle(); + assert.strictEqual(hasFilesTab(), false, 'reconcile should be stalled before opening the Files tab'); + + // Dispose while stalled: the generation bump on dispose must make the resumed reconcile bail before any later editor open. + controller.dispose(); + releaseChangesOpen(); + await settle(); + + assert.strictEqual(hasFilesTab(), false, 'a reconcile resumed after dispose must not open further editors'); + }); + + test('[managed tabs / dispose] ignores an in-flight editor replacement failure after the controller is disposed', async () => { + const originalUnexpectedErrorHandler = errorHandler.getUnexpectedErrorHandler(); + const unexpectedErrors: Error[] = []; + errorHandler.setUnexpectedErrorHandler(error => unexpectedErrors.push(error)); + try { + const controller = createSinglePaneController({ activateAux: true }); + await settle(); + harness.activeSessionObs.set(makeSession(URI.parse('session:a')), undefined); + await settle(); + + let replaceStarted = false; + let rejectReplace!: (error: Error) => void; + const replaceGate = new Promise((_, reject) => { rejectReplace = reject; }); + harness.onReplaceEditors = replacements => { + replaceStarted = true; + store.add(replacements[0].replacement); + return replaceGate; + }; + + harness.activeSessionObs.set(makeSession(URI.parse('session:b')), undefined); + await settle(); + assert.strictEqual(replaceStarted, true, 'the reconcile should be stalled replacing the outgoing Changes editor'); + + controller.dispose(); + rejectReplace(new Error('InstantiationService has been disposed')); + await settle(); + + assert.deepStrictEqual(unexpectedErrors, []); + } finally { + errorHandler.setUnexpectedErrorHandler(originalUnexpectedErrorHandler); + } + }); + + test('[managed tabs / dispose] ignores an in-flight editor replacement failure after the target group is disposed', async () => { + const originalUnexpectedErrorHandler = errorHandler.getUnexpectedErrorHandler(); + const unexpectedErrors: Error[] = []; + errorHandler.setUnexpectedErrorHandler(error => unexpectedErrors.push(error)); + try { + createSinglePaneController({ activateAux: true }); + await settle(); + harness.activeSessionObs.set(makeSession(URI.parse('session:a')), undefined); + await settle(); + + let replaceStarted = false; + let rejectReplace!: (error: Error) => void; + const replaceGate = new Promise((_, reject) => { rejectReplace = reject; }); + harness.onReplaceEditors = replacements => { + replaceStarted = true; + store.add(replacements[0].replacement); + return replaceGate; + }; + + harness.activeSessionObs.set(makeSession(URI.parse('session:b')), undefined); + await settle(); + assert.strictEqual(replaceStarted, true, 'the reconcile should be stalled replacing the outgoing Changes editor'); + + harness.onWillDisposeActiveGroup.fire(); + rejectReplace(new Error('InstantiationService has been disposed')); + await settle(); + + assert.deepStrictEqual(unexpectedErrors, []); + } finally { + errorHandler.setUnexpectedErrorHandler(originalUnexpectedErrorHandler); + } + }); + + test('[managed tabs / errors] reports an editor replacement failure while the reconcile is active', async () => { + const originalUnexpectedErrorHandler = errorHandler.getUnexpectedErrorHandler(); + const unexpectedErrors: Error[] = []; + errorHandler.setUnexpectedErrorHandler(error => unexpectedErrors.push(error)); + try { + createSinglePaneController({ activateAux: true }); + await settle(); + harness.activeSessionObs.set(makeSession(URI.parse('session:a')), undefined); + await settle(); + + const failure = new Error('replace failed'); + harness.onReplaceEditors = replacements => { + store.add(replacements[0].replacement); + throw failure; + }; + + harness.activeSessionObs.set(makeSession(URI.parse('session:b')), undefined); + await settle(); + + assert.deepStrictEqual(unexpectedErrors, [failure]); + } finally { + errorHandler.setUnexpectedErrorHandler(originalUnexpectedErrorHandler); + } + }); + test('[managed tabs / details-only] always restores both docked inputs while only details are visible', async () => { createSinglePaneController({ activateAux: true, diff --git a/src/vs/sessions/contrib/layout/test/browser/layoutControllerTestUtils.ts b/src/vs/sessions/contrib/layout/test/browser/layoutControllerTestUtils.ts index 1e337915b040af..4f68574569d036 100644 --- a/src/vs/sessions/contrib/layout/test/browser/layoutControllerTestUtils.ts +++ b/src/vs/sessions/contrib/layout/test/browser/layoutControllerTestUtils.ts @@ -215,6 +215,8 @@ export interface ITestLayoutHarness { activateAux: boolean; /** Editors in the main part's active group (drives the single-pane managed-tab logic). */ activeGroupEditors: EditorInput[]; + /** Fires when the active editor group begins disposal. */ + onWillDisposeActiveGroup: Emitter; /** Records editors closed via `IEditorService.closeEditors`. */ closedEditors: EditorInput[]; /** Records untyped editors reopened via `IEditorService.openEditors`. */ @@ -248,7 +250,7 @@ export interface ITestLayoutHarness { /** Optional async hook awaited before `closeEditors` mutates the group. */ onCloseEditors?: () => Promise | void; /** Optional async hook awaited before `replaceEditors` mutates the group. */ - onReplaceEditors?: () => Promise | void; + onReplaceEditors?: (replacements: IEditorReplacement[]) => Promise | void; /** Records every `openChangesEditor` call for assertions (session + whether active). */ openChangesEditorCalls: { sessionResource: URI; active: boolean }[]; readonly sessionChangesService: ISessionChangesService; @@ -334,6 +336,7 @@ export function createTestHarness(store: DisposableStore, options: ICreateOption editorPartAutoVisibilitySuppressionDepth: 0, activateAux: options.activateAux ?? false, activeGroupEditors: [], + onWillDisposeActiveGroup: store.add(new Emitter()), closedEditors: [], openedEditors: [], closeSuppressionFlags: [], @@ -356,6 +359,7 @@ export function createTestHarness(store: DisposableStore, options: ICreateOption const testActiveGroup: IEditorGroup = new class extends mock() { override readonly id = 1; override get editors() { return harness.activeGroupEditors as IEditorGroup['editors']; } + override readonly onWillDispose = harness.onWillDisposeActiveGroup.event; override readonly onWillCloseEditor = harness.onWillCloseEditor.event as IEditorGroup['onWillCloseEditor']; override get count() { return harness.activeGroupEditors.length; } override get isEmpty() { return harness.activeGroupEditors.length === 0; } @@ -365,7 +369,7 @@ export function createTestHarness(store: DisposableStore, options: ICreateOption override pinEditor() { } override getIndexOfEditor(editor: EditorInput) { return harness.activeGroupEditors.indexOf(editor); } override async replaceEditors(replacements: IEditorReplacement[]) { - await harness.onReplaceEditors?.(); + await harness.onReplaceEditors?.(replacements); for (const replacement of replacements) { const index = harness.activeGroupEditors.indexOf(replacement.editor); if (index === -1) {