Microsoft/vscode
1
0
Fork 0
mirror of https://github.com/microsoft/vscode.git synced 2026-02-27 21:23:48 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix ChatService.onDidDispose (#278917)

Browse Source 
* Fix ChatService.onDidDispose

* Fix?

* Wait for disposal

* Mop up remaining disposable leaks

* Dispose IChatService

* And this

* Simplify

* Dispose Emitter
This commit is contained in:
Rob Lourens
2025-11-23 21:21:12 -08:00
committed by GitHub
parent 7e370bd080
commit 30cfcc9385
10 changed files with 42 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files

11
src/vs/workbench/contrib/chat/browser/chatWidget.ts
View File

@@ -1632,9 +1632,9 @@ export class ChatWidget extends Disposable implements IChatWidget {
}
}, 0);
dom.scheduleAtNextAnimationFrame(dom.getWindow(this.listContainer), () => {
this._register(dom.scheduleAtNextAnimationFrame(dom.getWindow(this.listContainer), () => {
this._onDidShow.fire();
});
}));
}
} else if (wasVisible) {
this._onDidHide.fire();
@@ -1991,11 +1991,11 @@ export class ChatWidget extends Disposable implements IChatWidget {
// Consider the tree to be scrolled all the way down if it is within 2px of the bottom.
const lastElementWasVisible = this.tree.scrollTop + this.tree.renderHeight >= this.previousTreeScrollHeight - 2;
if (lastElementWasVisible) {
dom.scheduleAtNextAnimationFrame(dom.getWindow(this.listContainer), () => {
this._register(dom.scheduleAtNextAnimationFrame(dom.getWindow(this.listContainer), () => {
// Can't set scrollTop during this event listener, the list might overwrite the change
this.scrollToEnd();
}, 0);
}, 0));
}
}
}
@@ -2205,7 +2205,8 @@ export class ChatWidget extends Disposable implements IChatWidget {
const renderImmediately = this.configurationService.getValue<boolean>('chat.experimental.renderMarkdownImmediately');
const delay = renderImmediately ? MicrotaskDelay : 0;
this.viewModelDisposables.add(Event.runAndSubscribe(Event.accumulate(this.viewModel.onDidChange, delay), (events => {
if (!this.viewModel) {
if (!this.viewModel || this._store.isDisposed) {
// See https://github.com/microsoft/vscode/issues/278969
return;
}

10
src/vs/workbench/contrib/chat/common/chatModelStore.ts
View File

@@ -4,7 +4,8 @@
*--------------------------------------------------------------------------------------------*/
import { CancellationToken } from '../../../../base/common/cancellation.js';
import { IDisposable, IReference, ReferenceCollection } from '../../../../base/common/lifecycle.js';
import { Emitter } from '../../../../base/common/event.js';
import { DisposableStore, IDisposable, IReference, ReferenceCollection } from '../../../../base/common/lifecycle.js';
import { ObservableMap } from '../../../../base/common/observable.js';
import { URI } from '../../../../base/common/uri.js';
import { ILogService } from '../../../../platform/log/common/log.js';
@@ -28,10 +29,15 @@ export interface ChatModelStoreDelegate {
}
export class ChatModelStore extends ReferenceCollection<ChatModel> implements IDisposable {
private readonly _store = new DisposableStore();
private readonly _models = new ObservableMap<string, ChatModel>();
private readonly _modelsToDispose = new Set<string>();
private readonly _pendingDisposals = new Set<Promise<void>>();
private readonly _onDidDisposeModel = this._store.add(new Emitter<ChatModel>());
public readonly onDidDisposeModel = this._onDidDisposeModel.event;
constructor(
private readonly delegate: ChatModelStoreDelegate,
@ILogService private readonly logService: ILogService,
@@ -108,6 +114,7 @@ export class ChatModelStore extends ReferenceCollection<ChatModel> implements ID
if (this._modelsToDispose.has(key)) {
this.logService.trace(`Disposing chat session ${key}`);
this._models.delete(key);
this._onDidDisposeModel.fire(object);
object.dispose();
}
this._modelsToDispose.delete(key);
@@ -126,6 +133,7 @@ export class ChatModelStore extends ReferenceCollection<ChatModel> implements ID
}
dispose(): void {
this._store.dispose();
this._models.forEach(model => model.dispose());
}
}

1
src/vs/workbench/contrib/chat/common/chatService.ts
View File

@@ -979,7 +979,6 @@ export interface IChatService {
adoptRequest(sessionResource: URI, request: IChatRequestModel): Promise<void>;
removeRequest(sessionResource: URI, requestId: string): Promise<void>;
cancelCurrentRequestForSession(sessionResource: URI): void;
forceClearSession(sessionResource: URI): Promise<void>;
addCompleteRequest(sessionResource: URI, message: IParsedChatRequest | string, variableData: IChatRequestVariableData | undefined, attempt: number | undefined, response: IChatCompleteResponse): void;
setChatSessionTitle(sessionResource: URI, title: string): void;
getLocalSessionHistory(): Promise<IChatDetail[]>;

18
src/vs/workbench/contrib/chat/common/chatServiceImpl.ts
View File

@@ -175,6 +175,9 @@ export class ChatService extends Disposable implements IChatService {
}
}
}));
this._register(this._sessionModels.onDidDisposeModel(model => {
this._onDidDisposeSession.fire({ sessionResource: model.sessionResource, reason: 'cleared' });
}));
this._chatServiceTelemetry = this.instantiationService.createInstance(ChatServiceTelemetry);
@@ -1252,21 +1255,6 @@ export class ChatService extends Disposable implements IChatService {
this._pendingRequests.deleteAndDispose(sessionResource);
}
// TODO should not exist
async forceClearSession(sessionResource: URI): Promise<void> {
this.trace('clearSession', `session: ${sessionResource}`);
const model = this._sessionModels.get(sessionResource);
if (!model) {
throw new Error(`Unknown session: ${sessionResource}`);
}
// this._sessionModels.delete(sessionResource);
model.dispose();
this._pendingRequests.get(sessionResource)?.cancel();
this._pendingRequests.deleteAndDispose(sessionResource);
this._onDidDisposeSession.fire({ sessionResource, reason: 'cleared' });
}
public hasSessions(): boolean {
return this._chatSessionStore.hasSessions();
}

4
src/vs/workbench/contrib/chat/test/browser/chatEditingService.test.ts
View File

@@ -161,7 +161,7 @@ suite('ChatEditingService', function () {
const uri = URI.from({ scheme: 'test', path: 'HelloWorld' });
const modelRef = chatService.startSession(ChatAgentLocation.Chat, CancellationToken.None);
const modelRef = store.add(chatService.startSession(ChatAgentLocation.Chat, CancellationToken.None));
const model = modelRef.object as ChatModel;
const session = model.editingSession;
if (!session) {
@@ -188,8 +188,6 @@ suite('ChatEditingService', function () {
await unset;
await entry.reject();
modelRef.dispose();
});
async function idleAfterEdit(session: IChatEditingSession, model: ChatModel, uri: URI, edits: TextEdit[]) {

19
src/vs/workbench/contrib/chat/test/common/chatService.test.ts
View File

@@ -7,6 +7,7 @@ import assert from 'assert';
import { CancellationToken } from '../../../../../base/common/cancellation.js';
import { Event } from '../../../../../base/common/event.js';
import { MarkdownString } from '../../../../../base/common/htmlContent.js';
import { DisposableStore } from '../../../../../base/common/lifecycle.js';
import { observableValue } from '../../../../../base/common/observable.js';
import { URI } from '../../../../../base/common/uri.js';
import { assertSnapshot } from '../../../../../base/test/common/snapshot.js';
@@ -43,7 +44,6 @@ import { IChatVariablesService } from '../../common/chatVariables.js';
import { ChatAgentLocation, ChatModeKind } from '../../common/constants.js';
import { MockChatService } from './mockChatService.js';
import { MockChatVariablesService } from './mockChatVariables.js';
import { DisposableStore } from '../../../../../base/common/lifecycle.js';
const chatAgentWithUsedContextId = 'ChatProviderWithUsedContext';
const chatAgentWithUsedContext: IChatAgent = {
@@ -397,6 +397,23 @@ suite('ChatService', () => {
await assertSnapshot(toSnapshotExportData(chatModel2));
});
test('onDidDisposeSession', async () => {
const testService = createChatService();
const modelRef = testService.startSession(ChatAgentLocation.Chat, CancellationToken.None);
const model = modelRef.object;
let disposed = false;
testDisposables.add(testService.onDidDisposeSession(e => {
if (e.sessionResource.toString() === model.sessionResource.toString()) {
disposed = true;
}
}));
modelRef.dispose();
await testService.waitForModelDisposals();
assert.strictEqual(disposed, true);
});
});

3
src/vs/workbench/contrib/chat/test/common/mockChatService.ts
View File

@@ -81,9 +81,6 @@ export class MockChatService implements IChatService {
cancelCurrentRequestForSession(sessionResource: URI): void {
throw new Error('Method not implemented.');
}
forceClearSession(sessionResource: URI): Promise<void> {
throw new Error('Method not implemented.');
}
addCompleteRequest(sessionResource: URI, message: IParsedChatRequest | string, variableData: IChatRequestVariableData | undefined, attempt: number | undefined, response: IChatCompleteResponse): void {
throw new Error('Method not implemented.');
}

7
src/vs/workbench/contrib/inlineChat/browser/inlineChatSessionServiceImpl.ts
View File

@@ -134,13 +134,6 @@ export class InlineChatSessionServiceImpl implements IInlineChatSessionService {
store.add(chatModelRef);
}
store.add(toDisposable(() => {
const doesOtherSessionUseChatModel = [...this._sessions.values()].some(data => data.session !== session && data.session.chatModel === chatModel);
if (!doesOtherSessionUseChatModel) {
this._chatService.forceClearSession(chatModel.sessionResource);
}
}));
const lastResponseListener = store.add(new MutableDisposable());
store.add(chatModel.onDidChange(e => {
if (e.kind !== 'addRequest' || !e.request.response) {

3
src/vs/workbench/contrib/inlineChat/test/browser/inlineChatController.test.ts
View File

@@ -275,9 +275,10 @@ suite('InlineChatController', function () {
});
teardown(function () {
teardown(async function () {
store.clear();
ctrl?.dispose();
await chatService.waitForModelDisposals();
});
// TODO@jrieken re-enable, looks like List/ChatWidget is leaking

4
src/vs/workbench/contrib/inlineChat/test/browser/inlineChatSession.test.ts
View File

@@ -144,6 +144,7 @@ suite('InlineChatSession', function () {
instaService = store.add(workbenchInstantiationService(undefined, store).createChild(serviceCollection));
inlineChatSessionService = store.add(instaService.get(IInlineChatSessionService));
store.add(instaService.get(IChatSessionsService) as ChatSessionsService); // Needs to be disposed in between test runs to clear extensionPoint contribution
store.add(instaService.get(IChatService) as ChatService);
instaService.get(IChatAgentService).registerDynamicAgent({
extensionId: nullExtensionDescription.identifier,
@@ -171,8 +172,9 @@ suite('InlineChatSession', function () {
editor = store.add(instantiateTestCodeEditor(instaService, model));
});
teardown(function () {
teardown(async function () {
store.clear();
await instaService.get(IChatService).waitForModelDisposals();
});
ensureNoDisposablesAreLeakedInTestSuite();