From 202ffce0aa4f34dba35fbb8e7d740af16efee35f Mon Sep 17 00:00:00 2001 From: Alex Rudenko Date: Wed, 24 Aug 2022 12:05:32 +0200 Subject: [PATCH] fix: remove deferred promise timeouts (#8835) Issue #8832 makes a good point that we should not be making implicit assumptions about the client's performance when waiting for internal events. At the same time, we want to be able to get the debug info if some promises never resolve because of missing backend events. This PR adds a variable to turn on timeouts for deferred promises created using `createDebuggableDeferredPromise`. We can use it in our tests to catch never-resolving promises or when reproducing bug reports where Puppeteer hangs indefinitely. Closes #8832 --- package.json | 8 ++++---- src/common/FrameManager.ts | 6 +++--- src/common/NetworkManager.ts | 7 +++---- src/environment.ts | 9 +++++++++ src/util/DeferredPromise.ts | 21 ++++++++++++++++++--- 5 files changed, 37 insertions(+), 14 deletions(-) diff --git a/package.json b/package.json index 13bdd1ca..ea306196 100644 --- a/package.json +++ b/package.json @@ -30,11 +30,11 @@ "test": "c8 --check-coverage --lines 93 run-s test:chrome:* test:firefox", "test:types": "tsd", "test:install": "scripts/test-install.sh", - "test:firefox": "cross-env PUPPETEER_PRODUCT=firefox MOZ_WEBRENDER=0 mocha", + "test:firefox": "cross-env PUPPETEER_PRODUCT=firefox MOZ_WEBRENDER=0 PUPPETEER_DEFERRED_PROMISE_DEBUG_TIMEOUT=20000 mocha", "test:chrome": "run-s test:chrome:*", - "test:chrome:headless": "cross-env HEADLESS=true mocha", - "test:chrome:headless-chrome": "cross-env HEADLESS=chrome mocha", - "test:chrome:headful": "cross-env HEADLESS=false mocha", + "test:chrome:headless": "cross-env HEADLESS=true PUPPETEER_DEFERRED_PROMISE_DEBUG_TIMEOUT=20000 mocha", + "test:chrome:headless-chrome": "cross-env HEADLESS=chrome PUPPETEER_DEFERRED_PROMISE_DEBUG_TIMEOUT=20000 mocha", + "test:chrome:headful": "cross-env HEADLESS=false PUPPETEER_DEFERRED_PROMISE_DEBUG_TIMEOUT=20000 mocha", "prepublishOnly": "npm run build", "prepare": "node typescript-if-required.js && husky install", "lint": "run-s lint:prettier lint:eslint", diff --git a/src/common/FrameManager.ts b/src/common/FrameManager.ts index 182dc1a1..e027cc62 100644 --- a/src/common/FrameManager.ts +++ b/src/common/FrameManager.ts @@ -17,7 +17,7 @@ import {Protocol} from 'devtools-protocol'; import {assert} from '../util/assert.js'; import { - createDeferredPromiseWithTimer, + createDebuggableDeferredPromise, DeferredPromise, } from '../util/DeferredPromise.js'; import {isErrorLike} from '../util/ErrorLike.js'; @@ -150,7 +150,7 @@ export class FrameManager extends EventEmitter { if (!this.#framesPendingTargetInit.has(targetId)) { this.#framesPendingTargetInit.set( targetId, - createDeferredPromiseWithTimer( + createDebuggableDeferredPromise( `Waiting for target frame ${targetId} failed` ) ); @@ -318,7 +318,7 @@ export class FrameManager extends EventEmitter { if (!this.#framesPendingAttachment.has(frameId)) { this.#framesPendingAttachment.set( frameId, - createDeferredPromiseWithTimer( + createDebuggableDeferredPromise( `Waiting for frame ${frameId} to attach failed` ) ); diff --git a/src/common/NetworkManager.ts b/src/common/NetworkManager.ts index f128892f..4aaacd4c 100644 --- a/src/common/NetworkManager.ts +++ b/src/common/NetworkManager.ts @@ -24,7 +24,7 @@ import {HTTPResponse} from './HTTPResponse.js'; import {FetchRequestId, NetworkEventManager} from './NetworkEventManager.js'; import {debugError, isString} from './util.js'; import { - createDeferredPromiseWithTimer, + createDebuggableDeferredPromise, DeferredPromise, } from '../util/DeferredPromise.js'; @@ -144,9 +144,8 @@ export class NetworkManager extends EventEmitter { if (this.#deferredInitPromise) { return this.#deferredInitPromise; } - this.#deferredInitPromise = createDeferredPromiseWithTimer( - 'NetworkManager initialization timed out', - 30000 + this.#deferredInitPromise = createDebuggableDeferredPromise( + 'NetworkManager initialization timed out' ); const init = Promise.all([ this.#ignoreHTTPSErrors diff --git a/src/environment.ts b/src/environment.ts index 5ceca84a..876eac8f 100644 --- a/src/environment.ts +++ b/src/environment.ts @@ -18,3 +18,12 @@ * @internal */ export const isNode = !!(typeof process !== 'undefined' && process.version); + +/** + * @internal + */ +export const deferredPromiseDebugTimeout = + typeof process !== 'undefined' && + typeof process.env['PUPPETEER_DEFERRED_PROMISE_DEBUG_TIMEOUT'] !== 'undefined' + ? Number(process.env['PUPPETEER_DEFERRED_PROMISE_DEBUG_TIMEOUT']) + : -1; diff --git a/src/util/DeferredPromise.ts b/src/util/DeferredPromise.ts index cc8d7f15..a91f5d96 100644 --- a/src/util/DeferredPromise.ts +++ b/src/util/DeferredPromise.ts @@ -1,15 +1,16 @@ import {TimeoutError} from '../common/Errors.js'; +import {deferredPromiseDebugTimeout} from '../environment.js'; /** * @internal */ - export interface DeferredPromise extends Promise { finished: () => boolean; resolved: () => boolean; resolve: (_: T) => void; reject: (_: Error) => void; } + /** * Creates an returns a promise along with the resolve/reject functions. * @@ -18,7 +19,6 @@ export interface DeferredPromise extends Promise { * * @internal */ - export function createDeferredPromiseWithTimer( timeoutMessage: string, timeout = 5000 @@ -54,12 +54,12 @@ export function createDeferredPromiseWithTimer( }, }); } + /** * Creates an returns a promise along with the resolve/reject functions. * * @internal */ - export function createDeferredPromise(): DeferredPromise { let isResolved = false; let isRejected = false; @@ -86,3 +86,18 @@ export function createDeferredPromise(): DeferredPromise { }, }); } + +/** + * @internal + */ +export function createDebuggableDeferredPromise( + timeoutMessage: string +): DeferredPromise { + if (deferredPromiseDebugTimeout > 0) { + return createDeferredPromiseWithTimer( + timeoutMessage, + deferredPromiseDebugTimeout + ); + } + return createDeferredPromise(); +}