Skip to content

Commit a176838

Browse files
dgozmandgozman
committed
chore: remove Progress.raceWithCleanup
Now that legacy timeout mechanism is gone, we can replace all kinds of cleanups with an explicit try/catch.
1 parent d0c3cff commit a176838

File tree

15 files changed

+154
-102
lines changed

15 files changed

+154
-102
lines changed

‎packages/playwright-core/src/server/android/android.ts‎

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ export class Android extends SdkObject {
7979
newSerials.add(d.serial);
8080
if (this._devices.has(d.serial))
8181
continue;
82-
const device = await progress.raceWithCleanup(AndroidDevice.create(this, d, options), device => device.close());
82+
const device = await progress.race(AndroidDevice.create(this, d, options));
8383
this._devices.set(d.serial, device);
8484
}
8585
for (const d of this._devices.keys()) {
@@ -152,7 +152,7 @@ export class AndroidDevice extends SdkObject {
152152
}
153153

154154
async open(progress: Progress, command: string): Promise<SocketBackend> {
155-
return await progress.raceWithCleanup(this._backend.open(`${command}`), socket => socket.close());
155+
return await this._open(progress, command);
156156
}
157157

158158
async screenshot(): Promise<Buffer> {
@@ -216,7 +216,7 @@ export class AndroidDevice extends SdkObject {
216216
debug('pw:android')(`Polling the socket localabstract:${socketName}`);
217217
while (!socket) {
218218
try {
219-
socket = await progress.raceWithCleanup(this._backend.open(`localabstract:${socketName}`), socket => socket.close());
219+
socket = await this._open(progress, `localabstract:${socketName}`);
220220
} catch (e) {
221221
if (isAbortError(e))
222222
throw e;
@@ -354,14 +354,29 @@ export class AndroidDevice extends SdkObject {
354354
return defaultContext;
355355
}
356356

357+
private async _open(progress: Progress, command: string): Promise<SocketBackend> {
358+
let aborted = false;
359+
try {
360+
return await progress.race(this._backend.open(command).then(socket => {
361+
// Make sure to close the opened socket if progress was aborted before that.
362+
if (aborted)
363+
socket.close();
364+
return socket;
365+
}));
366+
} catch (error) {
367+
aborted = true;
368+
throw error;
369+
}
370+
}
371+
357372
webViews(): channels.AndroidWebView[] {
358373
return [...this._webViews.values()];
359374
}
360375

361376
async installApk(progress: Progress, content: Buffer, options?: { args?: string[] }): Promise<void> {
362377
const args = options && options.args ? options.args : ['-r', '-t', '-S'];
363378
debug('pw:android')('Opening install socket');
364-
const installSocket = await progress.raceWithCleanup(this._backend.open(`shell:cmd package install ${args.join(' ')} ${content.length}`), socket => socket.close());
379+
const installSocket = await this._open(progress, `shell:cmd package install ${args.join(' ')} ${content.length}`);
365380
debug('pw:android')('Writing driver bytes: ' + content.length);
366381
await progress.race(installSocket.write(content));
367382
const success = await progress.race(new Promise(f => installSocket.on('data', f)));
@@ -370,7 +385,7 @@ export class AndroidDevice extends SdkObject {
370385
}
371386

372387
async push(progress: Progress, content: Buffer, path: string, mode = 0o644): Promise<void> {
373-
const socket = await progress.raceWithCleanup(this._backend.open(`sync:`), socket => socket.close());
388+
const socket = await this._open(progress, `sync:`);
374389
const sendHeader = async (command: string, length: number) => {
375390
const buffer = Buffer.alloc(command.length + 4);
376391
buffer.write(command, 0);

‎packages/playwright-core/src/server/bidi/bidiChromium.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ export class BidiChromium extends BrowserType {
8282
}
8383

8484
override attemptToGracefullyCloseBrowser(transport: ConnectionTransport): void {
85+
// Note that it's fine to reuse the transport, since our connection ignores kBrowserCloseMessageId.
8586
const bidiTransport = (transport as any)[kBidiOverCdpWrapper];
8687
if (bidiTransport)
8788
transport = bidiTransport;

‎packages/playwright-core/src/server/bidi/bidiFirefox.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ export class BidiFirefox extends BrowserType {
7878
}
7979

8080
override attemptToGracefullyCloseBrowser(transport: ConnectionTransport): void {
81+
// Note that it's fine to reuse the transport, since our connection ignores kBrowserCloseMessageId.
8182
transport.send({ method: 'browser.close', params: {}, id: kBrowserCloseMessageId });
8283
}
8384

‎packages/playwright-core/src/server/browser.ts‎

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -100,20 +100,27 @@ export abstract class Browser extends SdkObject {
100100
async newContext(progress: Progress, options: types.BrowserContextOptions): Promise<BrowserContext> {
101101
validateBrowserContextOptions(options, this.options);
102102
let clientCertificatesProxy: ClientCertificatesProxy | undefined;
103-
if (options.clientCertificates?.length) {
104-
clientCertificatesProxy = await progress.raceWithCleanup(ClientCertificatesProxy.create(options), proxy => proxy.close());
105-
options = { ...options };
106-
options.proxyOverride = clientCertificatesProxy.proxySettings();
107-
options.internalIgnoreHTTPSErrors = true;
103+
let context: BrowserContext | undefined;
104+
try {
105+
if (options.clientCertificates?.length) {
106+
clientCertificatesProxy = await ClientCertificatesProxy.create(progress, options);
107+
options = { ...options };
108+
options.proxyOverride = clientCertificatesProxy.proxySettings();
109+
options.internalIgnoreHTTPSErrors = true;
110+
}
111+
context = await progress.race(this.doCreateNewContext(options));
112+
context._clientCertificatesProxy = clientCertificatesProxy;
113+
if ((options as any).__testHookBeforeSetStorageState)
114+
await progress.race((options as any).__testHookBeforeSetStorageState());
115+
if (options.storageState)
116+
await context.setStorageState(progress, options.storageState);
117+
this.emit(Browser.Events.Context, context);
118+
return context;
119+
} catch (error) {
120+
await context?.close({ reason: 'Failed to create context' }).catch(() => {});
121+
await clientCertificatesProxy?.close().catch(() => {});
122+
throw error;
108123
}
109-
const context = await progress.raceWithCleanup(this.doCreateNewContext(options), context => context.close({ reason: 'Failed to create context' }));
110-
context._clientCertificatesProxy = clientCertificatesProxy;
111-
if ((options as any).__testHookBeforeSetStorageState)
112-
await progress.race((options as any).__testHookBeforeSetStorageState());
113-
if (options.storageState)
114-
await context.setStorageState(progress, options.storageState);
115-
this.emit(Browser.Events.Context, context);
116-
return context;
117124
}
118125

119126
async newContextForReuse(progress: Progress, params: channels.BrowserNewContextForReuseParams): Promise<BrowserContext> {

‎packages/playwright-core/src/server/browserContext.ts‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -527,14 +527,20 @@ export abstract class BrowserContext extends SdkObject {
527527
}
528528

529529
async newPage(progress: Progress, isServerSide: boolean): Promise<Page> {
530-
const page = await progress.raceWithCleanup(this.doCreateNewPage(isServerSide), page => page.close());
531-
const pageOrError = await progress.race(page.waitForInitializedOrError());
532-
if (pageOrError instanceof Page) {
533-
if (pageOrError.isClosed())
534-
throw new Error('Page has been closed.');
535-
return pageOrError;
530+
let page: Page | undefined;
531+
try {
532+
page = await progress.race(this.doCreateNewPage(isServerSide));
533+
const pageOrError = await progress.race(page.waitForInitializedOrError());
534+
if (pageOrError instanceof Page) {
535+
if (pageOrError.isClosed())
536+
throw new Error('Page has been closed.');
537+
return pageOrError;
538+
}
539+
throw pageOrError;
540+
} catch (error) {
541+
await page?.close({ reason: 'Failed to create page' }).catch(() => {});
542+
throw error;
536543
}
537-
throw pageOrError;
538544
}
539545

540546
addVisitedOrigin(origin: string) {

‎packages/playwright-core/src/server/browserType.ts‎

Lines changed: 65 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -78,14 +78,19 @@ export abstract class BrowserType extends SdkObject {
7878
// Note: Any initial TLS requests will fail since we rely on the Page/Frames initialize which sets ignoreHTTPSErrors.
7979
let clientCertificatesProxy: ClientCertificatesProxy | undefined;
8080
if (options.clientCertificates?.length) {
81-
clientCertificatesProxy = await progress.raceWithCleanup(ClientCertificatesProxy.create(options), proxy => proxy.close());
81+
clientCertificatesProxy = await ClientCertificatesProxy.create(progress, options);
8282
launchOptions.proxyOverride = clientCertificatesProxy.proxySettings();
8383
options = { ...options };
8484
options.internalIgnoreHTTPSErrors = true;
8585
}
86-
const browser = await this._innerLaunchWithRetries(progress, launchOptions, options, helper.debugProtocolLogger(), userDataDir).catch(e => { throw this._rewriteStartupLog(e); });
87-
browser._defaultContext!._clientCertificatesProxy = clientCertificatesProxy;
88-
return browser._defaultContext!;
86+
try {
87+
const browser = await this._innerLaunchWithRetries(progress, launchOptions, options, helper.debugProtocolLogger(), userDataDir).catch(e => { throw this._rewriteStartupLog(e); });
88+
browser._defaultContext!._clientCertificatesProxy = clientCertificatesProxy;
89+
return browser._defaultContext!;
90+
} catch (error) {
91+
await clientCertificatesProxy?.close().catch(() => {});
92+
throw error;
93+
}
8994
}
9095

9196
async _innerLaunchWithRetries(progress: Progress, options: types.LaunchOptions, persistent: types.BrowserContextOptions | undefined, protocolLogger: types.ProtocolLogger, userDataDir?: string): Promise<Browser> {
@@ -106,35 +111,40 @@ export abstract class BrowserType extends SdkObject {
106111
options.proxy = options.proxy ? normalizeProxySettings(options.proxy) : undefined;
107112
const browserLogsCollector = new RecentLogsCollector();
108113
const { browserProcess, userDataDir, artifactsDir, transport } = await this._launchProcess(progress, options, !!persistent, browserLogsCollector, maybeUserDataDir);
109-
if ((options as any).__testHookBeforeCreateBrowser)
110-
await progress.race((options as any).__testHookBeforeCreateBrowser());
111-
const browserOptions: BrowserOptions = {
112-
name: this._name,
113-
isChromium: this._name === 'chromium',
114-
channel: options.channel,
115-
slowMo: options.slowMo,
116-
persistent,
117-
headful: !options.headless,
118-
artifactsDir,
119-
downloadsPath: (options.downloadsPath || artifactsDir)!,
120-
tracesDir: (options.tracesDir || artifactsDir)!,
121-
browserProcess,
122-
customExecutablePath: options.executablePath,
123-
proxy: options.proxy,
124-
protocolLogger,
125-
browserLogsCollector,
126-
wsEndpoint: transport instanceof WebSocketTransport ? transport.wsEndpoint : undefined,
127-
originalLaunchOptions: options,
128-
};
129-
if (persistent)
130-
validateBrowserContextOptions(persistent, browserOptions);
131-
copyTestHooks(options, browserOptions);
132-
const browser = await progress.race(this.connectToTransport(transport, browserOptions, browserLogsCollector));
133-
(browser as any)._userDataDirForTest = userDataDir;
134-
// We assume no control when using custom arguments, and do not prepare the default context in that case.
135-
if (persistent && !options.ignoreAllDefaultArgs)
136-
await browser._defaultContext!._loadDefaultContext(progress);
137-
return browser;
114+
try {
115+
if ((options as any).__testHookBeforeCreateBrowser)
116+
await progress.race((options as any).__testHookBeforeCreateBrowser());
117+
const browserOptions: BrowserOptions = {
118+
name: this._name,
119+
isChromium: this._name === 'chromium',
120+
channel: options.channel,
121+
slowMo: options.slowMo,
122+
persistent,
123+
headful: !options.headless,
124+
artifactsDir,
125+
downloadsPath: (options.downloadsPath || artifactsDir)!,
126+
tracesDir: (options.tracesDir || artifactsDir)!,
127+
browserProcess,
128+
customExecutablePath: options.executablePath,
129+
proxy: options.proxy,
130+
protocolLogger,
131+
browserLogsCollector,
132+
wsEndpoint: transport instanceof WebSocketTransport ? transport.wsEndpoint : undefined,
133+
originalLaunchOptions: options,
134+
};
135+
if (persistent)
136+
validateBrowserContextOptions(persistent, browserOptions);
137+
copyTestHooks(options, browserOptions);
138+
const browser = await progress.race(this.connectToTransport(transport, browserOptions, browserLogsCollector));
139+
(browser as any)._userDataDirForTest = userDataDir;
140+
// We assume no control when using custom arguments, and do not prepare the default context in that case.
141+
if (persistent && !options.ignoreAllDefaultArgs)
142+
await browser._defaultContext!._loadDefaultContext(progress);
143+
return browser;
144+
} catch (error) {
145+
await browserProcess.close().catch(() => {});
146+
throw error;
147+
}
138148
}
139149

140150
private async _prepareToLaunch(options: types.LaunchOptions, isPersistent: boolean, userDataDir: string | undefined) {
@@ -194,7 +204,6 @@ export abstract class BrowserType extends SdkObject {
194204

195205
const env = options.env ? envArrayToObject(options.env) : process.env;
196206
const prepared = await progress.race(this._prepareToLaunch(options, isPersistent, userDataDir));
197-
progress.cleanupWhenAborted(() => removeFolders(prepared.tempDirectories));
198207

199208
// Note: it is important to define these variables before launchProcess, so that we don't get
200209
// "Cannot access 'browserServer' before initialization" if something went wrong.
@@ -217,10 +226,13 @@ export abstract class BrowserType extends SdkObject {
217226
attemptToGracefullyClose: async () => {
218227
if ((options as any).__testHookGracefullyClose)
219228
await (options as any).__testHookGracefullyClose();
220-
// We try to gracefully close to prevent crash reporting and core dumps.
221-
// Note that it's fine to reuse the pipe transport, since
222-
// our connection ignores kBrowserCloseMessageId.
223-
this.attemptToGracefullyCloseBrowser(transport!);
229+
if (transport) {
230+
// We try to gracefully close to prevent crash reporting and core dumps.
231+
this.attemptToGracefullyCloseBrowser(transport);
232+
} else {
233+
// When no transport is available, throw and trigger the force-kill.
234+
throw new Error();
235+
}
224236
},
225237
onExit: (exitCode, signal) => {
226238
// Unblock launch when browser prematurely exits.
@@ -249,19 +261,22 @@ export abstract class BrowserType extends SdkObject {
249261
close: () => closeOrKill((options as any).__testHookBrowserCloseTimeout || DEFAULT_PLAYWRIGHT_TIMEOUT),
250262
kill
251263
};
252-
progress.cleanupWhenAborted(() => closeOrKill(DEFAULT_PLAYWRIGHT_TIMEOUT));
253-
const { wsEndpoint } = await progress.race([
254-
this.waitForReadyState(options, browserLogsCollector),
255-
exitPromise.then(() => ({ wsEndpoint: undefined })),
256-
]);
257-
if (options.cdpPort !== undefined || !this.supportsPipeTransport()) {
258-
transport = await WebSocketTransport.connect(progress, wsEndpoint!);
259-
} else {
260-
const stdio = launchedProcess.stdio as unknown as [NodeJS.ReadableStream, NodeJS.WritableStream, NodeJS.WritableStream, NodeJS.WritableStream, NodeJS.ReadableStream];
261-
transport = new PipeTransport(stdio[3], stdio[4]);
264+
try {
265+
const { wsEndpoint } = await progress.race([
266+
this.waitForReadyState(options, browserLogsCollector),
267+
exitPromise.then(() => ({ wsEndpoint: undefined })),
268+
]);
269+
if (options.cdpPort !== undefined || !this.supportsPipeTransport()) {
270+
transport = await WebSocketTransport.connect(progress, wsEndpoint!);
271+
} else {
272+
const stdio = launchedProcess.stdio as unknown as [NodeJS.ReadableStream, NodeJS.WritableStream, NodeJS.WritableStream, NodeJS.WritableStream, NodeJS.ReadableStream];
273+
transport = new PipeTransport(stdio[3], stdio[4]);
274+
}
275+
return { browserProcess, artifactsDir: prepared.artifactsDir, userDataDir: prepared.userDataDir, transport };
276+
} catch (error) {
277+
await closeOrKill(DEFAULT_PLAYWRIGHT_TIMEOUT).catch(() => {});
278+
throw error;
262279
}
263-
progress.cleanupWhenAborted(() => transport.close());
264-
return { browserProcess, artifactsDir: prepared.artifactsDir, userDataDir: prepared.userDataDir, transport };
265280
}
266281

267282
async _createArtifactDirs(options: types.LaunchOptions): Promise<void> {

‎packages/playwright-core/src/server/chromium/chromium.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,7 @@ export class Chromium extends BrowserType {
163163
}
164164

165165
override attemptToGracefullyCloseBrowser(transport: ConnectionTransport): void {
166+
// Note that it's fine to reuse the transport, since our connection ignores kBrowserCloseMessageId.
166167
const message: ProtocolRequest = { method: 'Browser.close', id: kBrowserCloseMessageId, params: {} };
167168
transport.send(message);
168169
}

‎packages/playwright-core/src/server/firefox/firefox.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ export class Firefox extends BrowserType {
6565
}
6666

6767
override attemptToGracefullyCloseBrowser(transport: ConnectionTransport): void {
68+
// Note that it's fine to reuse the transport, since our connection ignores kBrowserCloseMessageId.
6869
const message = { method: 'Browser.close', params: {}, id: kBrowserCloseMessageId };
6970
transport.send(message);
7071
}

‎packages/playwright-core/src/server/frames.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -761,7 +761,7 @@ export class Frame extends SdkObject {
761761
return null;
762762
return continuePolling;
763763
}
764-
const result = await progress.raceWithCleanup(resolved.injected.evaluateHandle((injected, { info, root }) => {
764+
const result = await progress.race(resolved.injected.evaluateHandle((injected, { info, root }) => {
765765
if (root && !root.isConnected)
766766
throw injected.createStacklessError('Element is not attached to the DOM');
767767
const elements = injected.querySelectorAll(info.parsed, root || document);
@@ -776,7 +776,7 @@ export class Frame extends SdkObject {
776776
log = ` locator resolved to ${visible ? 'visible' : 'hidden'} ${injected.previewNode(element)}`;
777777
}
778778
return { log, element, visible, attached: !!element };
779-
}, { info: resolved.info, root: resolved.frame === this ? scope : undefined }), handle => handle.dispose());
779+
}, { info: resolved.info, root: resolved.frame === this ? scope : undefined }));
780780
const { log, visible, attached } = await progress.race(result.evaluate(r => ({ log: r.log, visible: r.visible, attached: r.attached })));
781781
if (log)
782782
progress.log(log);
@@ -1085,7 +1085,7 @@ export class Frame extends SdkObject {
10851085
const resolved = await progress.race(this.selectors.resolveInjectedForSelector(selector, { strict }));
10861086
if (!resolved)
10871087
return continuePolling;
1088-
const result = await progress.raceWithCleanup(resolved.injected.evaluateHandle((injected, { info, callId }) => {
1088+
const result = await progress.race(resolved.injected.evaluateHandle((injected, { info, callId }) => {
10891089
const elements = injected.querySelectorAll(info.parsed, document);
10901090
if (callId)
10911091
injected.markTargetElements(new Set(elements), callId);
@@ -1099,7 +1099,7 @@ export class Frame extends SdkObject {
10991099
log = ` locator resolved to ${injected.previewNode(element)}`;
11001100
}
11011101
return { log, success: !!element, element };
1102-
}, { info: resolved.info, callId: progress.metadata.id }), handle => handle.dispose());
1102+
}, { info: resolved.info, callId: progress.metadata.id }));
11031103
const { log, success } = await progress.race(result.evaluate(r => ({ log: r.log, success: r.success })));
11041104
if (log)
11051105
progress.log(log);

‎packages/playwright-core/src/server/localUtils.ts‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -141,13 +141,18 @@ export async function harOpen(progress: Progress, harBackends: Map<string, HarBa
141141
let harBackend: HarBackend;
142142
if (params.file.endsWith('.zip')) {
143143
const zipFile = new ZipFile(params.file);
144-
const entryNames = await zipFile.entries();
145-
const harEntryName = entryNames.find(e => e.endsWith('.har'));
146-
if (!harEntryName)
147-
return { error: 'Specified archive does not have a .har file' };
148-
const har = await progress.raceWithCleanup(zipFile.read(harEntryName), () => zipFile.close());
149-
const harFile = JSON.parse(har.toString()) as har.HARFile;
150-
harBackend = new HarBackend(harFile, null, zipFile);
144+
try {
145+
const entryNames = await progress.race(zipFile.entries());
146+
const harEntryName = entryNames.find(e => e.endsWith('.har'));
147+
if (!harEntryName)
148+
return { error: 'Specified archive does not have a .har file' };
149+
const har = await progress.race(zipFile.read(harEntryName));
150+
const harFile = JSON.parse(har.toString()) as har.HARFile;
151+
harBackend = new HarBackend(harFile, null, zipFile);
152+
} catch (error) {
153+
zipFile.close();
154+
throw error;
155+
}
151156
} else {
152157
const harFile = JSON.parse(await progress.race(fs.promises.readFile(params.file, 'utf-8'))) as har.HARFile;
153158
harBackend = new HarBackend(harFile, path.dirname(params.file), null);

0 commit comments

Comments
 (0)