Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions packages/utils/processLauncher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,14 @@ export async function launchProcess(options: LaunchProcessOptions): Promise<Laun
let processClosed = false;
let fulfillCleanup = () => {};
const waitForCleanup = new Promise<void>(f => fulfillCleanup = f);
spawnedProcess.once('close', (exitCode, signal) => {
// Resolve cleanup on 'exit' (the process itself is gone) rather than waiting for
// 'close' (stdio EOF). A grandchild that inherits the browser's stdio pipe — for
// example msedge spawning EdgeUpdater — can keep the pipe open long after the
// browser process exits, which would otherwise delay close() until that grandchild
// exits. 'exit' always precedes 'close', so run the handler at most once.
const onProcessGone = (exitCode: number | null, signal: NodeJS.Signals | null) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are almost always regressions whenever this code is modified as it is used by everything

typically we use launch flags to disable stuff like this, so perhaps we're not doing that correctly or something changed recently in Edge?

if (processClosed)
return;
options.log(`[pid=${spawnedProcess.pid}] <process did exit: exitCode=${exitCode}, signal=${signal}>`);
processClosed = true;
gracefullyCloseSet.delete(gracefullyClose);
Expand All @@ -191,7 +198,9 @@ export async function launchProcess(options: LaunchProcessOptions): Promise<Laun
options.onExit(exitCode, signal);
// Cleanup as process exits.
cleanup().then(fulfillCleanup);
});
};
spawnedProcess.once('exit', onProcessGone);
spawnedProcess.once('close', onProcessGone);

addProcessHandlerIfNeeded('exit');
if (options.handleSIGINT)
Expand Down
50 changes: 50 additions & 0 deletions tests/library/launcher.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
* limitations under the License.
*/

import { launchProcess } from '@utils/processLauncher';
import { inheritAndCleanEnv } from '../config/utils';
import { playwrightTest as it, expect } from '../config/browserTest';

Expand Down Expand Up @@ -42,6 +43,55 @@ it('should kill browser process on timeout after close', async ({ browserType, m
expect(stalled).toBeTruthy();
});

it('should resolve close on process exit even if a child keeps stdio open', async ({ mode }) => {
it.skip(mode !== 'default', 'Exercises @utils/processLauncher directly; no browser needed');

// Regression: msedge spawns EdgeUpdater, which inherits the browser's stdio pipe
// and outlives it. The process 'close' event (stdio EOF) is then delayed until
// EdgeUpdater exits, which used to block close() for ~20s. Model that with a fake
// "browser" that backgrounds a grandchild inheriting stdio for a few seconds, then
// exits on graceful close. close() must resolve on the process 'exit' event rather
// than wait for the lingering grandchild to release the pipe.
const grandchildLifetimeMs = 5_000;
const script = [
`const cp = require('child_process');`,
// Detached grandchild in its own process group (like msedge's EdgeUpdater): it
// inherits this process's stdio pipe and outlives it, and is not taken down by a
// process-group kill of the parent.
`const g = cp.spawn(process.execPath, ['-e', 'setTimeout(() => {}, ${grandchildLifetimeMs})'], { stdio: ['ignore', 'inherit', 'inherit'], detached: true });`,
`g.unref();`,
`process.on('SIGTERM', () => process.exit(0));`,
// Signal readiness only after the grandchild holds the pipe and the SIGTERM
// handler is installed, so close() runs against a fully-started process.
`console.log('READY_FOR_CLOSE');`,
`setInterval(() => {}, 1000);`,
].join('\n');

let onReady = () => {};
const ready = new Promise<void>(f => onReady = f);
const result = await launchProcess({
command: process.execPath,
args: ['-e', script],
stdio: 'pipe',
tempDirectories: [],
attemptToGracefullyClose: async () => void result.launchedProcess.kill('SIGTERM'),
onExit: () => {},
log: message => {
if (message.includes('[out] READY_FOR_CLOSE'))
onReady();
},
});
await ready;

const start = Date.now();
await result.gracefullyClose();
const elapsed = Date.now() - start;

// With the fix, close resolves on 'exit' (~tens of ms). Without it, it waits for
// the grandchild to release the inherited pipe (~grandchildLifetimeMs).
expect(elapsed).toBeLessThan(grandchildLifetimeMs - 1_000);
});

it('should throw a friendly error if its headed and there is no xserver on linux running', async ({ mode, browserType, platform, channel }) => {
it.skip(platform !== 'linux');
it.skip(mode.startsWith('service'));
Expand Down
Loading