Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/vs/base/parts/ipc/common/ipc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1103,7 +1103,14 @@ export namespace ProxyChannel {
disableMarshalling?: boolean;
}

export interface ICreateServiceChannelOptions extends IProxyOptions { }
export interface ICreateServiceChannelOptions extends IProxyOptions {
/**
* If an event is triggered automatically but may not have subscribers,
* then `isLazyEvent` should return `true`. Otherwise the event data will
* be cached in the buffer which causing a memory leak
*/
isLazyEvent?: (key: string) => boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am reading this comment and fail to understand what actually happens when I use this option. If I understand correctly, a "lazy" event will not buffer any data until a first listener is added. But then I would update this comment to actually explain this better. And should be called differently?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I have updated the comment, please help review again .

}

export function fromService<TContext>(service: unknown, disposables: DisposableStore, options?: ICreateServiceChannelOptions): IServerChannel<TContext> {
const handler = service as { [key: string]: unknown };
Expand All @@ -1117,6 +1124,9 @@ export namespace ProxyChannel {
const mapEventNameToEvent = new Map<string, Event<unknown>>();
for (const key in handler) {
if (propertyIsEvent(key)) {
if (options?.isLazyEvent?.(key)) {
continue;
}
mapEventNameToEvent.set(key, Event.buffer(handler[key] as Event<unknown>, true, undefined, disposables));
}
}
Expand Down
39 changes: 39 additions & 0 deletions src/vs/base/parts/ipc/test/common/ipc.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -508,4 +508,43 @@ suite('Base IPC', function () {
client2.dispose();
});
});

suite('handle event lazily', function () {
let server: IPCServer;
let client: IPCClient;
let service: TestService;
let ipcService: ITestService;
const disposables = new DisposableStore();

setup(function () {
service = store.add(new TestService());
const testServer = disposables.add(new TestIPCServer());
server = testServer;

server.registerChannel(TestChannelId, ProxyChannel.fromService(service, disposables, { isLazyEvent: key => key === 'onPong' }));

client = disposables.add(testServer.createConnection('client1'));
ipcService = ProxyChannel.toService(client.getChannel(TestChannelId));
});

teardown(function () {
disposables.clear();
});

test('lazy event buffer', async function () {
// This message should be dropped
service.ping('hello 1');
// Listener should be called only once and the message should be 'hello 2'
const promise = new Promise(resolve => {
disposables.add(ipcService.onPong(msg => {
assert.strictEqual(msg, 'hello 2');
resolve(undefined);
}));
});
// This message should be received
service.ping('hello 2');
await promise;
});

});
});
4 changes: 3 additions & 1 deletion src/vs/code/electron-main/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1212,7 +1212,9 @@ export class CodeApplication extends Disposable {
sharedProcessClient.then(client => client.registerChannel('profileStorageListener', profileStorageListener));

// Terminal
const ptyHostChannel = ProxyChannel.fromService(accessor.get(ILocalPtyService), disposables);
const ptyHostChannel = ProxyChannel.fromService(accessor.get(ILocalPtyService), disposables, {
isLazyEvent: (key: string) => key === 'onProcessData'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whats the rule here to decide if an event should be lazy? Does this apply to other proxy channels?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing ! An event should be lazy if it is triggered automatically but may not have subscribers; otherwise, the event data will be cached in the buffer (Event.buffer), causing a memory leak. I only find that ptyHostService.onProcessData should be lazy currently.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@theanarkh I think what @bpasero is likely getting at is this should get fixed generically to prevent future leaks like this in the future. onProcessData is used, but conditionally, just because an event isn't listened to shouldn't mean the data should hang around forever. Any thoughts on how we could solve this generically for all events without hardcoding this particular event or compromising performance?

Copy link
Author

@theanarkh theanarkh Jan 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onProcessData is used in both local and remote scenarios. Only in the local scenario it have no subscribers. Therefore, we cannot directly configure onProcessData event to skip event buffer. Instead, we configure it in the caller of fromService. I haven't thought of a general way to deal with this problem currently.

});
mainProcessElectronServer.registerChannel(TerminalIpcChannels.LocalPty, ptyHostChannel);

// External Terminal
Expand Down