From 016daf2fdd5056b29b6f80ab9fbeac55b218e2de Mon Sep 17 00:00:00 2001 From: Asher Date: Wed, 18 Nov 2020 13:01:46 -0600 Subject: [PATCH] Parse arguments once Fixes #2316. --- src/node/entry.ts | 17 +++++++------ src/node/wrapper.ts | 61 ++++++++++++++++++++++++++++++--------------- 2 files changed, 51 insertions(+), 27 deletions(-) diff --git a/src/node/entry.ts b/src/node/entry.ts index 8fcacc0d..9aa69a65 100644 --- a/src/node/entry.ts +++ b/src/node/entry.ts @@ -154,19 +154,22 @@ const main = async (args: DefaultedArgs): Promise => { } async function entry(): Promise { - const cliArgs = parse(process.argv.slice(2)) - const configArgs = await readConfigFile(cliArgs.config) - const args = await setDefaults(cliArgs, configArgs) - // There's no need to check flags like --help or to spawn in an existing // instance for the child process because these would have already happened in - // the parent and the child wouldn't have been spawned. + // the parent and the child wouldn't have been spawned. We also get the + // arguments from the parent so we don't have to parse twice and to account + // for environment manipulation (like how PASSWORD gets removed to avoid + // leaking to child processes). if (isChild(wrapper)) { - await wrapper.handshake() + const args = await wrapper.handshake() wrapper.preventExit() return main(args) } + const cliArgs = parse(process.argv.slice(2)) + const configArgs = await readConfigFile(cliArgs.config) + const args = await setDefaults(cliArgs, configArgs) + if (args.help) { console.log("code-server", version, commit) console.log("") @@ -201,7 +204,7 @@ async function entry(): Promise { return openInExistingInstance(args, socketPath) } - return wrapper.start() + return wrapper.start(args) } entry().catch((error) => { diff --git a/src/node/wrapper.ts b/src/node/wrapper.ts index 5933725e..b63d6057 100644 --- a/src/node/wrapper.ts +++ b/src/node/wrapper.ts @@ -1,8 +1,9 @@ -import { Logger, field, logger } from "@coder/logger" +import { field, Logger, logger } from "@coder/logger" import * as cp from "child_process" import * as path from "path" import * as rfs from "rotating-file-stream" import { Emitter } from "../common/emitter" +import { DefaultedArgs } from "./cli" import { paths } from "./util" const timeoutInterval = 10000 // 10s, matches VS Code's timeouts. @@ -58,7 +59,12 @@ export function onMessage( }) } -interface HandshakeMessage { +interface ParentHandshakeMessage { + type: "handshake" + args: DefaultedArgs +} + +interface ChildHandshakeMessage { type: "handshake" } @@ -67,7 +73,8 @@ interface RelaunchMessage { version: string } -type Message = RelaunchMessage | HandshakeMessage +type ChildMessage = RelaunchMessage | ChildHandshakeMessage +type ParentMessage = ParentHandshakeMessage class ProcessError extends Error { public constructor(message: string, public readonly code: number | undefined) { @@ -164,15 +171,16 @@ class ChildProcess extends Process { /** * Initiate the handshake and wait for a response from the parent. */ - public async handshake(): Promise { + public async handshake(): Promise { this.send({ type: "handshake" }) - await onMessage( + const message = await onMessage( process, - (message): message is HandshakeMessage => { + (message): message is ParentHandshakeMessage => { return message.type === "handshake" }, this.logger, ) + return message.args } /** @@ -185,7 +193,7 @@ class ChildProcess extends Process { /** * Send a message to the parent. */ - private send(message: Message): void { + private send(message: ChildMessage): void { if (!process.send) { throw new Error("not spawned with IPC") } @@ -211,12 +219,19 @@ export class ParentProcess extends Process { private readonly logStdoutStream: rfs.RotatingFileStream private readonly logStderrStream: rfs.RotatingFileStream - protected readonly _onChildMessage = new Emitter() + protected readonly _onChildMessage = new Emitter() protected readonly onChildMessage = this._onChildMessage.event + private args?: DefaultedArgs + public constructor(private currentVersion: string, private readonly options?: WrapperOptions) { super() + process.on("SIGUSR1", async () => { + this.logger.info("Received SIGUSR1; hotswapping") + this.relaunch() + }) + const opts = { size: "10M", maxFiles: 10, @@ -253,21 +268,17 @@ export class ParentProcess extends Process { private async relaunch(): Promise { this.disposeChild() try { - await this.start() + this.started = this._start() + await this.started } catch (error) { this.logger.error(error.message) this.exit(typeof error.code === "number" ? error.code : 1) } } - public start(): Promise { - // If we have a process then we've already bound this. - if (!this.child) { - process.on("SIGUSR1", async () => { - this.logger.info("Received SIGUSR1; hotswapping") - this.relaunch() - }) - } + public start(args: DefaultedArgs): Promise { + // Store for relaunches. + this.args = args if (!this.started) { this.started = this._start() } @@ -320,14 +331,24 @@ export class ParentProcess extends Process { * Wait for a handshake from the child then reply. */ private async handshake(child: cp.ChildProcess): Promise { - await onMessage( + if (!this.args) { + throw new Error("started without args") + } + await onMessage( child, - (message): message is HandshakeMessage => { + (message): message is ChildHandshakeMessage => { return message.type === "handshake" }, this.logger, ) - child.send({ type: "handshake" }) + this.send(child, { type: "handshake", args: this.args }) + } + + /** + * Send a message to the child. + */ + private send(child: cp.ChildProcess, message: ParentMessage): void { + child.send(message) } }