diff --git a/packages/playwright-core/src/cli/driver.ts b/packages/playwright-core/src/cli/driver.ts index 7e9d9e4f86..0c21e33c6b 100644 --- a/packages/playwright-core/src/cli/driver.ts +++ b/packages/playwright-core/src/cli/driver.ts @@ -21,7 +21,7 @@ import * as playwright from '../..'; import type { BrowserType } from '../client/browserType'; import type { LaunchServerOptions } from '../client/types'; import { createPlaywright, DispatcherConnection, RootDispatcher, PlaywrightDispatcher } from '../server'; -import { IpcTransport, PipeTransport } from '../protocol/transport'; +import { PipeTransport } from '../protocol/transport'; import { PlaywrightServer } from '../remote/playwrightServer'; import { gracefullyCloseAll } from '../utils/processLauncher'; @@ -36,14 +36,19 @@ export function runDriver() { const playwright = createPlaywright(sdkLanguage); return new PlaywrightDispatcher(rootScope, playwright); }); - const transport = process.send ? new IpcTransport(process) : new PipeTransport(process.stdout, process.stdin); - transport.onmessage = message => dispatcherConnection.dispatch(JSON.parse(message)); + const transport = new PipeTransport(process.stdout, process.stdin); + transport.onmessage = (message: string) => dispatcherConnection.dispatch(JSON.parse(message)); dispatcherConnection.onmessage = message => transport.send(JSON.stringify(message)); transport.onclose = () => { // Drop any messages during shutdown on the floor. dispatcherConnection.onmessage = () => {}; selfDestruct(); }; + // Ignore the SIGINT signal in the driver process so the parent can gracefully close the connection. + // We still will destruct everything (close browsers and exit) when the transport pipe closes. + process.on('SIGINT', () => { + // Keep the process running. + }); } export type RunServerOptions = { diff --git a/packages/playwright-core/src/outofprocess.ts b/packages/playwright-core/src/outofprocess.ts index 1a7b0dcd08..89c5d16da2 100644 --- a/packages/playwright-core/src/outofprocess.ts +++ b/packages/playwright-core/src/outofprocess.ts @@ -15,7 +15,7 @@ */ import { Connection } from './client/connection'; -import { IpcTransport } from './protocol/transport'; +import { PipeTransport } from './protocol/transport'; import type { Playwright } from './client/playwright'; import * as childProcess from 'child_process'; import * as path from 'path'; @@ -32,12 +32,10 @@ class PlaywrightClient { _playwright: Promise; _driverProcess: childProcess.ChildProcess; private _closePromise = new ManualPromise(); - private _transport: IpcTransport; - private _stopped = false; constructor(env: any) { this._driverProcess = childProcess.fork(path.join(__dirname, 'cli', 'cli.js'), ['run-driver'], { - stdio: ['inherit', 'inherit', 'inherit', 'ipc'], + stdio: 'pipe', detached: true, env: { ...process.env, @@ -49,25 +47,23 @@ class PlaywrightClient { const connection = new Connection(); connection.markAsRemote(); - this._transport = new IpcTransport(this._driverProcess); - connection.onmessage = message => this._transport.send(JSON.stringify(message)); - this._transport.onmessage = message => connection.dispatch(JSON.parse(message)); - this._transport.onclose = () => this._closePromise.resolve(); + const transport = new PipeTransport(this._driverProcess.stdin!, this._driverProcess.stdout!); + connection.onmessage = message => transport.send(JSON.stringify(message)); + transport.onmessage = message => connection.dispatch(JSON.parse(message)); + transport.onclose = () => this._closePromise.resolve(); this._playwright = connection.initializePlaywright(); } async stop() { - this._stopped = true; - this._transport.close(); + this._driverProcess.removeListener('exit', this._onExit); + this._driverProcess.stdin!.destroy(); + this._driverProcess.stdout!.destroy(); + this._driverProcess.stderr!.destroy(); await this._closePromise; } private _onExit(exitCode: number | null, signal: string | null) { - if (this._stopped) - this._closePromise.resolve(); - else - throw new Error(`Server closed with exitCode=${exitCode} signal=${signal}`); + throw new Error(`Server closed with exitCode=${exitCode} signal=${signal}`); } - } diff --git a/packages/playwright-core/src/protocol/transport.ts b/packages/playwright-core/src/protocol/transport.ts index 92991af7ae..e647100c63 100644 --- a/packages/playwright-core/src/protocol/transport.ts +++ b/packages/playwright-core/src/protocol/transport.ts @@ -14,7 +14,6 @@ * limitations under the License. */ -import type { ChildProcess } from 'child_process'; import { makeWaitForNextTask } from '../utils'; export interface WritableStream { @@ -103,28 +102,3 @@ export class PipeTransport { } } } - -export class IpcTransport { - private _process: NodeJS.Process | ChildProcess; - private _waitForNextTask = makeWaitForNextTask(); - onmessage?: (message: string) => void; - onclose?: () => void; - - constructor(process: NodeJS.Process | ChildProcess) { - this._process = process; - this._process.on('message', message => this._waitForNextTask(() => { - if (message === '') - this.onclose?.(); - else - this.onmessage?.(message); - })); - } - - send(message: string) { - this._process.send!(message); - } - - close() { - this._process.send!(''); - } -} diff --git a/tests/installation/driver-should-work.spec.ts b/tests/installation/driver-should-work.spec.ts index d0024adb4b..7c30bcee6b 100755 --- a/tests/installation/driver-should-work.spec.ts +++ b/tests/installation/driver-should-work.spec.ts @@ -13,10 +13,29 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { test } from './npmTest'; +import { test, expect } from './npmTest'; test('driver should work', async ({ exec }) => { await exec('npm i --foreground-scripts playwright', { env: { PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD: '1' } }); await exec('npx playwright install'); await exec('node driver-client.js'); }); + +test('driver should ignore SIGINT', async ({ exec }) => { + test.skip(process.platform === 'win32', 'Only relevant for POSIX'); + test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright-python/issues/1843' }); + await exec('npm i --foreground-scripts playwright', { env: { PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD: '1' } }); + await exec('npx playwright install chromium'); + const output = await exec('node driver-client-sigint.js'); + const lines = output.split('\n').filter(l => l.trim()); + expect(lines).toEqual([ + 'launching driver', + 'launched driver', + 'closing gracefully', + 'closed page', + 'closed context', + 'closed browser', + 'stopped driver', + 'SUCCESS', + ]); +}); diff --git a/tests/installation/fixture-scripts/driver-client-sigint.js b/tests/installation/fixture-scripts/driver-client-sigint.js new file mode 100644 index 0000000000..8a36efaeff --- /dev/null +++ b/tests/installation/fixture-scripts/driver-client-sigint.js @@ -0,0 +1,54 @@ +/** + * Copyright (c) Microsoft Corporation. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +// @ts-check + +const pw = require.resolve('playwright'); +const oop = require.resolve('playwright-core/lib/outofprocess', { paths: [pw] }); +const { start } = require(oop); + +(async () => { + console.log('launching driver') + const { playwright, stop } = await start(); + console.log('launched driver') + try { + const browser = await playwright.chromium.launch({ handleSIGINT: false }); + const context = await browser.newContext(); + const page = await context.newPage(); + // let things settle down + await page.waitForTimeout(100); + // send SIGINT to driver + process.kill(playwright.driverProcess.pid, 'SIGINT'); + // wait and see if driver exits + await page.waitForTimeout(100); + console.log(`closing gracefully`) + await page.close(); + console.log('closed page'); + await context.close(); + console.log('closed context'); + await browser.close(); + console.log('closed browser'); + await stop(); + console.log('stopped driver'); + } catch (e) { + console.error(`Should be able to launch from ${process.cwd()}`); + console.error(e); + process.exit(1); + } + console.log(`SUCCESS`); +})().catch(err => { + console.error(err); + process.exit(1); +});