fix: implement protocol-level timeouts (#9877)

This commit is contained in:
Alex Rudenko 2023-03-21 12:53:13 +01:00 committed by GitHub
parent f1488b6c3a
commit 510b36c500
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
17 changed files with 307 additions and 253 deletions

View File

@ -74,7 +74,6 @@ sidebar_label: API
| [ClickOptions](./puppeteer.clickoptions.md) | |
| [CommonEventEmitter](./puppeteer.commoneventemitter.md) | |
| [Configuration](./puppeteer.configuration.md) | <p>Defines options to configure Puppeteer's behavior during installation and runtime.</p><p>See individual properties for more information.</p> |
| [ConnectionCallback](./puppeteer.connectioncallback.md) | |
| [ConnectionTransport](./puppeteer.connectiontransport.md) | |
| [ConnectOptions](./puppeteer.connectoptions.md) | |
| [ConsoleMessageLocation](./puppeteer.consolemessagelocation.md) | |

View File

@ -18,5 +18,6 @@ export interface BrowserConnectOptions
| ---------------------------------------------------------------------------- | --------- | ----------------------------------------------------------- | ------------------------------------------------------------------------------------------------------ | ------- |
| [defaultViewport?](./puppeteer.browserconnectoptions.defaultviewport.md) | | [Viewport](./puppeteer.viewport.md) \| null | _(Optional)_ Sets the viewport for each page. | |
| [ignoreHTTPSErrors?](./puppeteer.browserconnectoptions.ignorehttpserrors.md) | | boolean | _(Optional)_ Whether to ignore HTTPS errors during navigation. | false |
| [protocolTimeout?](./puppeteer.browserconnectoptions.protocoltimeout.md) | | number | _(Optional)_ Timeout setting for individual protocol (CDP) calls. | 30000 |
| [slowMo?](./puppeteer.browserconnectoptions.slowmo.md) | | number | _(Optional)_ Slows down Puppeteer operations by the specified amount of milliseconds to aid debugging. | |
| [targetFilter?](./puppeteer.browserconnectoptions.targetfilter.md) | | [TargetFilterCallback](./puppeteer.targetfiltercallback.md) | _(Optional)_ Callback to decide if Puppeteer should connect to a given target or not. | |

View File

@ -0,0 +1,19 @@
---
sidebar_label: BrowserConnectOptions.protocolTimeout
---
# BrowserConnectOptions.protocolTimeout property
Timeout setting for individual protocol (CDP) calls.
#### Signature:
```typescript
interface BrowserConnectOptions {
protocolTimeout?: number;
}
```
#### Default value:
30000

View File

@ -10,7 +10,12 @@ Constructs a new instance of the `Connection` class
```typescript
class Connection {
constructor(url: string, transport: ConnectionTransport, delay?: number);
constructor(
url: string,
transport: ConnectionTransport,
delay?: number,
timeout?: number
);
}
```
@ -21,3 +26,4 @@ class Connection {
| url | string | |
| transport | [ConnectionTransport](./puppeteer.connectiontransport.md) | |
| delay | number | _(Optional)_ |
| timeout | number | _(Optional)_ |

View File

@ -15,8 +15,14 @@ export declare class Connection extends EventEmitter
## Constructors
| Constructor | Modifiers | Description |
| ------------------------------------------------------------------------------- | --------- | -------------------------------------------------------------- |
| [(constructor)(url, transport, delay)](./puppeteer.connection._constructor_.md) | | Constructs a new instance of the <code>Connection</code> class |
| ---------------------------------------------------------------------------------------- | --------- | -------------------------------------------------------------- |
| [(constructor)(url, transport, delay, timeout)](./puppeteer.connection._constructor_.md) | | Constructs a new instance of the <code>Connection</code> class |
## Properties
| Property | Modifiers | Type | Description |
| -------------------------------------------- | --------------------- | ------ | ----------- |
| [timeout](./puppeteer.connection.timeout.md) | <code>readonly</code> | number | |
## Methods

View File

@ -0,0 +1,13 @@
---
sidebar_label: Connection.timeout
---
# Connection.timeout property
#### Signature:
```typescript
class Connection {
get timeout(): number;
}
```

View File

@ -1,13 +0,0 @@
---
sidebar_label: ConnectionCallback.error
---
# ConnectionCallback.error property
#### Signature:
```typescript
interface ConnectionCallback {
error: ProtocolError;
}
```

View File

@ -1,25 +0,0 @@
---
sidebar_label: ConnectionCallback
---
# ConnectionCallback interface
#### Signature:
```typescript
export interface ConnectionCallback
```
## Properties
| Property | Modifiers | Type | Description | Default |
| -------------------------------------------------- | --------- | --------------------------------------------- | ----------- | ------- |
| [error](./puppeteer.connectioncallback.error.md) | | [ProtocolError](./puppeteer.protocolerror.md) | | |
| [method](./puppeteer.connectioncallback.method.md) | | string | | |
## Methods
| Method | Description |
| ---------------------------------------------------------- | ----------- |
| [reject(args)](./puppeteer.connectioncallback.reject.md) | |
| [resolve(args)](./puppeteer.connectioncallback.resolve.md) | |

View File

@ -1,13 +0,0 @@
---
sidebar_label: ConnectionCallback.method
---
# ConnectionCallback.method property
#### Signature:
```typescript
interface ConnectionCallback {
method: string;
}
```

View File

@ -1,23 +0,0 @@
---
sidebar_label: ConnectionCallback.reject
---
# ConnectionCallback.reject() method
#### Signature:
```typescript
interface ConnectionCallback {
reject(args: unknown): void;
}
```
## Parameters
| Parameter | Type | Description |
| --------- | ------- | ----------- |
| args | unknown | |
**Returns:**
void

View File

@ -1,23 +0,0 @@
---
sidebar_label: ConnectionCallback.resolve
---
# ConnectionCallback.resolve() method
#### Signature:
```typescript
interface ConnectionCallback {
resolve(args: unknown): void;
}
```
## Parameters
| Parameter | Type | Description |
| --------- | ------- | ----------- |
| args | unknown | |
**Returns:**
void

View File

@ -59,6 +59,12 @@ export interface BrowserConnectOptions {
* @internal
*/
protocol?: 'cdp' | 'webDriverBiDi';
/**
* Timeout setting for individual protocol (CDP) calls.
*
* @defaultValue 30000
*/
protocolTimeout?: number;
}
const getWebSocketTransportClass = async () => {
@ -87,6 +93,7 @@ export async function _connectToCDPBrowser(
slowMo = 0,
targetFilter,
_isPageTarget: isPageTarget,
protocolTimeout,
} = options;
assert(
@ -97,18 +104,28 @@ export async function _connectToCDPBrowser(
let connection!: Connection;
if (transport) {
connection = new Connection('', transport, slowMo);
connection = new Connection('', transport, slowMo, protocolTimeout);
} else if (browserWSEndpoint) {
const WebSocketClass = await getWebSocketTransportClass();
const connectionTransport: ConnectionTransport =
await WebSocketClass.create(browserWSEndpoint, headers);
connection = new Connection(browserWSEndpoint, connectionTransport, slowMo);
connection = new Connection(
browserWSEndpoint,
connectionTransport,
slowMo,
protocolTimeout
);
} else if (browserURL) {
const connectionURL = await getWSEndpoint(browserURL);
const WebSocketClass = await getWebSocketTransportClass();
const connectionTransport: ConnectionTransport =
await WebSocketClass.create(connectionURL);
connection = new Connection(connectionURL, connectionTransport, slowMo);
connection = new Connection(
connectionURL,
connectionTransport,
slowMo,
protocolTimeout
);
}
const version = await connection.send('Browser.getVersion');

View File

@ -18,6 +18,7 @@ import {Protocol} from 'devtools-protocol';
import {ProtocolMapping} from 'devtools-protocol/types/protocol-mapping.js';
import {assert} from '../util/assert.js';
import {createDeferredPromise} from '../util/util.js';
import {ConnectionTransport} from './ConnectionTransport.js';
import {debug} from './Debug.js';
@ -32,16 +33,6 @@ const debugProtocolReceive = debug('puppeteer:protocol:RECV ◀');
*/
export {ConnectionTransport, ProtocolMapping};
/**
* @public
*/
export interface ConnectionCallback {
resolve(args: unknown): void;
reject(args: unknown): void;
error: ProtocolError;
method: string;
}
/**
* Internal events that the Connection class emits.
*
@ -51,6 +42,140 @@ export const ConnectionEmittedEvents = {
Disconnected: Symbol('Connection.Disconnected'),
} as const;
/**
* @internal
*/
type GetIdFn = () => number;
/**
* @internal
*/
function createIncrementalIdGenerator(): GetIdFn {
let id = 0;
return (): number => {
return ++id;
};
}
/**
* @internal
*/
class Callback {
#id: number;
#error = new ProtocolError();
#promise = createDeferredPromise<unknown>();
#timer?: ReturnType<typeof setTimeout>;
#label: string;
constructor(id: number, label: string, timeout?: number) {
this.#id = id;
this.#label = label;
if (timeout) {
this.#timer = setTimeout(() => {
this.#promise.reject(rewriteError(this.#error, `${label} timed out.`));
}, timeout);
}
}
resolve(value: unknown): void {
clearTimeout(this.#timer);
this.#promise.resolve(value);
}
reject(error: Error): void {
clearTimeout(this.#timer);
this.#promise.reject(error);
}
get id(): number {
return this.#id;
}
get promise(): Promise<unknown> {
return this.#promise;
}
get error(): ProtocolError {
return this.#error;
}
get label(): string {
return this.#label;
}
}
/**
* Manages callbacks and their IDs for the protocol request/response communication.
*
* @internal
*/
export class CallbackRegistry {
#callbacks: Map<number, Callback> = new Map();
#idGenerator = createIncrementalIdGenerator();
create(
label: string,
timeout: number | undefined,
request: (id: number) => void
): Promise<unknown> {
const callback = new Callback(this.#idGenerator(), label, timeout);
this.#callbacks.set(callback.id, callback);
try {
request(callback.id);
} catch (error) {
// We still throw sync errors synchronously and clean up the scheduled
// callback.
callback.promise.catch(() => {
this.#callbacks.delete(callback.id);
});
callback.reject(error as Error);
throw error;
}
// Must only have sync code up until here.
return callback.promise.finally(() => {
this.#callbacks.delete(callback.id);
});
}
getCallback(id: number): Callback | undefined {
return this.#callbacks.get(id);
}
reject(id: number, message: string, originalMessage?: string): void {
const callback = this.#callbacks.get(id);
if (!callback) {
return;
}
this._reject(callback, message, originalMessage);
}
_reject(callback: Callback, message: string, originalMessage?: string): void {
callback.reject(
rewriteError(
callback.error,
`Protocol error (${callback.label}): ${message}`,
originalMessage
)
);
}
resolve(id: number, value: unknown): void {
const callback = this.#callbacks.get(id);
if (!callback) {
return;
}
callback.resolve(value);
}
clear(): void {
for (const callback of this.#callbacks.values()) {
// TODO: probably we can accept error messages as params.
this._reject(callback, 'Target closed');
}
this.#callbacks.clear();
}
}
/**
* @public
*/
@ -58,16 +183,22 @@ export class Connection extends EventEmitter {
#url: string;
#transport: ConnectionTransport;
#delay: number;
#lastId = 0;
#timeout: number;
#sessions: Map<string, CDPSessionImpl> = new Map();
#closed = false;
#callbacks: Map<number, ConnectionCallback> = new Map();
#manuallyAttached = new Set<string>();
#callbacks = new CallbackRegistry();
constructor(url: string, transport: ConnectionTransport, delay = 0) {
constructor(
url: string,
transport: ConnectionTransport,
delay = 0,
timeout?: number
) {
super();
this.#url = url;
this.#delay = delay;
this.#timeout = timeout ?? 30000;
this.#transport = transport;
this.#transport.onmessage = this.onMessage.bind(this);
@ -78,6 +209,10 @@ export class Connection extends EventEmitter {
return session.connection();
}
get timeout(): number {
return this.#timeout;
}
/**
* @internal
*/
@ -115,26 +250,28 @@ export class Connection extends EventEmitter {
// type-inference.
// So now we check if there are any params or not and deal with them accordingly.
const params = paramArgs.length ? paramArgs[0] : undefined;
const id = this._rawSend({method, params});
return new Promise((resolve, reject) => {
this.#callbacks.set(id, {
resolve,
reject,
error: new ProtocolError(),
method,
});
});
return this._rawSend(this.#callbacks, method, params);
}
/**
* @internal
*/
_rawSend(message: Record<string, unknown>): number {
const id = ++this.#lastId;
const stringifiedMessage = JSON.stringify(Object.assign({}, message, {id}));
_rawSend<T extends keyof ProtocolMapping.Commands>(
callbacks: CallbackRegistry,
method: T,
params: ProtocolMapping.Commands[T]['paramsType'][0],
sessionId?: string
): Promise<ProtocolMapping.Commands[T]['returnType']> {
return callbacks.create(method, this.#timeout, id => {
const stringifiedMessage = JSON.stringify({
method,
params,
id,
sessionId,
});
debugProtocolSend(stringifiedMessage);
this.#transport.send(stringifiedMessage);
return id;
}) as Promise<ProtocolMapping.Commands[T]['returnType']>;
}
/**
@ -179,17 +316,14 @@ export class Connection extends EventEmitter {
session._onMessage(object);
}
} else if (object.id) {
const callback = this.#callbacks.get(object.id);
// Callbacks could be all rejected if someone has called `.dispose()`.
if (callback) {
this.#callbacks.delete(object.id);
if (object.error) {
callback.reject(
createProtocolError(callback.error, callback.method, object)
this.#callbacks.reject(
object.id,
createProtocolErrorMessage(object),
object.error.message
);
} else {
callback.resolve(object.result);
}
this.#callbacks.resolve(object.id, object.result);
}
} else {
this.emit(object.method, object.params);
@ -203,14 +337,6 @@ export class Connection extends EventEmitter {
this.#closed = true;
this.#transport.onmessage = undefined;
this.#transport.onclose = undefined;
for (const callback of this.#callbacks.values()) {
callback.reject(
rewriteError(
callback.error,
`Protocol error (${callback.method}): Target closed.`
)
);
}
this.#callbacks.clear();
for (const session of this.#sessions.values()) {
session._onClosed();
@ -356,7 +482,7 @@ export class CDPSession extends EventEmitter {
export class CDPSessionImpl extends CDPSession {
#sessionId: string;
#targetType: string;
#callbacks: Map<number, ConnectionCallback> = new Map();
#callbacks = new CallbackRegistry();
#connection?: Connection;
/**
@ -386,39 +512,29 @@ export class CDPSessionImpl extends CDPSession {
)
);
}
// See the comment in Connection#send explaining why we do this.
const params = paramArgs.length ? paramArgs[0] : undefined;
const id = this.#connection._rawSend({
sessionId: this.#sessionId,
return this.#connection._rawSend(
this.#callbacks,
method,
params,
});
return new Promise((resolve, reject) => {
this.#callbacks.set(id, {
resolve,
reject,
error: new ProtocolError(),
method,
});
});
this.#sessionId
);
}
/**
* @internal
*/
_onMessage(object: CDPSessionOnMessageObject): void {
const callback = object.id ? this.#callbacks.get(object.id) : undefined;
if (object.id && callback) {
this.#callbacks.delete(object.id);
if (object.id) {
if (object.error) {
callback.reject(
createProtocolError(callback.error, callback.method, object)
this.#callbacks.reject(
object.id,
createProtocolErrorMessage(object),
object.error.message
);
} else {
callback.resolve(object.result);
this.#callbacks.resolve(object.id, object.result);
}
} else {
assert(!object.id);
@ -447,14 +563,6 @@ export class CDPSessionImpl extends CDPSession {
* @internal
*/
_onClosed(): void {
for (const callback of this.#callbacks.values()) {
callback.reject(
rewriteError(
callback.error,
`Protocol error (${callback.method}): Target closed.`
)
);
}
this.#callbacks.clear();
this.#connection = undefined;
this.emit(CDPSessionEmittedEvents.Disconnected);
@ -468,16 +576,14 @@ export class CDPSessionImpl extends CDPSession {
}
}
function createProtocolError(
error: ProtocolError,
method: string,
object: {error: {message: string; data: any; code: number}}
): Error {
let message = `Protocol error (${method}): ${object.error.message}`;
function createProtocolErrorMessage(object: {
error: {message: string; data: any; code: number};
}): string {
let message = `${object.error.message}`;
if ('data' in object.error) {
message += ` ${object.error.data}`;
}
return rewriteError(error, message, object.error.message);
return message;
}
function rewriteError(

View File

@ -16,10 +16,9 @@
import * as Bidi from 'chromium-bidi/lib/cjs/protocol/protocol.js';
import {ConnectionCallback} from '../Connection.js';
import {CallbackRegistry} from '../Connection.js';
import {ConnectionTransport} from '../ConnectionTransport.js';
import {debug} from '../Debug.js';
import {ProtocolError} from '../Errors.js';
import {EventEmitter} from '../EventEmitter.js';
import {Context} from './Context.js';
@ -81,14 +80,15 @@ interface Commands {
export class Connection extends EventEmitter {
#transport: ConnectionTransport;
#delay: number;
#lastId = 0;
#timeout? = 0;
#closed = false;
#callbacks: Map<number, ConnectionCallback> = new Map();
#callbacks = new CallbackRegistry();
#contexts: Map<string, Context> = new Map();
constructor(transport: ConnectionTransport, delay = 0) {
constructor(transport: ConnectionTransport, delay = 0, timeout?: number) {
super();
this.#delay = delay;
this.#timeout = timeout;
this.#transport = transport;
this.#transport.onmessage = this.onMessage.bind(this);
@ -107,7 +107,7 @@ export class Connection extends EventEmitter {
method: T,
params: Commands[T]['params']
): Promise<Commands[T]['returnType']> {
const id = ++this.#lastId;
return this.#callbacks.create(method, this.#timeout, id => {
const stringifiedMessage = JSON.stringify({
id,
method,
@ -115,14 +115,7 @@ export class Connection extends EventEmitter {
} as Bidi.Message.CommandRequest);
debugProtocolSend(stringifiedMessage);
this.#transport.send(stringifiedMessage);
return new Promise((resolve, reject) => {
this.#callbacks.set(id, {
resolve,
reject,
error: new ProtocolError(),
method,
});
});
}) as Promise<Commands[T]['returnType']>;
}
/**
@ -140,23 +133,23 @@ export class Connection extends EventEmitter {
| Bidi.Message.EventMessage;
if ('id' in object) {
const callback = this.#callbacks.get(object.id);
// Callbacks could be all rejected if someone has called `.dispose()`.
if (callback) {
this.#callbacks.delete(object.id);
if ('error' in object) {
callback.reject(
createProtocolError(callback.error, callback.method, object)
this.#callbacks.reject(
object.id,
createProtocolError(object),
object.message
);
} else {
if (callback.method === 'browsingContext.create') {
if (
this.#callbacks.getCallback(object.id)?.label ===
'browsingContext.create'
) {
this.#contexts.set(
object.result.context,
new Context(this, object.result)
);
}
callback.resolve(object);
}
this.#callbacks.resolve(object.id, object);
}
} else {
let context: Context | undefined;
@ -178,14 +171,6 @@ export class Connection extends EventEmitter {
this.#closed = true;
this.#transport.onmessage = undefined;
this.#transport.onclose = undefined;
for (const callback of this.#callbacks.values()) {
callback.reject(
rewriteError(
callback.error,
`Protocol error (${callback.method}): Connection closed.`
)
);
}
this.#callbacks.clear();
}
@ -195,27 +180,13 @@ export class Connection extends EventEmitter {
}
}
function rewriteError(
error: ProtocolError,
message: string,
originalMessage?: string
): Error {
error.message = message;
error.originalMessage = originalMessage ?? error.originalMessage;
return error;
}
/**
* @internal
*/
function createProtocolError(
error: ProtocolError,
method: string,
object: Bidi.Message.ErrorResult
): Error {
let message = `Protocol error (${method}): ${object.error} ${object.message}`;
function createProtocolError(object: Bidi.Message.ErrorResult): string {
let message = `${object.error} ${object.message}`;
if (object.stacktrace) {
message += ` ${object.stacktrace}`;
}
return rewriteError(error, message, object.message);
return message;
}

View File

@ -252,10 +252,11 @@ export class BrowserRunner {
timeout: number;
slowMo: number;
preferredRevision: string;
protocolTimeout?: number;
}): Promise<BiDiConnection> {
assert(this.proc, 'BrowserRunner not started.');
const {timeout, slowMo, preferredRevision} = options;
const {timeout, slowMo, preferredRevision, protocolTimeout} = options;
let browserWSEndpoint = await waitForWSEndpoint(
this.proc,
timeout,
@ -267,7 +268,7 @@ export class BrowserRunner {
const BiDi = await import(
/* webpackIgnore: true */ '../common/bidi/bidi.js'
);
return new BiDi.Connection(transport, slowMo);
return new BiDi.Connection(transport, slowMo, protocolTimeout);
}
async setupConnection(options: {
@ -275,10 +276,12 @@ export class BrowserRunner {
timeout: number;
slowMo: number;
preferredRevision: string;
protocolTimeout?: number;
}): Promise<Connection> {
assert(this.proc, 'BrowserRunner not started.');
const {usePipe, timeout, slowMo, preferredRevision} = options;
const {usePipe, timeout, slowMo, preferredRevision, protocolTimeout} =
options;
if (!usePipe) {
const browserWSEndpoint = await waitForWSEndpoint(
this.proc,
@ -286,7 +289,12 @@ export class BrowserRunner {
preferredRevision
);
const transport = await WebSocketTransport.create(browserWSEndpoint);
this.connection = new Connection(browserWSEndpoint, transport, slowMo);
this.connection = new Connection(
browserWSEndpoint,
transport,
slowMo,
protocolTimeout
);
} else {
// stdio was assigned during start(), and the 'pipe' option there adds the
// 4th and 5th items to stdio array
@ -295,7 +303,7 @@ export class BrowserRunner {
pipeWrite as NodeJS.WritableStream,
pipeRead as NodeJS.ReadableStream
);
this.connection = new Connection('', transport, slowMo);
this.connection = new Connection('', transport, slowMo, protocolTimeout);
}
return this.connection;
}

View File

@ -45,6 +45,7 @@ export class ChromeLauncher extends ProductLauncher {
waitForInitialPage = true,
debuggingPort,
protocol,
protocolTimeout,
} = options;
const chromeArguments = [];
@ -127,6 +128,7 @@ export class ChromeLauncher extends ProductLauncher {
timeout,
slowMo,
preferredRevision: this.puppeteer.browserRevision,
protocolTimeout,
});
if (protocol === 'webDriverBiDi') {

View File

@ -43,6 +43,7 @@ export class FirefoxLauncher extends ProductLauncher {
waitForInitialPage = true,
debuggingPort = null,
protocol = 'cdp',
protocolTimeout,
} = options;
const firefoxArguments = [];
@ -132,6 +133,7 @@ export class FirefoxLauncher extends ProductLauncher {
timeout,
slowMo,
preferredRevision: this.puppeteer.browserRevision,
protocolTimeout,
});
const BiDi = await import(
/* webpackIgnore: true */ '../common/bidi/bidi.js'
@ -156,6 +158,7 @@ export class FirefoxLauncher extends ProductLauncher {
timeout,
slowMo,
preferredRevision: this.puppeteer.browserRevision,
protocolTimeout,
});
browser = await CDPBrowser._create(
this.product,