From b2179193c68f6d7a092163e6741b48eb3f5975ba Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Mon, 27 Jul 2020 10:21:39 -0700 Subject: [PATCH] feat(rpc): replace implicit scopes with explicit dispose (#3173) This adds one more protocol message __dispose__ to dispose a scope and all child objects. Now, client side does not need a notion of scope anymore - it just disposes the whole object subtree upon __dispose__. Server, on the other hand, marks some objects as scopes and disposes them manually, also asserting that all parents are proper scopes. --- src/rpc/channels.ts | 2 -- src/rpc/client/browser.ts | 3 +- src/rpc/client/browserContext.ts | 3 +- src/rpc/client/browserServer.ts | 3 +- src/rpc/client/browserType.ts | 2 +- src/rpc/client/cdpSession.ts | 3 +- src/rpc/client/channelOwner.ts | 19 +++---------- src/rpc/client/connection.ts | 6 +++- src/rpc/client/electron.ts | 9 ++---- src/rpc/protocol.yml | 2 -- src/rpc/server/cdpSessionDispatcher.ts | 5 +--- src/rpc/server/dispatcher.ts | 36 +++++++++++++----------- test/channels.jest.js | 38 +++++++++++--------------- 13 files changed, 54 insertions(+), 77 deletions(-) diff --git a/src/rpc/channels.ts b/src/rpc/channels.ts index 6fc4d7917f..10df89b257 100644 --- a/src/rpc/channels.ts +++ b/src/rpc/channels.ts @@ -1575,7 +1575,6 @@ export type StreamReadResult = { export type CDPSessionInitializer = {}; export interface CDPSessionChannel extends Channel { on(event: 'event', callback: (params: CDPSessionEventEvent) => void): this; - on(event: 'disconnected', callback: (params: CDPSessionDisconnectedEvent) => void): this; send(params: CDPSessionSendParams): Promise; detach(params?: CDPSessionDetachParams): Promise; } @@ -1583,7 +1582,6 @@ export type CDPSessionEventEvent = { method: string, params?: SerializedValue, }; -export type CDPSessionDisconnectedEvent = {}; export type CDPSessionSendParams = { method: string, params?: SerializedValue, diff --git a/src/rpc/client/browser.ts b/src/rpc/client/browser.ts index 4bf9db4a5c..b45e3dd52d 100644 --- a/src/rpc/client/browser.ts +++ b/src/rpc/client/browser.ts @@ -40,13 +40,12 @@ export class Browser extends ChannelOwner { } constructor(parent: ChannelOwner, type: string, guid: string, initializer: BrowserInitializer) { - super(parent, type, guid, initializer, true); + super(parent, type, guid, initializer); this._browserType = parent as BrowserType; this._channel.on('close', () => { this._isConnected = false; this.emit(Events.Browser.Disconnected); this._isClosedOrClosing = true; - this._dispose(); }); this._closedPromise = new Promise(f => this.once(Events.Browser.Disconnected, f)); } diff --git a/src/rpc/client/browserContext.ts b/src/rpc/client/browserContext.ts index ad1e321ff2..13a7b85a3d 100644 --- a/src/rpc/client/browserContext.ts +++ b/src/rpc/client/browserContext.ts @@ -48,7 +48,7 @@ export class BrowserContext extends ChannelOwner { diff --git a/src/rpc/client/browserServer.ts b/src/rpc/client/browserServer.ts index 726ede933a..76e672bb26 100644 --- a/src/rpc/client/browserServer.ts +++ b/src/rpc/client/browserServer.ts @@ -25,10 +25,9 @@ export class BrowserServer extends ChannelOwner { this.emit(Events.BrowserServer.Close, exitCode === undefined ? null : exitCode, signal === undefined ? null : signal); - this._dispose(); }); } diff --git a/src/rpc/client/browserType.ts b/src/rpc/client/browserType.ts index 8fed4ca9f0..465423a7d6 100644 --- a/src/rpc/client/browserType.ts +++ b/src/rpc/client/browserType.ts @@ -34,7 +34,7 @@ export class BrowserType extends ChannelOwner(event: T, listener: (payload: T extends symbol ? any : Protocol.Events[T extends keyof Protocol.Events ? T : never]) => void) => this; constructor(parent: ChannelOwner, type: string, guid: string, initializer: CDPSessionInitializer) { - super(parent, type, guid, initializer, true); + super(parent, type, guid, initializer); this._channel.on('event', ({ method, params }) => { const cdpParams = params ? parseResult(params) : undefined; this.emit(method, cdpParams); }); - this._channel.on('disconnected', () => this._dispose()); this.on = super.on; this.addListener = super.addListener; diff --git a/src/rpc/client/channelOwner.ts b/src/rpc/client/channelOwner.ts index 90b251c0f3..b5a9041bef 100644 --- a/src/rpc/client/channelOwner.ts +++ b/src/rpc/client/channelOwner.ts @@ -17,16 +17,12 @@ import { EventEmitter } from 'events'; import type { Channel } from '../channels'; import type { Connection } from './connection'; -import { assert } from '../../helper'; import type { LoggerSink } from '../../loggerSink'; import { DebugLoggerSink } from '../../logger'; export abstract class ChannelOwner extends EventEmitter { private _connection: Connection; - private _isScope: boolean; - // Parent is always "isScope". private _parent: ChannelOwner | undefined; - // Only "isScope" channel owners have registered objects inside. private _objects = new Map(); readonly _type: string; @@ -35,12 +31,11 @@ export abstract class ChannelOwner o._debugScopeState()) : undefined, + objects: Array.from(this._objects.values()).map(o => o._debugScopeState()), }; } diff --git a/src/rpc/client/connection.ts b/src/rpc/client/connection.ts index 9f95bca692..13bd8a9d08 100644 --- a/src/rpc/client/connection.ts +++ b/src/rpc/client/connection.ts @@ -44,7 +44,7 @@ import { FirefoxBrowser } from './firefoxBrowser'; class Root extends ChannelOwner { constructor(connection: Connection) { - super(connection, '', '', {}, true); + super(connection, '', '', {}); } } @@ -97,6 +97,10 @@ export class Connection { this._createRemoteObject(guid, params.type, params.guid, params.initializer); return; } + if (method === '__dispose__') { + this._objects.get(guid)!._dispose(); + return; + } const object = this._objects.get(guid)!; object._channel.emit(method, this._replaceGuidsWithChannels(params)); } diff --git a/src/rpc/client/electron.ts b/src/rpc/client/electron.ts index 03b7136e39..458e2280e8 100644 --- a/src/rpc/client/electron.ts +++ b/src/rpc/client/electron.ts @@ -33,7 +33,7 @@ export class Electron extends ChannelOwner } constructor(parent: ChannelOwner, type: string, guid: string, initializer: ElectronInitializer) { - super(parent, type, guid, initializer, true); + super(parent, type, guid, initializer); } async launch(executablePath: string, options: ElectronLaunchOptionsBase & { logger?: LoggerSink } = {}): Promise { @@ -60,7 +60,7 @@ export class ElectronApplication extends ChannelOwner this._context = BrowserContext.from(context)); this._channel.on('window', ({ page, browserWindow }) => { const window = Page.from(page); @@ -69,10 +69,7 @@ export class ElectronApplication extends ChannelOwner this._windows.delete(window)); }); - this._channel.on('close', () => { - this.emit(ElectronEvents.ElectronApplication.Close); - this._dispose(); - }); + this._channel.on('close', () => this.emit(ElectronEvents.ElectronApplication.Close)); } windows(): Page[] { diff --git a/src/rpc/protocol.yml b/src/rpc/protocol.yml index c918baf51d..d189262fbc 100644 --- a/src/rpc/protocol.yml +++ b/src/rpc/protocol.yml @@ -1852,8 +1852,6 @@ CDPSession: method: string params: SerializedValue? - disconnected: - Electron: diff --git a/src/rpc/server/cdpSessionDispatcher.ts b/src/rpc/server/cdpSessionDispatcher.ts index 1d317114de..6f0d818294 100644 --- a/src/rpc/server/cdpSessionDispatcher.ts +++ b/src/rpc/server/cdpSessionDispatcher.ts @@ -26,10 +26,7 @@ export class CDPSessionDispatcher extends Dispatcher { - this._dispatchEvent('disconnected'); - this._dispose(); - }); + crSession.on(CRSessionEvents.Disconnected, () => this._dispose()); } async send(params: { method: string, params?: SerializedValue }): Promise<{ result: SerializedValue }> { diff --git a/src/rpc/server/dispatcher.ts b/src/rpc/server/dispatcher.ts index 85dcdafac6..21c055bbf7 100644 --- a/src/rpc/server/dispatcher.ts +++ b/src/rpc/server/dispatcher.ts @@ -43,6 +43,7 @@ export class Dispatcher extends EventEmitter implements Chann private _parent: Dispatcher | undefined; // Only "isScope" channel owners have registered dispatchers inside. private _dispatchers = new Map>(); + private _disposed = false; readonly _guid: string; readonly _type: string; @@ -70,7 +71,7 @@ export class Dispatcher extends EventEmitter implements Chann (object as any)[dispatcherSymbol] = this; if (this._parent) - this._connection.sendMessageToClient(this._parent._guid, '__create__', { type, initializer, guid }); + this._connection.sendMessageToClient(this._parent._guid, '__create__', { type, initializer, guid }, !!isScope); } _dispatchEvent(method: string, params: Dispatcher | any = {}) { @@ -78,7 +79,7 @@ export class Dispatcher extends EventEmitter implements Chann } _dispose() { - assert(this._isScope); + assert(!this._disposed); // Clean up from parent and connection. if (this._parent) @@ -86,19 +87,18 @@ export class Dispatcher extends EventEmitter implements Chann this._connection._dispatchers.delete(this._guid); // Dispose all children. - for (const [guid, dispatcher] of [...this._dispatchers]) { - if (dispatcher._isScope) - dispatcher._dispose(); - else - this._connection._dispatchers.delete(guid); - } + for (const dispatcher of [...this._dispatchers.values()]) + dispatcher._dispose(); this._dispatchers.clear(); + + if (this._isScope) + this._connection.sendMessageToClient(this._guid, '__dispose__', {}); } _debugScopeState(): any { return { _guid: this._guid, - objects: this._isScope ? Array.from(this._dispatchers.values()).map(o => o._debugScopeState()) : undefined, + objects: Array.from(this._dispatchers.values()).map(o => o._debugScopeState()), }; } } @@ -117,8 +117,9 @@ export class DispatcherConnection { onmessage = (message: object) => {}; private _validateParams: (type: string, method: string, params: any) => any; - async sendMessageToClient(guid: string, method: string, params: any): Promise { - this.onmessage({ guid, method, params: this._replaceDispatchersWithGuids(params) }); + async sendMessageToClient(guid: string, method: string, params: any, disallowDispatchers?: boolean): Promise { + const allowDispatchers = !disallowDispatchers; + this.onmessage({ guid, method, params: this._replaceDispatchersWithGuids(params, allowDispatchers) }); } constructor() { @@ -165,23 +166,26 @@ export class DispatcherConnection { try { const validated = this._validateParams(dispatcher._type, method, params); const result = await (dispatcher as any)[method](validated); - this.onmessage({ id, result: this._replaceDispatchersWithGuids(result) }); + this.onmessage({ id, result: this._replaceDispatchersWithGuids(result, true) }); } catch (e) { this.onmessage({ id, error: serializeError(e) }); } } - private _replaceDispatchersWithGuids(payload: any): any { + private _replaceDispatchersWithGuids(payload: any, allowDispatchers: boolean): any { if (!payload) return payload; - if (payload instanceof Dispatcher) + if (payload instanceof Dispatcher) { + if (!allowDispatchers) + throw new Error(`Channels are not allowed in the scope's initialzier`); return { guid: payload._guid }; + } if (Array.isArray(payload)) - return payload.map(p => this._replaceDispatchersWithGuids(p)); + return payload.map(p => this._replaceDispatchersWithGuids(p, allowDispatchers)); if (typeof payload === 'object') { const result: any = {}; for (const key of Object.keys(payload)) - result[key] = this._replaceDispatchersWithGuids(payload[key]); + result[key] = this._replaceDispatchersWithGuids(payload[key], allowDispatchers); return result; } return payload; diff --git a/test/channels.jest.js b/test/channels.jest.js index 23223a20d9..1be794311b 100644 --- a/test/channels.jest.js +++ b/test/channels.jest.js @@ -31,8 +31,8 @@ describe.skip(!CHANNEL)('Channels', function() { ] }, { _guid: 'BrowserType', objects: [] }, { _guid: 'BrowserType', objects: [] }, - { _guid: 'Playwright' }, - { _guid: 'Selectors' }, + { _guid: 'Playwright', objects: [] }, + { _guid: 'Selectors', objects: [] }, { _guid: 'Electron', objects: [] }, ] }; @@ -49,15 +49,15 @@ describe.skip(!CHANNEL)('Channels', function() { { _guid: 'BrowserType', objects: [ { _guid: 'Browser', objects: [ { _guid: 'BrowserContext', objects: [ - { _guid: 'Frame' }, - { _guid: 'Page' }, - { _guid: 'Request' }, - { _guid: 'Response' }, + { _guid: 'Frame', objects: [] }, + { _guid: 'Page', objects: [] }, + { _guid: 'Request', objects: [] }, + { _guid: 'Response', objects: [] }, ]}, ] }, ] }, - { _guid: 'Playwright' }, - { _guid: 'Selectors' }, + { _guid: 'Playwright', objects: [] }, + { _guid: 'Selectors', objects: [] }, { _guid: 'Electron', objects: [] }, ] }); @@ -75,8 +75,8 @@ describe.skip(!CHANNEL)('Channels', function() { ] }, { _guid: 'BrowserType', objects: [] }, { _guid: 'BrowserType', objects: [] }, - { _guid: 'Playwright' }, - { _guid: 'Selectors' }, + { _guid: 'Playwright', objects: [] }, + { _guid: 'Selectors', objects: [] }, { _guid: 'Electron', objects: [] }, ] }; @@ -93,8 +93,8 @@ describe.skip(!CHANNEL)('Channels', function() { ] }, { _guid: 'BrowserType', objects: [] }, { _guid: 'BrowserType', objects: [] }, - { _guid: 'Playwright' }, - { _guid: 'Selectors' }, + { _guid: 'Playwright', objects: [] }, + { _guid: 'Selectors', objects: [] }, { _guid: 'Electron', objects: [] }, ] }); @@ -112,8 +112,8 @@ describe.skip(!CHANNEL)('Channels', function() { ] }, { _guid: 'BrowserType', objects: [] }, { _guid: 'BrowserType', objects: [] }, - { _guid: 'Playwright' }, - { _guid: 'Selectors' }, + { _guid: 'Playwright', objects: [] }, + { _guid: 'Selectors', objects: [] }, { _guid: 'Electron', objects: [] }, ] }; @@ -132,8 +132,8 @@ describe.skip(!CHANNEL)('Channels', function() { ] }, { _guid: 'BrowserType', objects: [] }, { _guid: 'BrowserType', objects: [] }, - { _guid: 'Playwright' }, - { _guid: 'Selectors' }, + { _guid: 'Playwright', objects: [] }, + { _guid: 'Selectors', objects: [] }, { _guid: 'Electron', objects: [] }, ] }); @@ -154,12 +154,6 @@ async function expectScopeState(object, golden) { function compareObjects(a, b) { if (a._guid !== b._guid) return a._guid.localeCompare(b._guid); - if (a.objects && !b.objects) - return -1; - if (!a.objects && b.objects) - return 1; - if (!a.objects && !b.objects) - return 0; return a.objects.length - b.objects.length; }