From ffd20f43f8ee1a7a016cd9b29c372e25ec685a62 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Tue, 26 Sep 2023 15:54:33 -0700 Subject: [PATCH] chore: dispose stale handles to prevent oom, 1000 of a kind max (#27315) https://github.com/microsoft/playwright/issues/6319 --- .github/workflows/tests_stress.yml | 2 + .../src/client/channelOwner.ts | 6 +- .../playwright-core/src/client/connection.ts | 4 +- .../src/server/dispatchers/dispatcher.ts | 60 +++++++++++++++---- tests/stress/heap.spec.ts | 41 ++++++++++++- 5 files changed, 95 insertions(+), 18 deletions(-) diff --git a/.github/workflows/tests_stress.yml b/.github/workflows/tests_stress.yml index bebf024835..d64eb07455 100644 --- a/.github/workflows/tests_stress.yml +++ b/.github/workflows/tests_stress.yml @@ -47,3 +47,5 @@ jobs: if: always() - run: npm run stest browsers -- --project=firefox if: always() + - run: npm run stest heap -- --project=chromium + if: always() diff --git a/packages/playwright-core/src/client/channelOwner.ts b/packages/playwright-core/src/client/channelOwner.ts index 3c3ed64415..9a76cb9307 100644 --- a/packages/playwright-core/src/client/channelOwner.ts +++ b/packages/playwright-core/src/client/channelOwner.ts @@ -40,6 +40,7 @@ export abstract class ChannelOwner = new Map(); + _wasCollected: boolean = false; constructor(parent: ChannelOwner | Connection, type: string, guid: string, initializer: channels.InitializerTraits) { super(); @@ -114,15 +115,16 @@ export abstract class ChannelOwner { if (this._closedErrorMessage) throw new Error(this._closedErrorMessage); + if (object._wasCollected) + throw new Error('The object has been collected to prevent unbounded heap growth.'); const { apiName, frames } = stackTrace || { apiName: '', frames: [] }; const guid = object._guid; @@ -170,7 +172,7 @@ export class Connection extends EventEmitter { } if (method === '__dispose__') { - object._dispose(); + object._dispose(params.reason); return; } diff --git a/packages/playwright-core/src/server/dispatchers/dispatcher.ts b/packages/playwright-core/src/server/dispatchers/dispatcher.ts index 54b939542d..66c65a4da6 100644 --- a/packages/playwright-core/src/server/dispatchers/dispatcher.ts +++ b/packages/playwright-core/src/server/dispatchers/dispatcher.ts @@ -35,6 +35,11 @@ export function existingDispatcher(object: any): DispatcherType return object[dispatcherSymbol]; } +let maxDispatchers = 1000; +export function setMaxDispatchersForTest(value: number | undefined) { + maxDispatchers = value || 1000; +} + export class Dispatcher extends EventEmitter implements channels.Channel { private _connection: DispatcherConnection; // Parent is always "isScope". @@ -55,18 +60,18 @@ export class Dispatcher { export class DispatcherConnection { readonly _dispatchers = new Map(); + // Collect stale dispatchers by type. + readonly _dispatchersByType = new Map>(); onmessage = (message: object) => {}; private _waitOperations = new Map(); private _isLocal: boolean; @@ -183,8 +191,8 @@ export class DispatcherConnection { this._sendMessageToClient(parent._guid, dispatcher._type, '__adopt__', { guid: dispatcher._guid }); } - sendDispose(dispatcher: DispatcherScope) { - this._sendMessageToClient(dispatcher._guid, dispatcher._type, '__dispose__', {}); + sendDispose(dispatcher: DispatcherScope, reason?: 'gc') { + this._sendMessageToClient(dispatcher._guid, dispatcher._type, '__dispose__', { reason }); } private _sendMessageToClient(guid: string, type: string, method: string, params: any, sdkObject?: SdkObject) { @@ -224,6 +232,32 @@ export class DispatcherConnection { throw new ValidationError(`${path}: expected dispatcher ${names.toString()}`); } + registerDispatcher(dispatcher: DispatcherScope) { + assert(!this._dispatchers.has(dispatcher._guid)); + this._dispatchers.set(dispatcher._guid, dispatcher); + const type = dispatcher._type; + + let list = this._dispatchersByType.get(type); + if (!list) { + list = new Set(); + this._dispatchersByType.set(type, list); + } + list.add(dispatcher._guid); + if (list.size > maxDispatchers) + this._disposeStaleDispatchers(type, list); + } + + private _disposeStaleDispatchers(type: string, dispatchers: Set) { + const dispatchersArray = [...dispatchers]; + this._dispatchersByType.set(type, new Set(dispatchersArray.slice(maxDispatchers / 10))); + for (let i = 0; i < maxDispatchers / 10; ++i) { + const d = this._dispatchers.get(dispatchersArray[i]); + if (!d) + continue; + d._dispose('gc'); + } + } + async dispatch(message: object) { const { id, guid, method, params, metadata } = message as any; const dispatcher = this._dispatchers.get(guid); diff --git a/tests/stress/heap.spec.ts b/tests/stress/heap.spec.ts index f41e3128ae..a88ab29005 100644 --- a/tests/stress/heap.spec.ts +++ b/tests/stress/heap.spec.ts @@ -18,6 +18,7 @@ import { contextTest as test, expect } from '../config/browserTest'; import { queryObjectCount } from '../config/queryObjects'; test.describe.configure({ mode: 'serial' }); +test.skip(({ browserName }) => browserName !== 'chromium'); for (let i = 0; i < 3; ++i) { test(`test #${i} to request page and context`, async ({ page, context }) => { @@ -65,7 +66,7 @@ test('should not leak dispatchers after closing page', async ({ context, server expect(await queryObjectCount(require('../../packages/playwright-core/lib/server/page').Page)).toBe(COUNT); expect(await queryObjectCount(require('../../packages/playwright-core/lib/server/dispatchers/networkDispatchers').RequestDispatcher)).toBe(COUNT); expect(await queryObjectCount(require('../../packages/playwright-core/lib/server/dispatchers/networkDispatchers').ResponseDispatcher)).toBe(COUNT); - expect(await queryObjectCount(require('../../packages/playwright-core/lib/server/dispatchers/consoleMessageDispatcher').ConsoleMessageDispatcher)).toBe(COUNT); + expect(await queryObjectCount(require('../../packages/playwright-core/lib/server/console').ConsoleMessage)).toBe(0); for (const page of pages) await page.close(); @@ -74,10 +75,46 @@ test('should not leak dispatchers after closing page', async ({ context, server expect(await queryObjectCount(require('../../packages/playwright-core/lib/server/page').Page)).toBe(0); expect(await queryObjectCount(require('../../packages/playwright-core/lib/server/dispatchers/networkDispatchers').RequestDispatcher)).toBe(0); expect(await queryObjectCount(require('../../packages/playwright-core/lib/server/dispatchers/networkDispatchers').ResponseDispatcher)).toBe(0); - expect(await queryObjectCount(require('../../packages/playwright-core/lib/server/dispatchers/consoleMessageDispatcher').ConsoleMessageDispatcher)).toBe(0); + expect(await queryObjectCount(require('../../packages/playwright-core/lib/server/console').ConsoleMessage)).toBe(0); expect(await queryObjectCount(require('../../packages/playwright-core/lib/client/page').Page)).toBeLessThan(COUNT); expect(await queryObjectCount(require('../../packages/playwright-core/lib/server/page').Page)).toBe(0); expect(await queryObjectCount(require('../../packages/playwright-core/lib/client/network').Request)).toBe(0); expect(await queryObjectCount(require('../../packages/playwright-core/lib/client/network').Response)).toBe(0); }); + +test.describe(() => { + test.beforeEach(() => { + require('../../packages/playwright-core/lib/server/dispatchers/dispatcher').setMaxDispatchersForTest(100); + }); + + test('should collect stale handles', async ({ page, server }) => { + page.on('request', () => {}); + const response = await page.goto(server.PREFIX + '/title.html'); + for (let i = 0; i < 200; ++i) { + await page.evaluate(async () => { + const response = await fetch('/'); + await response.text(); + }); + } + const e = await response.allHeaders().catch(e => e); + expect(e.message).toContain('The object has been collected to prevent unbounded heap growth.'); + + const counts = [ + { count: await queryObjectCount(require('../../packages/playwright-core/lib/client/network').Request), message: 'client.Request' }, + { count: await queryObjectCount(require('../../packages/playwright-core/lib/client/network').Response), message: 'client.Response' }, + { count: await queryObjectCount(require('../../packages/playwright-core/lib/server/network').Request), message: 'server.Request' }, + { count: await queryObjectCount(require('../../packages/playwright-core/lib/server/network').Response), message: 'server.Response' }, + { count: await queryObjectCount(require('../../packages/playwright-core/lib/server/dispatchers/networkDispatchers').RequestDispatcher), message: 'dispatchers.RequestDispatcher' }, + { count: await queryObjectCount(require('../../packages/playwright-core/lib/server/dispatchers/networkDispatchers').ResponseDispatcher), message: 'dispatchers.ResponseDispatcher' }, + ]; + for (const { count, message } of counts) { + expect(count, { message }).toBeGreaterThan(50); + expect(count, { message }).toBeLessThan(150); + } + }); + + test.afterEach(() => { + require('../../packages/playwright-core/lib/server/dispatchers/dispatcher').setMaxDispatchersForTest(null); + }); +});