From 6bdaada689dc9e78c461e4158310d99faa54347f Mon Sep 17 00:00:00 2001 From: Asher Date: Mon, 14 Sep 2020 15:56:08 -0500 Subject: [PATCH 01/10] Move uncaught exception handler to wrapper Feels more appropriate there to me. --- src/node/entry.ts | 7 ------- src/node/wrapper.ts | 8 ++++++++ 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/node/entry.ts b/src/node/entry.ts index 5869ae461..dfd78cda7 100644 --- a/src/node/entry.ts +++ b/src/node/entry.ts @@ -18,13 +18,6 @@ import { loadPlugins } from "./plugin" import { generateCertificate, hash, humanPath, open } from "./util" import { ipcMain, wrap } from "./wrapper" -process.on("uncaughtException", (error) => { - logger.error(`Uncaught exception: ${error.message}`) - if (typeof error.stack !== "undefined") { - logger.error(error.stack) - } -}) - let pkg: { version?: string; commit?: string } = {} try { pkg = require("../../package.json") diff --git a/src/node/wrapper.ts b/src/node/wrapper.ts index ba459efd1..2f1eb037f 100644 --- a/src/node/wrapper.ts +++ b/src/node/wrapper.ts @@ -254,6 +254,14 @@ if (!process.stdout.isTTY) { process.stdout.on("error", () => ipcMain().exit()) } +// Don't let uncaught exceptions crash the process. +process.on("uncaughtException", (error) => { + logger.error(`Uncaught exception: ${error.message}`) + if (typeof error.stack !== "undefined") { + logger.error(error.stack) + } +}) + export const wrap = (fn: () => Promise): void => { if (ipcMain().parentPid) { ipcMain() From 0a8e71c6474ed59f3979c68005288f75c29bcd10 Mon Sep 17 00:00:00 2001 From: Asher Date: Mon, 14 Sep 2020 15:57:58 -0500 Subject: [PATCH 02/10] Refactor wrapper - Immediately create ipcMain so it doesn't have to be a function which I think feels cleaner. - Move exit handling to a separate function to compensate (otherwise the VS Code CLI for example won't be able to exit on its own). - New isChild prop that is clearer than checking for parentPid (IMO). - Skip all the checks that aren't necessary for the child process (like --help, --version, etc). - Since we check if we're the child in entry go ahead and move the wrap code into entry as well since that's basically what it does. - Use a single catch at the end of the entry. - Split out the VS Code CLI and existing instance code into separate functions. --- src/node/cli.ts | 20 ++++ src/node/entry.ts | 231 ++++++++++++++++++++++++++------------------ src/node/wrapper.ts | 102 +++++++++---------- 3 files changed, 199 insertions(+), 154 deletions(-) diff --git a/src/node/cli.ts b/src/node/cli.ts index b723417d1..9683945d4 100644 --- a/src/node/cli.ts +++ b/src/node/cli.ts @@ -496,3 +496,23 @@ async function copyOldMacOSDataDir(): Promise { await fs.copy(oldDataDir, paths.data) } } + +export const shouldRunVsCodeCli = (args: Args): boolean => { + return !!args["list-extensions"] || !!args["install-extension"] || !!args["uninstall-extension"] +} + +/** + * Determine if it looks like the user is trying to open a file or folder in an + * existing instance. The arguments here should be the arguments the user + * explicitly passed on the command line, not defaults or the configuration. + */ +export const shouldOpenInExistingInstance = async (args: Args): Promise => { + // Always use the existing instance if we're running from VS Code's terminal. + if (process.env.VSCODE_IPC_HOOK_CLI) { + return process.env.VSCODE_IPC_HOOK_CLI + } + + // TODO: implement + + return undefined +} diff --git a/src/node/entry.ts b/src/node/entry.ts index dfd78cda7..0d16c250d 100644 --- a/src/node/entry.ts +++ b/src/node/entry.ts @@ -11,12 +11,21 @@ import { ProxyHttpProvider } from "./app/proxy" import { StaticHttpProvider } from "./app/static" import { UpdateHttpProvider } from "./app/update" import { VscodeHttpProvider } from "./app/vscode" -import { Args, bindAddrFromAllSources, optionDescriptions, parse, readConfigFile, setDefaults } from "./cli" +import { + Args, + bindAddrFromAllSources, + optionDescriptions, + parse, + readConfigFile, + setDefaults, + shouldOpenInExistingInstance, + shouldRunVsCodeCli, +} from "./cli" import { coderCloudBind } from "./coder-cloud" import { AuthType, HttpServer, HttpServerOptions } from "./http" import { loadPlugins } from "./plugin" import { generateCertificate, hash, humanPath, open } from "./util" -import { ipcMain, wrap } from "./wrapper" +import { ipcMain, WrapperProcess } from "./wrapper" let pkg: { version?: string; commit?: string } = {} try { @@ -28,6 +37,86 @@ try { const version = pkg.version || "development" const commit = pkg.commit || "development" +export const runVsCodeCli = (args: Args): void => { + logger.debug("forking vs code cli...") + const vscode = cp.fork(path.resolve(__dirname, "../../lib/vscode/out/vs/server/fork"), [], { + env: { + ...process.env, + CODE_SERVER_PARENT_PID: process.pid.toString(), + }, + }) + vscode.once("message", (message: any) => { + logger.debug("got message from VS Code", field("message", message)) + if (message.type !== "ready") { + logger.error("Unexpected response waiting for ready response", field("type", message.type)) + process.exit(1) + } + const send: CliMessage = { type: "cli", args } + vscode.send(send) + }) + vscode.once("error", (error) => { + logger.error("Got error from VS Code", field("error", error)) + process.exit(1) + }) + vscode.on("exit", (code) => process.exit(code || 0)) +} + +export const openInExistingInstance = async (args: Args, socketPath: string): Promise => { + const pipeArgs: OpenCommandPipeArgs & { fileURIs: string[] } = { + type: "open", + folderURIs: [], + fileURIs: [], + forceReuseWindow: args["reuse-window"], + forceNewWindow: args["new-window"], + } + + const isDir = async (path: string): Promise => { + try { + const st = await fs.stat(path) + return st.isDirectory() + } catch (error) { + return false + } + } + + for (let i = 0; i < args._.length; i++) { + const fp = path.resolve(args._[i]) + if (await isDir(fp)) { + pipeArgs.folderURIs.push(fp) + } else { + pipeArgs.fileURIs.push(fp) + } + } + + if (pipeArgs.forceNewWindow && pipeArgs.fileURIs.length > 0) { + logger.error("--new-window can only be used with folder paths") + process.exit(1) + } + + if (pipeArgs.folderURIs.length === 0 && pipeArgs.fileURIs.length === 0) { + logger.error("Please specify at least one file or folder") + process.exit(1) + } + + const vscode = http.request( + { + path: "/", + method: "POST", + socketPath, + }, + (response) => { + response.on("data", (message) => { + logger.debug("got message from VS Code", field("message", message.toString())) + }) + }, + ) + vscode.on("error", (error: unknown) => { + logger.error("got error from VS Code", field("error", error)) + }) + vscode.write(JSON.stringify(pipeArgs)) + vscode.end() +} + const main = async (args: Args, configArgs: Args): Promise => { if (args.link) { // If we're being exposed to the cloud, we listen on a random address and disable auth. @@ -92,7 +181,7 @@ const main = async (args: Args, configArgs: Args): Promise => { await loadPlugins(httpServer, args) - ipcMain().onDispose(() => { + ipcMain.onDispose(() => { httpServer.dispose().then((errors) => { errors.forEach((error) => logger.error(error.message)) }) @@ -132,7 +221,9 @@ const main = async (args: Args, configArgs: Args): Promise => { if (serverAddress && !options.socket && args.open) { // The web socket doesn't seem to work if browsing with 0.0.0.0. const openAddress = serverAddress.replace(/:\/\/0.0.0.0/, "://localhost") - await open(openAddress).catch(console.error) + await open(openAddress).catch((error: Error) => { + logger.error("Failed to open", field("address", openAddress), field("error", error)) + }) logger.info(`Opened ${openAddress}`) } @@ -141,27 +232,32 @@ const main = async (args: Args, configArgs: Args): Promise => { await coderCloudBind(serverAddress!, args.link.value) } catch (err) { logger.error(err.message) - ipcMain().exit(1) + ipcMain.exit(1) } } } async function entry(): Promise { - const tryParse = async (): Promise<[Args, Args]> => { - try { - const cliArgs = parse(process.argv.slice(2)) - const configArgs = await readConfigFile(cliArgs.config) - // This prioritizes the flags set in args over the ones in the config file. - let args = Object.assign(configArgs, cliArgs) - args = await setDefaults(args) - return [args, configArgs] - } catch (error) { - console.error(error.message) - process.exit(1) - } + const tryParse = async (): Promise<[Args, Args, Args]> => { + const cliArgs = parse(process.argv.slice(2)) + const configArgs = await readConfigFile(cliArgs.config) + // This prioritizes the flags set in args over the ones in the config file. + let args = Object.assign(configArgs, cliArgs) + args = await setDefaults(args) + return [args, cliArgs, configArgs] + } + + const [args, cliArgs, configArgs] = await tryParse() + + // 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. + if (ipcMain.isChild) { + await ipcMain.handshake() + ipcMain.preventExit() + return main(args, configArgs) } - const [args, configArgs] = await tryParse() if (args.help) { console.log("code-server", version, commit) console.log("") @@ -171,7 +267,10 @@ async function entry(): Promise { optionDescriptions().forEach((description) => { console.log("", description) }) - } else if (args.version) { + return + } + + if (args.version) { if (args.json) { console.log({ codeServer: version, @@ -181,83 +280,23 @@ async function entry(): Promise { } else { console.log(version, commit) } - process.exit(0) - } else if (args["list-extensions"] || args["install-extension"] || args["uninstall-extension"]) { - logger.debug("forking vs code cli...") - const vscode = cp.fork(path.resolve(__dirname, "../../lib/vscode/out/vs/server/fork"), [], { - env: { - ...process.env, - CODE_SERVER_PARENT_PID: process.pid.toString(), - }, - }) - vscode.once("message", (message: any) => { - logger.debug("Got message from VS Code", field("message", message)) - if (message.type !== "ready") { - logger.error("Unexpected response waiting for ready response") - process.exit(1) - } - const send: CliMessage = { type: "cli", args } - vscode.send(send) - }) - vscode.once("error", (error) => { - logger.error(error.message) - process.exit(1) - }) - vscode.on("exit", (code) => process.exit(code || 0)) - } else if (process.env.VSCODE_IPC_HOOK_CLI) { - const pipeArgs: OpenCommandPipeArgs = { - type: "open", - folderURIs: [], - forceReuseWindow: args["reuse-window"], - forceNewWindow: args["new-window"], - } - const isDir = async (path: string): Promise => { - try { - const st = await fs.stat(path) - return st.isDirectory() - } catch (error) { - return false - } - } - for (let i = 0; i < args._.length; i++) { - const fp = path.resolve(args._[i]) - if (await isDir(fp)) { - pipeArgs.folderURIs.push(fp) - } else { - if (!pipeArgs.fileURIs) { - pipeArgs.fileURIs = [] - } - pipeArgs.fileURIs.push(fp) - } - } - if (pipeArgs.forceNewWindow && pipeArgs.fileURIs && pipeArgs.fileURIs.length > 0) { - logger.error("new-window can only be used with folder paths") - process.exit(1) - } - if (pipeArgs.folderURIs.length === 0 && (!pipeArgs.fileURIs || pipeArgs.fileURIs.length === 0)) { - logger.error("Please specify at least one file or folder argument") - process.exit(1) - } - const vscode = http.request( - { - path: "/", - method: "POST", - socketPath: process.env["VSCODE_IPC_HOOK_CLI"], - }, - (res) => { - res.on("data", (message) => { - logger.debug("Got message from VS Code", field("message", message.toString())) - }) - }, - ) - vscode.on("error", (err) => { - logger.debug("Got error from VS Code", field("error", err)) - }) - vscode.write(JSON.stringify(pipeArgs)) - vscode.end() - } else { - wrap(() => main(args, configArgs)) + return } + + if (shouldRunVsCodeCli(args)) { + return runVsCodeCli(args) + } + + const socketPath = await shouldOpenInExistingInstance(cliArgs) + if (socketPath) { + return openInExistingInstance(args, socketPath) + } + + const wrapper = new WrapperProcess(require("../../package.json").version) + return wrapper.start() } -entry() +entry().catch((error) => { + logger.error(error.message) + ipcMain.exit(error) +}) diff --git a/src/node/wrapper.ts b/src/node/wrapper.ts index 2f1eb037f..ea29eb28a 100644 --- a/src/node/wrapper.ts +++ b/src/node/wrapper.ts @@ -32,19 +32,13 @@ export class IpcMain { public readonly onMessage = this._onMessage.event private readonly _onDispose = new Emitter() public readonly onDispose = this._onDispose.event - public readonly processExit: (code?: number) => never + public readonly processExit: (code?: number) => never = process.exit - public constructor(public readonly parentPid?: number) { + public constructor(private readonly parentPid?: number) { process.on("SIGINT", () => this._onDispose.emit("SIGINT")) process.on("SIGTERM", () => this._onDispose.emit("SIGTERM")) process.on("exit", () => this._onDispose.emit(undefined)) - // Ensure we control when the process exits. - this.processExit = process.exit - process.exit = function (code?: number) { - logger.warn(`process.exit() was prevented: ${code || "unknown code"}.`) - } as (code?: number) => never - this.onDispose((signal) => { // Remove listeners to avoid possibly triggering disposal again. process.removeAllListeners() @@ -71,6 +65,19 @@ export class IpcMain { } } + /** + * Ensure we control when the process exits. + */ + public preventExit(): void { + process.exit = function (code?: number) { + logger.warn(`process.exit() was prevented: ${code || "unknown code"}.`) + } as (code?: number) => never + } + + public get isChild(): boolean { + return typeof this.parentPid !== "undefined" + } + public exit(error?: number | ProcessError): never { if (error && typeof error !== "number") { this.processExit(typeof error.code === "number" ? error.code : 1) @@ -127,17 +134,12 @@ export class IpcMain { } } -let _ipcMain: IpcMain -export const ipcMain = (): IpcMain => { - if (!_ipcMain) { - _ipcMain = new IpcMain( - typeof process.env.CODE_SERVER_PARENT_PID !== "undefined" - ? parseInt(process.env.CODE_SERVER_PARENT_PID) - : undefined, - ) - } - return _ipcMain -} +/** + * Channel for communication between the child and parent processes. + */ +export const ipcMain = new IpcMain( + typeof process.env.CODE_SERVER_PARENT_PID !== "undefined" ? parseInt(process.env.CODE_SERVER_PARENT_PID) : undefined, +) export interface WrapperOptions { maxMemory?: number @@ -162,14 +164,11 @@ export class WrapperProcess { this.logStdoutStream = rfs.createStream(path.join(paths.data, "coder-logs", "code-server-stdout.log"), opts) this.logStderrStream = rfs.createStream(path.join(paths.data, "coder-logs", "code-server-stderr.log"), opts) - ipcMain().onDispose(() => { - if (this.process) { - this.process.removeAllListeners() - this.process.kill() - } + ipcMain.onDispose(() => { + this.disposeChild() }) - ipcMain().onMessage((message) => { + ipcMain.onMessage((message) => { switch (message.type) { case "relaunch": logger.info(`Relaunching: ${this.currentVersion} -> ${message.version}`) @@ -181,28 +180,35 @@ export class WrapperProcess { break } }) - - process.on("SIGUSR1", async () => { - logger.info("Received SIGUSR1; hotswapping") - this.relaunch() - }) } - private async relaunch(): Promise { + private disposeChild(): void { this.started = undefined if (this.process) { this.process.removeAllListeners() this.process.kill() } + } + + private async relaunch(): Promise { + this.disposeChild() try { await this.start() } catch (error) { logger.error(error.message) - ipcMain().exit(typeof error.code === "number" ? error.code : 1) + ipcMain.exit(typeof error.code === "number" ? error.code : 1) } } public start(): Promise { + // If we have a process then we've already bound this. + if (!this.process) { + process.on("SIGUSR1", async () => { + logger.info("Received SIGUSR1; hotswapping") + this.relaunch() + }) + } + if (!this.started) { this.started = this.spawn().then((child) => { // Log both to stdout and to the log directory. @@ -215,14 +221,12 @@ export class WrapperProcess { child.stderr.pipe(process.stderr) } logger.debug(`spawned inner process ${child.pid}`) - ipcMain() - .handshake(child) - .then(() => { - child.once("exit", (code) => { - logger.debug(`inner process ${child.pid} exited unexpectedly`) - ipcMain().exit(code || 0) - }) + ipcMain.handshake(child).then(() => { + child.once("exit", (code) => { + logger.debug(`inner process ${child.pid} exited unexpectedly`) + ipcMain.exit(code || 0) }) + }) this.process = child }) } @@ -251,7 +255,7 @@ export class WrapperProcess { // It's possible that the pipe has closed (for example if you run code-server // --version | head -1). Assume that means we're done. if (!process.stdout.isTTY) { - process.stdout.on("error", () => ipcMain().exit()) + process.stdout.on("error", () => ipcMain.exit()) } // Don't let uncaught exceptions crash the process. @@ -261,21 +265,3 @@ process.on("uncaughtException", (error) => { logger.error(error.stack) } }) - -export const wrap = (fn: () => Promise): void => { - if (ipcMain().parentPid) { - ipcMain() - .handshake() - .then(() => fn()) - .catch((error: ProcessError): void => { - logger.error(error.message) - ipcMain().exit(error) - }) - } else { - const wrapper = new WrapperProcess(require("../../package.json").version) - wrapper.start().catch((error) => { - logger.error(error.message) - ipcMain().exit(error) - }) - } -} From bb1bf88439738eeacca246703fcf05f4a3182037 Mon Sep 17 00:00:00 2001 From: Asher Date: Mon, 14 Sep 2020 17:22:24 -0500 Subject: [PATCH 03/10] Fix wrapper.start not actually waiting for anything --- src/node/wrapper.ts | 47 +++++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 21 deletions(-) diff --git a/src/node/wrapper.ts b/src/node/wrapper.ts index ea29eb28a..cce841901 100644 --- a/src/node/wrapper.ts +++ b/src/node/wrapper.ts @@ -208,32 +208,37 @@ export class WrapperProcess { this.relaunch() }) } - if (!this.started) { - this.started = this.spawn().then((child) => { - // Log both to stdout and to the log directory. - if (child.stdout) { - child.stdout.pipe(this.logStdoutStream) - child.stdout.pipe(process.stdout) - } - if (child.stderr) { - child.stderr.pipe(this.logStderrStream) - child.stderr.pipe(process.stderr) - } - logger.debug(`spawned inner process ${child.pid}`) - ipcMain.handshake(child).then(() => { - child.once("exit", (code) => { - logger.debug(`inner process ${child.pid} exited unexpectedly`) - ipcMain.exit(code || 0) - }) - }) - this.process = child - }) + this.started = this._start() } return this.started } - private async spawn(): Promise { + private async _start(): Promise { + const child = this.spawn() + this.process = child + + // Log both to stdout and to the log directory. + if (child.stdout) { + child.stdout.pipe(this.logStdoutStream) + child.stdout.pipe(process.stdout) + } + if (child.stderr) { + child.stderr.pipe(this.logStderrStream) + child.stderr.pipe(process.stderr) + } + + logger.debug(`spawned inner process ${child.pid}`) + + await ipcMain.handshake(child) + + child.once("exit", (code) => { + logger.debug(`inner process ${child.pid} exited unexpectedly`) + ipcMain.exit(code || 0) + }) + } + + private spawn(): cp.ChildProcess { // Flags to pass along to the Node binary. let nodeOptions = `${process.env.NODE_OPTIONS || ""} ${(this.options && this.options.nodeOptions) || ""}` if (!/max_old_space_size=(\d+)/g.exec(nodeOptions)) { From 19022967024ac690cc5e0e7ea11ade0f05792318 Mon Sep 17 00:00:00 2001 From: Asher Date: Tue, 15 Sep 2020 12:47:33 -0500 Subject: [PATCH 04/10] Remove references to --open-in flag --- src/node/cli.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node/cli.ts b/src/node/cli.ts index 9683945d4..3d4d0659c 100644 --- a/src/node/cli.ts +++ b/src/node/cli.ts @@ -152,12 +152,12 @@ const options: Options> = { "new-window": { type: "boolean", short: "n", - description: "Force to open a new window. (use with open-in)", + description: "Force to open a new window.", }, "reuse-window": { type: "boolean", short: "r", - description: "Force to open a file or folder in an already opened window. (use with open-in)", + description: "Force to open a file or folder in an already opened window.", }, locale: { type: "string" }, From 021c084e4315f6fbc40ca0b9feed1cf0930d43d7 Mon Sep 17 00:00:00 2001 From: Asher Date: Tue, 15 Sep 2020 13:43:07 -0500 Subject: [PATCH 05/10] Move log level defaults into setDefaults This will allow cliArgs to be only the actual arguments the user passed which will be used for some logic around opening in existing instances. --- src/node/cli.ts | 30 ++++++++++----------- test/cli.test.ts | 69 ++++++++++++++++++++++++++++++++---------------- 2 files changed, 61 insertions(+), 38 deletions(-) diff --git a/src/node/cli.ts b/src/node/cli.ts index 3d4d0659c..a52796a14 100644 --- a/src/node/cli.ts +++ b/src/node/cli.ts @@ -327,6 +327,21 @@ export const parse = ( logger.debug("parsed command line", field("args", args)) + return args +} + +export async function setDefaults(args: Args): Promise { + args = { ...args } + + if (!args["user-data-dir"]) { + await copyOldMacOSDataDir() + args["user-data-dir"] = paths.data + } + + if (!args["extensions-dir"]) { + args["extensions-dir"] = path.join(args["user-data-dir"], "extensions") + } + // --verbose takes priority over --log and --log takes priority over the // environment variable. if (args.verbose) { @@ -369,21 +384,6 @@ export const parse = ( return args } -export async function setDefaults(args: Args): Promise { - args = { ...args } - - if (!args["user-data-dir"]) { - await copyOldMacOSDataDir() - args["user-data-dir"] = paths.data - } - - if (!args["extensions-dir"]) { - args["extensions-dir"] = path.join(args["user-data-dir"], "extensions") - } - - return args -} - async function defaultConfigFile(): Promise { return `bind-addr: 127.0.0.1:8080 auth: password diff --git a/test/cli.test.ts b/test/cli.test.ts index f4f6c8849..fe78659d8 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -1,20 +1,23 @@ -import { logger, Level } from "@coder/logger" +import { Level, logger } from "@coder/logger" import * as assert from "assert" import * as path from "path" -import { parse } from "../src/node/cli" +import { parse, setDefaults } from "../src/node/cli" +import { paths } from "../src/node/util" describe("cli", () => { beforeEach(() => { delete process.env.LOG_LEVEL }) - // The parser will always fill these out. + // The parser should not set any defaults so the caller can determine what + // values the user actually set. These are set after calling `setDefaults`. const defaults = { - _: [], + "extensions-dir": path.join(paths.data, "extensions"), + "user-data-dir": paths.data, } it("should set defaults", () => { - assert.deepEqual(parse([]), defaults) + assert.deepEqual(parse([]), { _: [] }) }) it("should parse all available options", () => { @@ -69,7 +72,7 @@ describe("cli", () => { help: true, host: "0.0.0.0", json: true, - log: "trace", + log: "error", open: true, port: 8081, socket: path.resolve("mumble"), @@ -83,19 +86,20 @@ describe("cli", () => { it("should work with short options", () => { assert.deepEqual(parse(["-vvv", "-v"]), { - ...defaults, - log: "trace", + _: [], verbose: true, version: true, }) - assert.equal(process.env.LOG_LEVEL, "trace") - assert.equal(logger.level, Level.Trace) }) - it("should use log level env var", () => { + it("should use log level env var", async () => { + const args = parse([]) + assert.deepEqual(args, { _: [] }) + process.env.LOG_LEVEL = "debug" - assert.deepEqual(parse([]), { + assert.deepEqual(await setDefaults(args), { ...defaults, + _: [], log: "debug", verbose: false, }) @@ -103,8 +107,9 @@ describe("cli", () => { assert.equal(logger.level, Level.Debug) process.env.LOG_LEVEL = "trace" - assert.deepEqual(parse([]), { + assert.deepEqual(await setDefaults(args), { ...defaults, + _: [], log: "trace", verbose: true, }) @@ -113,9 +118,16 @@ describe("cli", () => { }) it("should prefer --log to env var and --verbose to --log", async () => { + let args = parse(["--log", "info"]) + assert.deepEqual(args, { + _: [], + log: "info", + }) + process.env.LOG_LEVEL = "debug" - assert.deepEqual(parse(["--log", "info"]), { + assert.deepEqual(await setDefaults(args), { ...defaults, + _: [], log: "info", verbose: false, }) @@ -123,17 +135,26 @@ describe("cli", () => { assert.equal(logger.level, Level.Info) process.env.LOG_LEVEL = "trace" - assert.deepEqual(parse(["--log", "info"]), { + assert.deepEqual(await setDefaults(args), { ...defaults, + _: [], log: "info", verbose: false, }) assert.equal(process.env.LOG_LEVEL, "info") assert.equal(logger.level, Level.Info) + args = parse(["--log", "info", "--verbose"]) + assert.deepEqual(args, { + _: [], + log: "info", + verbose: true, + }) + process.env.LOG_LEVEL = "warn" - assert.deepEqual(parse(["--log", "info", "--verbose"]), { + assert.deepEqual(await setDefaults(args), { ...defaults, + _: [], log: "trace", verbose: true, }) @@ -141,9 +162,12 @@ describe("cli", () => { assert.equal(logger.level, Level.Trace) }) - it("should ignore invalid log level env var", () => { + it("should ignore invalid log level env var", async () => { process.env.LOG_LEVEL = "bogus" - assert.deepEqual(parse([]), defaults) + assert.deepEqual(await setDefaults(parse([])), { + _: [], + ...defaults, + }) }) it("should error if value isn't provided", () => { @@ -166,7 +190,7 @@ describe("cli", () => { it("should not error if the value is optional", () => { assert.deepEqual(parse(["--cert"]), { - ...defaults, + _: [], cert: { value: undefined, }, @@ -177,7 +201,7 @@ describe("cli", () => { assert.throws(() => parse(["--socket", "--socket-path-value"]), /--socket requires a value/) // If you actually had a path like this you would do this instead: assert.deepEqual(parse(["--socket", "./--socket-path-value"]), { - ...defaults, + _: [], socket: path.resolve("--socket-path-value"), }) assert.throws(() => parse(["--cert", "--socket-path-value"]), /Unknown option --socket-path-value/) @@ -185,7 +209,6 @@ describe("cli", () => { it("should allow positional arguments before options", () => { assert.deepEqual(parse(["foo", "test", "--auth", "none"]), { - ...defaults, _: ["foo", "test"], auth: "none", }) @@ -193,11 +216,11 @@ describe("cli", () => { it("should support repeatable flags", () => { assert.deepEqual(parse(["--proxy-domain", "*.coder.com"]), { - ...defaults, + _: [], "proxy-domain": ["*.coder.com"], }) assert.deepEqual(parse(["--proxy-domain", "*.coder.com", "--proxy-domain", "test.com"]), { - ...defaults, + _: [], "proxy-domain": ["*.coder.com", "test.com"], }) }) From fe19391c036bf234e84b2e30f321de73f3b689e2 Mon Sep 17 00:00:00 2001 From: Asher Date: Tue, 15 Sep 2020 16:51:43 -0500 Subject: [PATCH 06/10] Read most recent socket path from file --- ci/dev/vscode.patch | 25 +++++++++++++ src/node/cli.ts | 32 +++++++++++++++- src/node/socket.ts | 13 +------ src/node/util.ts | 15 ++++++++ test/cli.test.ts | 89 +++++++++++++++++++++++++++++++++++++++++++-- 5 files changed, 156 insertions(+), 18 deletions(-) diff --git a/ci/dev/vscode.patch b/ci/dev/vscode.patch index b3a7289df..f15c7d7a7 100644 --- a/ci/dev/vscode.patch +++ b/ci/dev/vscode.patch @@ -3035,6 +3035,31 @@ index b3c89e51cfc25a53293a352a2a8ad50d5f26d595..e21abe4e13bc25a5b72f556bbfb61085 registerSingleton(IExtHostTerminalService, ExtHostTerminalService); registerSingleton(IExtHostTunnelService, ExtHostTunnelService); +registerSingleton(IExtHostNodeProxy, class extends NotImplementedProxy(String(IExtHostNodeProxy)) { whenReady = Promise.resolve(); }); +diff --git a/src/vs/workbench/api/node/extHostCLIServer.ts b/src/vs/workbench/api/node/extHostCLIServer.ts +index 7cae126cc0f804273850933468690e0f9f10a5b8..08c2aa5cdae3f3d06bb08b7055dc7e7def260132 100644 +--- a/src/vs/workbench/api/node/extHostCLIServer.ts ++++ b/src/vs/workbench/api/node/extHostCLIServer.ts +@@ -11,6 +11,8 @@ import { IWindowOpenable, IOpenWindowOptions } from 'vs/platform/windows/common/ + import { URI } from 'vs/base/common/uri'; + import { hasWorkspaceFileExtension } from 'vs/platform/workspaces/common/workspaces'; + import { ILogService } from 'vs/platform/log/common/log'; ++import { join } from 'vs/base/common/path'; ++import { tmpdir } from 'os'; + + export interface OpenCommandPipeArgs { + type: 'open'; +@@ -54,6 +56,11 @@ export class CLIServer { + private async setup(): Promise { + this._ipcHandlePath = generateRandomPipeName(); + ++ // NOTE@coder: Write this out so we can get the most recent path. ++ fs.promises.writeFile(join(tmpdir(), "vscode-ipc"), this._ipcHandlePath).catch((error) => { ++ this.logService.error(error); ++ }); ++ + try { + this._server.listen(this.ipcHandlePath); + this._server.on('error', err => this.logService.error(err)); diff --git a/src/vs/workbench/api/worker/extHost.worker.services.ts b/src/vs/workbench/api/worker/extHost.worker.services.ts index 3843fdec386edc09a1d361b63de892a04e0070ed..8aac4df527857e964798362a69f5591bef07c165 100644 --- a/src/vs/workbench/api/worker/extHost.worker.services.ts diff --git a/src/node/cli.ts b/src/node/cli.ts index a52796a14..028ea0d45 100644 --- a/src/node/cli.ts +++ b/src/node/cli.ts @@ -5,7 +5,7 @@ import * as os from "os" import * as path from "path" import { Args as VsArgs } from "../../lib/vscode/src/vs/server/ipc" import { AuthType } from "./http" -import { generatePassword, humanPath, paths } from "./util" +import { canConnect, generatePassword, humanPath, paths } from "./util" export class Optional { public constructor(public readonly value?: T) {} @@ -512,7 +512,35 @@ export const shouldOpenInExistingInstance = async (args: Args): Promise => { + try { + return await fs.readFile(path.join(os.tmpdir(), "vscode-ipc"), "utf8") + } catch (error) { + if (error.code !== "ENOENT") { + throw error + } + } + return undefined + } + + // If these flags are set then assume the user is trying to open in an + // existing instance since these flags have no effect otherwise. + const openInFlagCount = ["reuse-window", "new-window"].reduce((prev, cur) => { + return args[cur as keyof Args] ? prev + 1 : prev + }, 0) + if (openInFlagCount > 0) { + return readSocketPath() + } + + // It's possible the user is trying to spawn another instance of code-server. + // Check if any unrelated flags are set (add one for `_` which always exists), + // that a file or directory was passed, and that the socket is active. + if (Object.keys(args).length === openInFlagCount + 1 && args._.length > 0) { + const socketPath = await readSocketPath() + if (socketPath && (await canConnect(socketPath))) { + return socketPath + } + } return undefined } diff --git a/src/node/socket.ts b/src/node/socket.ts index e5fe66778..ada024831 100644 --- a/src/node/socket.ts +++ b/src/node/socket.ts @@ -4,7 +4,7 @@ import * as path from "path" import * as tls from "tls" import { Emitter } from "../common/emitter" import { generateUuid } from "../common/util" -import { tmpdir } from "./util" +import { canConnect, tmpdir } from "./util" /** * Provides a way to proxy a TLS socket. Can be used when you need to pass a @@ -89,17 +89,6 @@ export class SocketProxyProvider { } public async findFreeSocketPath(basePath: string, maxTries = 100): Promise { - const canConnect = (path: string): Promise => { - return new Promise((resolve) => { - const socket = net.connect(path) - socket.once("error", () => resolve(false)) - socket.once("connect", () => { - socket.destroy() - resolve(true) - }) - }) - } - let i = 0 let path = basePath while ((await canConnect(path)) && i < maxTries) { diff --git a/src/node/util.ts b/src/node/util.ts index c0f37f74b..75122fe76 100644 --- a/src/node/util.ts +++ b/src/node/util.ts @@ -2,6 +2,7 @@ import * as cp from "child_process" import * as crypto from "crypto" import envPaths from "env-paths" import * as fs from "fs-extra" +import * as net from "net" import * as os from "os" import * as path from "path" import * as util from "util" @@ -246,3 +247,17 @@ export function pathToFsPath(path: string, keepDriveLetterCasing = false): strin } return value } + +/** + * Return a promise that resolves with whether the socket path is active. + */ +export function canConnect(path: string): Promise { + return new Promise((resolve) => { + const socket = net.connect(path) + socket.once("error", () => resolve(false)) + socket.once("connect", () => { + socket.destroy() + resolve(true) + }) + }) +} diff --git a/test/cli.test.ts b/test/cli.test.ts index fe78659d8..ae5256142 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -1,16 +1,24 @@ import { Level, logger } from "@coder/logger" import * as assert from "assert" +import * as fs from "fs-extra" +import * as net from "net" +import * as os from "os" import * as path from "path" -import { parse, setDefaults } from "../src/node/cli" -import { paths } from "../src/node/util" +import { Args, parse, setDefaults, shouldOpenInExistingInstance } from "../src/node/cli" +import { paths, tmpdir } from "../src/node/util" -describe("cli", () => { +type Mutable = { + -readonly [P in keyof T]: T[P] +} + +describe("parser", () => { beforeEach(() => { delete process.env.LOG_LEVEL }) // The parser should not set any defaults so the caller can determine what - // values the user actually set. These are set after calling `setDefaults`. + // values the user actually set. These are only set after explicitly calling + // `setDefaults`. const defaults = { "extensions-dir": path.join(paths.data, "extensions"), "user-data-dir": paths.data, @@ -225,3 +233,76 @@ describe("cli", () => { }) }) }) + +describe("cli", () => { + let args: Mutable = { _: [] } + const testDir = path.join(tmpdir, "tests/cli") + const vscodeIpcPath = path.join(os.tmpdir(), "vscode-ipc") + + before(async () => { + await fs.remove(testDir) + await fs.mkdirp(testDir) + }) + + beforeEach(async () => { + delete process.env.VSCODE_IPC_HOOK_CLI + args = { _: [] } + await fs.remove(vscodeIpcPath) + }) + + it("should use existing if inside code-server", async () => { + process.env.VSCODE_IPC_HOOK_CLI = "test" + assert.strictEqual(await shouldOpenInExistingInstance(args), "test") + + args.port = 8081 + args._.push("./file") + assert.strictEqual(await shouldOpenInExistingInstance(args), "test") + }) + + it("should use existing if --reuse-window is set", async () => { + args["reuse-window"] = true + assert.strictEqual(await shouldOpenInExistingInstance(args), undefined) + + await fs.writeFile(vscodeIpcPath, "test") + assert.strictEqual(await shouldOpenInExistingInstance(args), "test") + + args.port = 8081 + assert.strictEqual(await shouldOpenInExistingInstance(args), "test") + }) + + it("should use existing if --new-window is set", async () => { + args["new-window"] = true + assert.strictEqual(await shouldOpenInExistingInstance(args), undefined) + + await fs.writeFile(vscodeIpcPath, "test") + assert.strictEqual(await shouldOpenInExistingInstance(args), "test") + + args.port = 8081 + assert.strictEqual(await shouldOpenInExistingInstance(args), "test") + }) + + it("should use existing if no unrelated flags are set, has positional, and socket is active", async () => { + assert.strictEqual(await shouldOpenInExistingInstance(args), undefined) + + args._.push("./file") + assert.strictEqual(await shouldOpenInExistingInstance(args), undefined) + + const socketPath = path.join(testDir, "socket") + await fs.writeFile(vscodeIpcPath, socketPath) + assert.strictEqual(await shouldOpenInExistingInstance(args), undefined) + + await new Promise((resolve) => { + const server = net.createServer(() => { + // Close after getting the first connection. + server.close() + }) + server.once("listening", () => resolve(server)) + server.listen(socketPath) + }) + + assert.strictEqual(await shouldOpenInExistingInstance(args), socketPath) + + args.port = 8081 + assert.strictEqual(await shouldOpenInExistingInstance(args), undefined) + }) +}) From e0769dc13a83653a10125eda19aec6b33c6f8601 Mon Sep 17 00:00:00 2001 From: Asher Date: Tue, 15 Sep 2020 17:29:53 -0500 Subject: [PATCH 07/10] Move config file info log Otherwise it outputs when trying to open a file in an existing instance externally. Externally there isn't an environment variable to branch on to skip this line so instead output it with the other info lines in the child process. --- src/node/cli.ts | 4 ---- src/node/entry.ts | 2 ++ 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/node/cli.ts b/src/node/cli.ts index 028ea0d45..92bf39cb8 100644 --- a/src/node/cli.ts +++ b/src/node/cli.ts @@ -410,10 +410,6 @@ export async function readConfigFile(configPath?: string): Promise { logger.info(`Wrote default config file to ${humanPath(configPath)}`) } - if (!process.env.CODE_SERVER_PARENT_PID && !process.env.VSCODE_IPC_HOOK_CLI) { - logger.info(`Using config file ${humanPath(configPath)}`) - } - const configFile = await fs.readFile(configPath) const config = yaml.safeLoad(configFile.toString(), { filename: configPath, diff --git a/src/node/entry.ts b/src/node/entry.ts index 0d16c250d..cec2a13cf 100644 --- a/src/node/entry.ts +++ b/src/node/entry.ts @@ -188,6 +188,8 @@ const main = async (args: Args, configArgs: Args): Promise => { }) logger.info(`code-server ${version} ${commit}`) + logger.info(`Using config file ${humanPath(args.config)}`) + const serverAddress = await httpServer.listen() logger.info(`HTTP server listening on ${serverAddress}`) From 466a04f8741ccada3f5e0e2c2a38c60ed57ee4a6 Mon Sep 17 00:00:00 2001 From: Asher Date: Wed, 7 Oct 2020 13:03:39 -0500 Subject: [PATCH 08/10] Remove pointless use of openInFlagCount It'll always be zero here. --- src/node/cli.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/node/cli.ts b/src/node/cli.ts index 92bf39cb8..1403d8920 100644 --- a/src/node/cli.ts +++ b/src/node/cli.ts @@ -529,9 +529,10 @@ export const shouldOpenInExistingInstance = async (args: Args): Promise 0) { + // Check if any unrelated flags are set (check against one because `_` always + // exists), that a file or directory was passed, and that the socket is + // active. + if (Object.keys(args).length === 1 && args._.length > 0) { const socketPath = await readSocketPath() if (socketPath && (await canConnect(socketPath))) { return socketPath From 26c735b4342674efbfd4003a17f99f3ff4c6ac3a Mon Sep 17 00:00:00 2001 From: Asher Date: Fri, 9 Oct 2020 17:05:21 -0500 Subject: [PATCH 09/10] Remove tryParse Now that the exception handling happens further up there doesn't seem to be an advantage in having this in a separate method. --- src/node/entry.ts | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/node/entry.ts b/src/node/entry.ts index cec2a13cf..96db046e2 100644 --- a/src/node/entry.ts +++ b/src/node/entry.ts @@ -240,16 +240,11 @@ const main = async (args: Args, configArgs: Args): Promise => { } async function entry(): Promise { - const tryParse = async (): Promise<[Args, Args, Args]> => { - const cliArgs = parse(process.argv.slice(2)) - const configArgs = await readConfigFile(cliArgs.config) - // This prioritizes the flags set in args over the ones in the config file. - let args = Object.assign(configArgs, cliArgs) - args = await setDefaults(args) - return [args, cliArgs, configArgs] - } - - const [args, cliArgs, configArgs] = await tryParse() + const cliArgs = parse(process.argv.slice(2)) + const configArgs = await readConfigFile(cliArgs.config) + // This prioritizes the flags set in args over the ones in the config file. + let args = Object.assign(configArgs, cliArgs) + args = await setDefaults(args) // 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 From d7e31126253afa0fd4b6b913318a57e8506e6aab Mon Sep 17 00:00:00 2001 From: Asher Date: Fri, 9 Oct 2020 18:01:43 -0500 Subject: [PATCH 10/10] Update standalone test --- ci/build/test-standalone-release.sh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ci/build/test-standalone-release.sh b/ci/build/test-standalone-release.sh index 92b58e8c0..0344ea39f 100755 --- a/ci/build/test-standalone-release.sh +++ b/ci/build/test-standalone-release.sh @@ -15,8 +15,7 @@ main() { ./release-standalone/bin/code-server --extensions-dir "$EXTENSIONS_DIR" --install-extension ms-python.python local installed_extensions installed_extensions="$(./release-standalone/bin/code-server --extensions-dir "$EXTENSIONS_DIR" --list-extensions 2>&1)" - if [[ $installed_extensions != *"info Using config file ~/.config/code-server/config.yaml -ms-python.python" ]]; then + if [[ $installed_extensions != "ms-python.python" ]]; then echo "Unexpected output from listing extensions:" echo "$installed_extensions" exit 1