-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(webserver): customize shutdown with new kill
option
#34130
base: main
Are you sure you want to change the base?
Changes from all commits
18f35f8
8283a09
184ca67
093df3a
9480b7c
8c341d1
1fb0491
a156cfe
970fad0
02284ed
5ae52f0
bf30120
5c89e57
8c7b02e
14c8ec3
4c881cf
220ed29
5bf5fcf
5b576ab
b311f0c
c6cf6bc
a30a3bb
6d1dfe1
907bfd2
5555d17
71bce85
6c85be4
caa941a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -180,7 +180,7 @@ export async function launchProcess(options: LaunchProcessOptions): Promise<Laun | |
let processClosed = false; | ||
let fulfillCleanup = () => {}; | ||
const waitForCleanup = new Promise<void>(f => fulfillCleanup = f); | ||
spawnedProcess.once('exit', (exitCode, signal) => { | ||
spawnedProcess.once('close', (exitCode, signal) => { | ||
options.log(`[pid=${spawnedProcess.pid}] <process did exit: exitCode=${exitCode}, signal=${signal}>`); | ||
processClosed = true; | ||
gracefullyCloseSet.delete(gracefullyClose); | ||
|
@@ -226,7 +226,7 @@ export async function launchProcess(options: LaunchProcessOptions): Promise<Laun | |
killSet.delete(killProcessAndCleanup); | ||
removeProcessHandlersIfNeeded(); | ||
options.log(`[pid=${spawnedProcess.pid}] <kill>`); | ||
if (spawnedProcess.pid && !spawnedProcess.killed && !processClosed) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously we wouldn't send the signal again if it has already been received, I believe it shouldn't matter for SIGKILL handling, but would be nice to double check. |
||
if (spawnedProcess.pid && !processClosed) { | ||
options.log(`[pid=${spawnedProcess.pid}] <will force kill>`); | ||
// Force kill the browser. | ||
try { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,7 @@ export type WebServerPluginOptions = { | |
url?: string; | ||
ignoreHTTPSErrors?: boolean; | ||
timeout?: number; | ||
kill?: { SIGINT?: number, SIGTERM?: number }; | ||
reuseExistingServer?: boolean; | ||
cwd?: string; | ||
env?: { [key: string]: string; }; | ||
|
@@ -91,8 +92,23 @@ export class WebServerPlugin implements TestRunnerPlugin { | |
throw new Error(`${this._options.url ?? `http://localhost${port ? ':' + port : ''}`} is already used, make sure that nothing is running on the port/url or set reuseExistingServer:true in config.webServer.`); | ||
} | ||
|
||
let signal: 'SIGINT' | 'SIGTERM' | undefined = undefined; | ||
let timeout = 0; | ||
if (this._options.kill) { | ||
if (typeof this._options.kill.SIGINT === 'number') { | ||
signal = 'SIGINT'; | ||
timeout = this._options.kill.SIGINT; | ||
} | ||
if (typeof this._options.kill.SIGTERM === 'number') { | ||
if (signal) | ||
throw new Error('Only one of SIGINT or SIGTERM can be specified in config.webServer.kill'); | ||
signal = 'SIGTERM'; | ||
timeout = this._options.kill.SIGTERM; | ||
} | ||
} | ||
|
||
debugWebServer(`Starting WebServer process ${this._options.command}...`); | ||
const { launchedProcess, kill } = await launchProcess({ | ||
const { launchedProcess, gracefullyClose } = await launchProcess({ | ||
command: this._options.command, | ||
env: { | ||
...DEFAULT_ENVIRONMENT_VARIABLES, | ||
|
@@ -102,14 +118,31 @@ export class WebServerPlugin implements TestRunnerPlugin { | |
cwd: this._options.cwd, | ||
stdio: 'stdin', | ||
shell: true, | ||
// Reject to indicate that we cannot close the web server gracefully | ||
// and should fallback to non-graceful shutdown. | ||
attemptToGracefullyClose: () => Promise.reject(), | ||
attemptToGracefullyClose: async () => { | ||
if (process.platform === 'win32') | ||
throw new Error('Graceful shutdown is not supported on Windows'); | ||
if (!signal) | ||
throw new Error('skip graceful shutdown'); | ||
|
||
// proper usage of SIGINT is to send it to the entire process group, see https://www.cons.org/cracauer/sigint.html | ||
// there's no such convention for SIGTERM, so we decide what we want. signaling the process group for consistency. | ||
process.kill(-launchedProcess.pid!, signal); | ||
|
||
return new Promise<void>((resolve, reject) => { | ||
const timer = timeout !== 0 | ||
? setTimeout(() => reject(new Error(`process didn't close gracefully within timeout, falling back to SIGKILL`)), timeout) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Drop 'falling back to SIGKILL' as it makes an assumption about the caller and how it handles errors. We already print |
||
: undefined; | ||
launchedProcess.once('close', (...args) => { | ||
clearTimeout(timer); | ||
resolve(); | ||
}); | ||
}); | ||
}, | ||
log: () => {}, | ||
onExit: code => processExitedReject(new Error(code ? `Process from config.webServer was not able to start. Exit code: ${code}` : 'Process from config.webServer exited early.')), | ||
tempDirectories: [], | ||
}); | ||
this._killProcess = kill; | ||
this._killProcess = gracefullyClose; | ||
|
||
debugWebServer(`Process started`); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd split it into 2 fields
{ signal: SIGINT', timeout 5000 }
to make it clear that only one signal can be specified and what the value semantics is.