Skip to content

Commit

Permalink
Revert "feat: send SIGTERM to webserver before SIGKILL'ing it. (#18220)…
Browse files Browse the repository at this point in the history
…" (#18661)

This reverts commit c63a0b5.

Reason: #18564
  • Loading branch information
aslushnikov authored Nov 9, 2022
1 parent a7f1c8c commit 9bcb28f
Show file tree
Hide file tree
Showing 6 changed files with 11 additions and 74 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. The same timeout is also used to terminate the process. Defaults to 60000.
- `timeout` ?<[int]> How long to wait for the process to start up and be available in milliseconds. 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: 4 additions & 26 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: (sendSigtermBeforeSigkillTimeout?: number) => Promise<void>,
kill: () => Promise<void>,
};

export const gracefullyCloseSet = new Set<() => Promise<void>>();
Expand Down Expand Up @@ -188,21 +188,6 @@ 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 @@ -217,16 +202,9 @@ export async function launchProcess(options: LaunchProcessOptions): Promise<Laun
options.log(`[pid=${spawnedProcess.pid || 'N/A'}] finished temporary directories cleanup`);
}

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

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

export class WebServerPlugin implements TestRunnerPlugin {
private _isAvailable?: () => Promise<boolean>;
private _killProcess?: (presendSigtermBeforeSigkillTimeout?: number) => Promise<void>;
private _killProcess?: () => 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 @@ -74,8 +72,7 @@ export class WebServerPlugin implements TestRunnerPlugin {
}

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

private async _startProcess(): Promise<void> {
Expand Down Expand Up @@ -125,14 +122,15 @@ 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), this._launchTerminateTimeout),
raceAgainstTimeout(() => waitFor(this._isAvailable!, cancellationToken), launchTimeout),
this._processExitedPromise,
]));
cancellationToken.canceled = true;
if (timedOut)
throw new Error(`Timed out waiting ${this._launchTerminateTimeout}ms from config.webServer.`);
throw new Error(`Timed out waiting ${launchTimeout}ms from config.webServer.`);
}
}

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

/**
* 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.
* How long to wait for the process to start up and be available in milliseconds. Defaults to 60000.
*/
timeout?: number;

Expand Down
13 changes: 0 additions & 13 deletions tests/playwright-test/assets/simple-server-ignores-sigterm.js

This file was deleted.

25 changes: 0 additions & 25 deletions tests/playwright-test/web-server.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ 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 @@ -602,27 +601,3 @@ 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 9bcb28f

Please sign in to comment.