Skip to content

Commit

Permalink
feat: send SIGTERM to webserver before SIGKILL'ing it. (microsoft#18220)
Browse files Browse the repository at this point in the history
We now will send `SIGTERM` to the webserver and wait for the `timeout`
before sending `SIGKILL` to it.

Fixes microsoft#18209
  • Loading branch information
aslushnikov authored Oct 21, 2022
1 parent f5bc6cc commit c63a0b5
Show file tree
Hide file tree
Showing 6 changed files with 74 additions and 11 deletions.
2 changes: 1 addition & 1 deletion docs/src/test-api/class-testconfig.md
Original file line number Diff line number Diff line change
Expand Up @@ -656,7 +656,7 @@ export default config;
- `port` ?<[int]> The port that your http server is expected to appear on. It does wait until it accepts connections. Exactly one of `port` or `url` is required.
- `url` ?<[string]> The url on your http server that is expected to return a 2xx, 3xx, 400, 401, 402, or 403 status code when the server is ready to accept connections. Exactly one of `port` or `url` is required.
- `ignoreHTTPSErrors` ?<[boolean]> Whether to ignore HTTPS errors when fetching the `url`. Defaults to `false`.
- `timeout` ?<[int]> How long to wait for the process to start up and be available in milliseconds. Defaults to 60000.
- `timeout` ?<[int]> How long to wait for the process to start up and be available in milliseconds. The same timeout is also used to terminate the process. Defaults to 60000.
- `reuseExistingServer` ?<[boolean]> If true, it will re-use an existing server on the `port` or `url` when available. If no server is running on that `port` or `url`, it will run the command to start a new server. If `false`, it will throw if an existing process is listening on the `port` or `url`. This should be commonly set to `!process.env.CI` to allow the local dev server when running tests locally.
- `cwd` ?<[string]> Current working directory of the spawned process, defaults to the directory of the configuration file.
- `env` ?<[Object]<[string], [string]>> Environment variables to set for the command, `process.env` by default.
Expand Down
30 changes: 26 additions & 4 deletions packages/playwright-core/src/utils/processLauncher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ export type LaunchProcessOptions = {
type LaunchResult = {
launchedProcess: childProcess.ChildProcess,
gracefullyClose: () => Promise<void>,
kill: () => Promise<void>,
kill: (sendSigtermBeforeSigkillTimeout?: number) => Promise<void>,
};

export const gracefullyCloseSet = new Set<() => Promise<void>>();
Expand Down Expand Up @@ -188,6 +188,21 @@ export async function launchProcess(options: LaunchProcessOptions): Promise<Laun
}
}

function sendPosixSIGTERM() {
if (spawnedProcess.pid && !spawnedProcess.killed && !processClosed) {
options.log(`[pid=${spawnedProcess.pid}] <will sigterm>`);
try {
// Send SIGTERM to process tree.
process.kill(-spawnedProcess.pid, 'SIGTERM');
} catch (e) {
// The process might have already stopped.
options.log(`[pid=${spawnedProcess.pid}] exception while trying to SIGTERM process: ${e}`);
}
} else {
options.log(`[pid=${spawnedProcess.pid}] <skipped sigterm spawnedProcess=${spawnedProcess.killed} processClosed=${processClosed}>`);
}
}

function killProcessAndCleanup() {
killProcess();
options.log(`[pid=${spawnedProcess.pid || 'N/A'}] starting temporary directories cleanup`);
Expand All @@ -202,9 +217,16 @@ export async function launchProcess(options: LaunchProcessOptions): Promise<Laun
options.log(`[pid=${spawnedProcess.pid || 'N/A'}] finished temporary directories cleanup`);
}

