Skip to content

Commit 3e9713f

Browse files
authored
Don't collect codeblocks after chat part disposal (microsoft#242392)
Fix microsoft#241859
1 parent 23e2fba commit 3e9713f

File tree

5 files changed

+49
-21
lines changed

5 files changed

+49
-21
lines changed

src/vs/base/common/lifecycle.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -827,3 +827,19 @@ export class DisposableMap<K, V extends IDisposable = IDisposable> implements ID
827827
return this._store[Symbol.iterator]();
828828
}
829829
}
830+
831+
/**
832+
* Call `then` on a Promise, unless the returned disposable is disposed.
833+
*/
834+
export function thenIfNotDisposed<T>(promise: Promise<T>, then: (result: T) => void): IDisposable {
835+
let disposed = false;
836+
promise.then(result => {
837+
if (disposed) {
838+
return;
839+
}
840+
then(result);
841+
});
842+
return toDisposable(() => {
843+
disposed = true;
844+
});
845+
}

src/vs/base/test/common/lifecycle.test.ts

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55

66
import assert from 'assert';
77
import { Emitter } from '../../common/event.js';
8-
import { DisposableStore, dispose, IDisposable, markAsSingleton, ReferenceCollection, SafeDisposable, toDisposable } from '../../common/lifecycle.js';
8+
import { DisposableStore, dispose, IDisposable, markAsSingleton, ReferenceCollection, SafeDisposable, thenIfNotDisposed, toDisposable } from '../../common/lifecycle.js';
99
import { ensureNoDisposablesAreLeakedInTestSuite, throwIfDisposablesAreLeaked } from './utils.js';
1010

1111
class Disposable implements IDisposable {
@@ -328,4 +328,30 @@ suite('No Leakage Utilities', () => {
328328
toDisposable(() => { }).dispose();
329329
});
330330
});
331+
332+
suite('thenIfNotDisposed', () => {
333+
const store = ensureNoDisposablesAreLeakedInTestSuite();
334+
335+
test('normal case', async () => {
336+
let called = false;
337+
store.add(thenIfNotDisposed(Promise.resolve(123), (result: number) => {
338+
assert.strictEqual(result, 123);
339+
called = true;
340+
}));
341+
342+
await new Promise(resolve => setTimeout(resolve, 0));
343+
assert.strictEqual(called, true);
344+
});
345+
346+
test('disposed before promise resolves', async () => {
347+
let called = false;
348+
const disposable = thenIfNotDisposed(Promise.resolve(123), () => {
349+
called = true;
350+
});
351+
352+
disposable.dispose();
353+
await new Promise(resolve => setTimeout(resolve, 0));
354+
assert.strictEqual(called, false);
355+
});
356+
});
331357
});

src/vs/workbench/contrib/chat/browser/chatListRenderer.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ import { FuzzyScore } from '../../../../base/common/filters.js';
1919
import { MarkdownString } from '../../../../base/common/htmlContent.js';
2020
import { Iterable } from '../../../../base/common/iterator.js';
2121
import { KeyCode } from '../../../../base/common/keyCodes.js';
22-
import { Disposable, DisposableStore, IDisposable, dispose, toDisposable } from '../../../../base/common/lifecycle.js';
22+
import { Disposable, DisposableStore, IDisposable, dispose, thenIfNotDisposed, toDisposable } from '../../../../base/common/lifecycle.js';
2323
import { ResourceMap } from '../../../../base/common/map.js';
2424
import { FileAccess } from '../../../../base/common/network.js';
2525
import { clamp } from '../../../../base/common/numbers.js';
@@ -986,7 +986,7 @@ export class ChatListItemRenderer extends Disposable implements ITreeRenderer<Ch
986986

987987
part.codeblocks?.forEach((info, i) => {
988988
codeBlocksByResponseId[codeBlockStartIndex + i] = info;
989-
info.uriPromise.then(uri => {
989+
part.addDisposable!(thenIfNotDisposed(info.uriPromise, uri => {
990990
if (!uri) {
991991
return;
992992
}
@@ -998,7 +998,7 @@ export class ChatListItemRenderer extends Disposable implements ITreeRenderer<Ch
998998
this.codeBlocksByEditorUri.delete(uri);
999999
}
10001000
}));
1001-
});
1001+
}));
10021002
});
10031003

10041004
}

src/vs/workbench/contrib/mergeEditor/browser/utils.ts

Lines changed: 1 addition & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55

66
import { ArrayQueue, CompareResult } from '../../../../base/common/arrays.js';
77
import { onUnexpectedError } from '../../../../base/common/errors.js';
8-
import { DisposableStore, IDisposable, toDisposable } from '../../../../base/common/lifecycle.js';
8+
import { DisposableStore, IDisposable } from '../../../../base/common/lifecycle.js';
99
import { IObservable, autorunOpts } from '../../../../base/common/observable.js';
1010
import { CodeEditorWidget } from '../../../../editor/browser/widget/codeEditor/codeEditorWidget.js';
1111
import { IModelDeltaDecoration } from '../../../../editor/common/model.js';
@@ -85,19 +85,6 @@ export function elementAtOrUndefined<T>(arr: T[], index: number): T | undefined
8585
return arr[index];
8686
}
8787

88-
export function thenIfNotDisposed<T>(promise: Promise<T>, then: () => void): IDisposable {
89-
let disposed = false;
90-
promise.then(() => {
91-
if (disposed) {
92-
return;
93-
}
94-
then();
95-
});
96-
return toDisposable(() => {
97-
disposed = true;
98-
});
99-
}
100-
10188
export function setFields<T extends {}>(obj: T, fields: Partial<T>): T {
10289
return Object.assign(obj, fields);
10390
}
@@ -154,4 +141,3 @@ export class PersistentStore<T> {
154141
);
155142
}
156143
}
157-

src/vs/workbench/contrib/mergeEditor/browser/view/mergeEditor.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import { CancellationToken } from '../../../../../base/common/cancellation.js';
1010
import { Color } from '../../../../../base/common/color.js';
1111
import { BugIndicatingError, onUnexpectedError } from '../../../../../base/common/errors.js';
1212
import { Emitter, Event } from '../../../../../base/common/event.js';
13-
import { Disposable, DisposableStore, IDisposable, MutableDisposable, toDisposable } from '../../../../../base/common/lifecycle.js';
13+
import { Disposable, DisposableStore, IDisposable, MutableDisposable, thenIfNotDisposed, toDisposable } from '../../../../../base/common/lifecycle.js';
1414
import { autorun, autorunWithStore, IObservable, IReader, observableValue, transaction } from '../../../../../base/common/observable.js';
1515
import { basename, isEqual } from '../../../../../base/common/resources.js';
1616
import { isDefined } from '../../../../../base/common/types.js';
@@ -39,7 +39,7 @@ import { readTransientState, writeTransientState } from '../../../codeEditor/bro
3939
import { MergeEditorInput } from '../mergeEditorInput.js';
4040
import { IMergeEditorInputModel } from '../mergeEditorInputModel.js';
4141
import { MergeEditorModel } from '../model/mergeEditorModel.js';
42-
import { deepMerge, PersistentStore, thenIfNotDisposed } from '../utils.js';
42+
import { deepMerge, PersistentStore } from '../utils.js';
4343
import { observableConfigValue } from '../../../../../platform/observable/common/platformObservableUtils.js';
4444
import { BaseCodeEditorView } from './editors/baseCodeEditorView.js';
4545
import { ScrollSynchronizer } from './scrollSynchronizer.js';

0 commit comments

Comments
 (0)