Auto merge of #18281 - darichey:async-subprocess, r=lnicola

Run subprocesses async in vscode extension

Extensions should not block the vscode extension host. Replace uses of `spawnSync` with `spawnAsync`, a convenience wrapper around `spawn`.

These `spawnSync`s are unlikely to cause a real issue in practice, because they spawn very short-lived processes, so we aren't blocking for very long. That said, blocking the extension host is poor practice, and if they _do_ block for too long for whatever reason, vscode becomes useless.

Author: bors

editors/code/src/bootstrap.ts

@@ -1,9 +1,9 @@
 import * as vscode from "vscode";
 import * as os from "os";
 import type { Config } from "./config";
-import { type Env, log } from "./util";
+import { type Env, log, spawnAsync } from "./util";
 import type { PersistentState } from "./persistent_state";
-import { exec, spawnSync } from "child_process";
+import { exec } from "child_process";
 import { TextDecoder } from "node:util";
 
 export async function bootstrap(
@@ -61,13 +61,12 @@ async function getServer(
             // if so, use the rust-analyzer component
             const toolchainUri = vscode.Uri.joinPath(workspaceFolder.uri, "rust-toolchain.toml");
             if (await hasToolchainFileWithRaDeclared(toolchainUri)) {
-                const res = spawnSync("rustup", ["which", "rust-analyzer"], {
-                    encoding: "utf8",
+                const res = await spawnAsync("rustup", ["which", "rust-analyzer"], {
                     env: { ...process.env },
                     cwd: workspaceFolder.uri.fsPath,
                 });
                 if (!res.error && res.status === 0) {
-                    toolchainServerPath = earliestToolchainPath(
+                    toolchainServerPath = await earliestToolchainPath(
                         toolchainServerPath,
                         res.stdout.trim(),
                         raVersionResolver,
@@ -114,10 +113,8 @@ async function getServer(
 }
 
 // Given a path to a rust-analyzer executable, resolve its version and return it.
-function raVersionResolver(path: string): string | undefined {
-    const res = spawnSync(path, ["--version"], {
-        encoding: "utf8",
-    });
+async function raVersionResolver(path: string): Promise<string | undefined> {
+    const res = await spawnAsync(path, ["--version"]);
     if (!res.error && res.status === 0) {
         return res.stdout;
     } else {
@@ -126,13 +123,16 @@ function raVersionResolver(path: string): string | undefined {
 }
 
 // Given a path to two rust-analyzer executables, return the earliest one by date.
-function earliestToolchainPath(
+async function earliestToolchainPath(
     path0: string | undefined,
     path1: string,
-    raVersionResolver: (path: string) => string | undefined,
-): string {
+    raVersionResolver: (path: string) => Promise<string | undefined>,
+): Promise<string> {
     if (path0) {
-        if (orderFromPath(path0, raVersionResolver) < orderFromPath(path1, raVersionResolver)) {
+        if (
+            (await orderFromPath(path0, raVersionResolver)) <
+            (await orderFromPath(path1, raVersionResolver))
+        ) {
             return path0;
         } else {
             return path1;
@@ -150,11 +150,11 @@ function earliestToolchainPath(
 //  nightly   - /Users/myuser/.rustup/toolchains/nightly-2022-11-22-aarch64-apple-darwin/bin/rust-analyzer
 //  versioned - /Users/myuser/.rustup/toolchains/1.72.1-aarch64-apple-darwin/bin/rust-analyzer
 //  stable    - /Users/myuser/.rustup/toolchains/stable-aarch64-apple-darwin/bin/rust-analyzer
-function orderFromPath(
+async function orderFromPath(
     path: string,
-    raVersionResolver: (path: string) => string | undefined,
-): string {
-    const raVersion = raVersionResolver(path);
+    raVersionResolver: (path: string) => Promise<string | undefined>,
+): Promise<string> {
+    const raVersion = await raVersionResolver(path);
     const raDate = raVersion?.match(/^rust-analyzer .*\(.* (\d{4}-\d{2}-\d{2})\)$/);
     if (raDate?.length === 2) {
         const precedence = path.includes("nightly-") ? "0" : "1";
@@ -184,11 +184,10 @@ async function hasToolchainFileWithRaDeclared(uri: vscode.Uri): Promise<boolean>
     }
 }
 
-export function isValidExecutable(path: string, extraEnv: Env): boolean {
+export async function isValidExecutable(path: string, extraEnv: Env): Promise<boolean> {
     log.debug("Checking availability of a binary at", path);
 
-    const res = spawnSync(path, ["--version"], {
-        encoding: "utf8",
+    const res = await spawnAsync(path, ["--version"], {
         env: { ...process.env, ...extraEnv },
     });
 

editors/code/src/util.ts

@@ -1,6 +1,6 @@
 import * as vscode from "vscode";
 import { strict as nativeAssert } from "assert";
-import { exec, type ExecOptions } from "child_process";
+import { exec, spawn, type SpawnOptionsWithoutStdio, type ExecOptions } from "child_process";
 import { inspect } from "util";
 import type { CargoRunnableArgs, ShellRunnableArgs } from "./lsp_ext";
 
@@ -233,3 +233,55 @@ export function expectNotUndefined<T>(input: Undefinable<T>, msg: string): NotUn
 export function unwrapUndefinable<T>(input: Undefinable<T>): NotUndefined<T> {
     return expectNotUndefined(input, `unwrapping \`undefined\``);
 }
+
+interface SpawnAsyncReturns {
+    stdout: string;
+    stderr: string;
+    status: number | null;
+    error?: Error | undefined;
+}
+
+export async function spawnAsync(
+    path: string,
+    args?: ReadonlyArray<string>,
+    options?: SpawnOptionsWithoutStdio,
+): Promise<SpawnAsyncReturns> {
+    const child = spawn(path, args, options);
+    const stdout: Array<Buffer> = [];
+    const stderr: Array<Buffer> = [];
+    try {
+        const res = await new Promise<{ stdout: string; stderr: string; status: number | null }>(
+            (resolve, reject) => {
+                child.stdout.on("data", (chunk) => stdout.push(Buffer.from(chunk)));
+                child.stderr.on("data", (chunk) => stderr.push(Buffer.from(chunk)));
+                child.on("error", (error) =>
+                    reject({
+                        stdout: Buffer.concat(stdout).toString("utf8"),
+                        stderr: Buffer.concat(stderr).toString("utf8"),
+                        error,
+                    }),
+                );
+                child.on("close", (status) =>
+                    resolve({
+                        stdout: Buffer.concat(stdout).toString("utf8"),
+                        stderr: Buffer.concat(stderr).toString("utf8"),
+                        status,
+                    }),
+                );
+            },
+        );
+
+        return {
+            stdout: res.stdout,
+            stderr: res.stderr,
+            status: res.status,
+        };
+    } catch (e: any) {
+        return {
+            stdout: e.stdout,
+            stderr: e.stderr,
+            status: e.status,
+            error: e.error,
+        };
+    }
+}

editors/code/tests/unit/bootstrap.test.ts

@@ -6,9 +6,9 @@ export async function getTests(ctx: Context) {
     await ctx.suite("Bootstrap/Select toolchain RA", (suite) => {
         suite.addTest("Order of nightly RA", async () => {
             assert.deepStrictEqual(
-                _private.orderFromPath(
+                await _private.orderFromPath(
                     "/Users/myuser/.rustup/toolchains/nightly-2022-11-22-aarch64-apple-darwin/bin/rust-analyzer",
-                    function (path: string) {
+                    async function (path: string) {
                         assert.deepStrictEqual(
                             path,
                             "/Users/myuser/.rustup/toolchains/nightly-2022-11-22-aarch64-apple-darwin/bin/rust-analyzer",
@@ -22,9 +22,9 @@ export async function getTests(ctx: Context) {
 
         suite.addTest("Order of versioned RA", async () => {
             assert.deepStrictEqual(
-                _private.orderFromPath(
+                await _private.orderFromPath(
                     "/Users/myuser/.rustup/toolchains/1.72.1-aarch64-apple-darwin/bin/rust-analyzer",
-                    function (path: string) {
+                    async function (path: string) {
                         assert.deepStrictEqual(
                             path,
                             "/Users/myuser/.rustup/toolchains/1.72.1-aarch64-apple-darwin/bin/rust-analyzer",
@@ -38,9 +38,9 @@ export async function getTests(ctx: Context) {
 
         suite.addTest("Order of versioned RA when unable to obtain version date", async () => {
             assert.deepStrictEqual(
-                _private.orderFromPath(
+                await _private.orderFromPath(
                     "/Users/myuser/.rustup/toolchains/1.72.1-aarch64-apple-darwin/bin/rust-analyzer",
-                    function () {
+                    async function () {
                         return "rust-analyzer 1.72.1";
                     },
                 ),
@@ -50,9 +50,9 @@ export async function getTests(ctx: Context) {
 
         suite.addTest("Order of stable RA", async () => {
             assert.deepStrictEqual(
-                _private.orderFromPath(
+                await _private.orderFromPath(
                     "/Users/myuser/.rustup/toolchains/stable-aarch64-apple-darwin/bin/rust-analyzer",
-                    function (path: string) {
+                    async function (path: string) {
                         assert.deepStrictEqual(
                             path,
                             "/Users/myuser/.rustup/toolchains/stable-aarch64-apple-darwin/bin/rust-analyzer",
@@ -66,7 +66,7 @@ export async function getTests(ctx: Context) {
 
         suite.addTest("Order with invalid path to RA", async () => {
             assert.deepStrictEqual(
-                _private.orderFromPath("some-weird-path", function () {
+                await _private.orderFromPath("some-weird-path", async function () {
                     return undefined;
                 }),
                 "2",
@@ -75,10 +75,10 @@ export async function getTests(ctx: Context) {
 
         suite.addTest("Earliest RA between nightly and stable", async () => {
             assert.deepStrictEqual(
-                _private.earliestToolchainPath(
+                await _private.earliestToolchainPath(
                     "/Users/myuser/.rustup/toolchains/stable-aarch64-apple-darwin/bin/rust-analyzer",
                     "/Users/myuser/.rustup/toolchains/nightly-2022-11-22-aarch64-apple-darwin/bin/rust-analyzer",
-                    function (path: string) {
+                    async function (path: string) {
                         if (
                             path ===
                             "/Users/myuser/.rustup/toolchains/nightly-2022-11-22-aarch64-apple-darwin/bin/rust-analyzer"