function killAndWait() {
killProcess();
return waitForCleanup;
async function killAndWait(sendSigtermBeforeSigkillTimeout?: number) {
if (process.platform !== 'win32' && sendSigtermBeforeSigkillTimeout) {
sendPosixSIGTERM();
const sigtermTimeoutId = setTimeout(killProcess, sendSigtermBeforeSigkillTimeout);
await waitForCleanup;
clearTimeout(sigtermTimeoutId);
} else {
killProcess();
await waitForCleanup;
}
}

return { launchedProcess: spawnedProcess, gracefullyClose, kill: killAndWait };
Expand Down
12 changes: 7 additions & 5 deletions packages/playwright-test/src/plugins/webServerPlugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,17 @@ const debugWebServer = debug('pw:webserver');

export class WebServerPlugin implements TestRunnerPlugin {
private _isAvailable?: () => Promise<boolean>;
private _killProcess?: () => Promise<void>;
private _killProcess?: (presendSigtermBeforeSigkillTimeout?: number) => Promise<void>;
private _processExitedPromise!: Promise<any>;
private _options: WebServerPluginOptions;
private _checkPortOnly: boolean;
private _reporter?: Reporter;
private _launchTerminateTimeout: number;
name = 'playwright:webserver';

constructor(options: WebServerPluginOptions, checkPortOnly: boolean) {
this._options = options;
this._launchTerminateTimeout = this._options.timeout || 60 * 1000;
this._checkPortOnly = checkPortOnly;
}

Expand All @@ -72,7 +74,8 @@ export class WebServerPlugin implements TestRunnerPlugin {
}

public async teardown() {
await this._killProcess?.();
// Send SIGTERM and wait for it to gracefully close.
await this._killProcess?.(this._launchTerminateTimeout);
}

private async _startProcess(): Promise<void> {
Expand Down Expand Up @@ -122,15 +125,14 @@ export class WebServerPlugin implements TestRunnerPlugin {
}

private async _waitForAvailability() {
const launchTimeout = this._options.timeout || 60 * 1000;
const cancellationToken = { canceled: false };
const { timedOut } = (await Promise.race([
raceAgainstTimeout(() => waitFor(this._isAvailable!, cancellationToken), launchTimeout),
raceAgainstTimeout(() => waitFor(this._isAvailable!, cancellationToken), this._launchTerminateTimeout),
this._processExitedPromise,
]));
cancellationToken.canceled = true;
if (timedOut)
throw new Error(`Timed out waiting ${launchTimeout}ms from config.webServer.`);
throw new Error(`Timed out waiting ${this._launchTerminateTimeout}ms from config.webServer.`);
}
}

Expand Down
3 changes: 2 additions & 1 deletion packages/playwright-test/types/test.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4599,7 +4599,8 @@ interface TestConfigWebServer {
ignoreHTTPSErrors?: boolean;

/**
* How long to wait for the process to start up and be available in milliseconds. Defaults to 60000.
* How long to wait for the process to start up and be available in milliseconds. The same timeout is also used to
* terminate the process. Defaults to 60000.
*/
timeout?: number;

Expand Down
13 changes: 13 additions & 0 deletions tests/playwright-test/assets/simple-server-ignores-sigterm.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
const http = require('http');

const port = process.argv[2] || 3000;

const server = http.createServer(function (req, res) {
res.end('running!');
});
process.on('SIGTERM', () => console.log('received SIGTERM - ignoring'));
process.on('SIGINT', () => console.log('received SIGINT - ignoring'));

server.listen(port, () => {
console.log('listening on port', port);
});
25 changes: 25 additions & 0 deletions tests/playwright-test/web-server.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import path from 'path';
import { test, expect } from './playwright-test-fixtures';

const SIMPLE_SERVER_PATH = path.join(__dirname, 'assets', 'simple-server.js');
const SIMPLE_SERVER_THAT_IGNORES_SIGTERM_PATH = path.join(__dirname, 'assets', 'simple-server-ignores-sigterm.js');

test('should create a server', async ({ runInlineTest }, { workerIndex }) => {
const port = workerIndex + 10500;
Expand Down Expand Up @@ -601,3 +602,27 @@ test('should treat 3XX as available server', async ({ runInlineTest }, { workerI
expect(result.output).toContain('[WebServer] listening');
expect(result.output).toContain('[WebServer] error from server');
});

test('should be able to kill process that ignores SIGTERM', async ({ runInlineTest }, { workerIndex }) => {
test.skip(process.platform === 'win32', 'there is no SIGTERM on Windows');
const port = workerIndex + 10500;
const result = await runInlineTest({
'test.spec.ts': `
const { test } = pwt;
test('pass', async ({}) => {});
`,
'playwright.config.ts': `
module.exports = {
webServer: {
command: 'node ${JSON.stringify(SIMPLE_SERVER_THAT_IGNORES_SIGTERM_PATH)} ${port}',
port: ${port},
timeout: 1000,
}
};
`,
}, {}, { DEBUG: 'pw:webserver' });
expect(result.exitCode).toBe(0);
expect(result.passed).toBe(1);
expect(result.output).toContain('[WebServer] listening');
expect(result.output).toContain('[WebServer] received SIGTERM - ignoring');
});

0 comments on commit c63a0b5

Please sign in to comment.