From 1dd4843b62085a2aeb3134573adf9a7c47c154e2 Mon Sep 17 00:00:00 2001 From: Leo Kettmeir Date: Mon, 28 Nov 2022 12:33:51 +0100 Subject: [PATCH] feat(unstable): rework Deno.Command (#16812) Refactors the `Deno.Command` class to not handle any state, but only being an intermediary to calling its methods, and as such any methods and properties besides `output`, `outputSync` & `spawn` have been removed. Interracting with a `spawn`ed subprocess now works by using the methods and properties on the returned class of the `spawn` method. --- cli/tests/unit/command_test.ts | 45 +++++----- cli/tsc/diagnostics.rs | 1 + cli/tsc/dts/lib.deno.unstable.d.ts | 36 +++++--- runtime/js/40_spawn.js | 136 +++++------------------------ runtime/js/90_deno_ns.js | 1 + runtime/js/99_main.js | 31 +++++++ 6 files changed, 102 insertions(+), 148 deletions(-) diff --git a/cli/tests/unit/command_test.ts b/cli/tests/unit/command_test.ts index 0b8aa934b1..d58053c84c 100644 --- a/cli/tests/unit/command_test.ts +++ b/cli/tests/unit/command_test.ts @@ -31,13 +31,13 @@ tryExit(); Deno.writeFileSync(`${cwd}/${programFile}`, enc.encode(program)); - const child = new Deno.Command(Deno.execPath(), { + const command = new Deno.Command(Deno.execPath(), { cwd, args: ["run", "--allow-read", programFile], stdout: "inherit", stderr: "inherit", }); - child.spawn(); + const child = command.spawn(); // Write the expected exit code *after* starting deno. // This is how we verify that `Child` is actually asynchronous. @@ -55,7 +55,7 @@ tryExit(); Deno.test( { permissions: { run: true, read: true } }, async function commandStdinPiped() { - const child = new Deno.Command(Deno.execPath(), { + const command = new Deno.Command(Deno.execPath(), { args: [ "eval", "if (new TextDecoder().decode(await Deno.readAll(Deno.stdin)) !== 'hello') throw new Error('Expected \\'hello\\'')", @@ -64,7 +64,7 @@ Deno.test( stdout: "null", stderr: "null", }); - child.spawn(); + const child = command.spawn(); assertThrows(() => child.stdout, TypeError, "stdout is not piped"); assertThrows(() => child.stderr, TypeError, "stderr is not piped"); @@ -85,14 +85,14 @@ Deno.test( Deno.test( { permissions: { run: true, read: true } }, async function commandStdoutPiped() { - const child = new Deno.Command(Deno.execPath(), { + const command = new Deno.Command(Deno.execPath(), { args: [ "eval", "await Deno.stdout.write(new TextEncoder().encode('hello'))", ], stderr: "null", }); - child.spawn(); + const child = command.spawn(); assertThrows(() => child.stdin, TypeError, "stdin is not piped"); assertThrows(() => child.stderr, TypeError, "stderr is not piped"); @@ -118,14 +118,14 @@ Deno.test( Deno.test( { permissions: { run: true, read: true } }, async function commandStderrPiped() { - const child = new Deno.Command(Deno.execPath(), { + const command = new Deno.Command(Deno.execPath(), { args: [ "eval", "await Deno.stderr.write(new TextEncoder().encode('hello'))", ], stdout: "null", }); - child.spawn(); + const child = command.spawn(); assertThrows(() => child.stdin, TypeError, "stdin is not piped"); assertThrows(() => child.stdout, TypeError, "stdout is not piped"); @@ -158,13 +158,13 @@ Deno.test( write: true, }); - const child = new Deno.Command(Deno.execPath(), { + const command = new Deno.Command(Deno.execPath(), { args: [ "eval", "Deno.stderr.write(new TextEncoder().encode('error\\n')); Deno.stdout.write(new TextEncoder().encode('output\\n'));", ], }); - child.spawn(); + const child = command.spawn(); await child.stdout.pipeTo(file.writable, { preventClose: true, }); @@ -189,7 +189,7 @@ Deno.test( await Deno.writeFile(fileName, encoder.encode("hello")); const file = await Deno.open(fileName); - const child = new Deno.Command(Deno.execPath(), { + const command = new Deno.Command(Deno.execPath(), { args: [ "eval", "if (new TextDecoder().decode(await Deno.readAll(Deno.stdin)) !== 'hello') throw new Error('Expected \\'hello\\'')", @@ -198,7 +198,7 @@ Deno.test( stdout: "null", stderr: "null", }); - child.spawn(); + const child = command.spawn(); await file.readable.pipeTo(child.stdin, { preventClose: true, }); @@ -212,12 +212,12 @@ Deno.test( Deno.test( { permissions: { run: true, read: true } }, async function commandKillSuccess() { - const child = new Deno.Command(Deno.execPath(), { + const command = new Deno.Command(Deno.execPath(), { args: ["eval", "setTimeout(() => {}, 10000)"], stdout: "null", stderr: "null", }); - child.spawn(); + const child = command.spawn(); child.kill("SIGKILL"); const status = await child.status; @@ -236,12 +236,12 @@ Deno.test( Deno.test( { permissions: { run: true, read: true } }, async function commandKillFailed() { - const child = new Deno.Command(Deno.execPath(), { + const command = new Deno.Command(Deno.execPath(), { args: ["eval", "setTimeout(() => {}, 5000)"], stdout: "null", stderr: "null", }); - child.spawn(); + const child = command.spawn(); assertThrows(() => { // @ts-expect-error testing runtime error of bad signal @@ -255,12 +255,12 @@ Deno.test( Deno.test( { permissions: { run: true, read: true } }, async function commandKillOptional() { - const child = new Deno.Command(Deno.execPath(), { + const command = new Deno.Command(Deno.execPath(), { args: ["eval", "setTimeout(() => {}, 10000)"], stdout: "null", stderr: "null", }); - child.spawn(); + const child = command.spawn(); child.kill(); const status = await child.status; @@ -280,7 +280,7 @@ Deno.test( { permissions: { run: true, read: true } }, async function commandAbort() { const ac = new AbortController(); - const child = new Deno.Command(Deno.execPath(), { + const command = new Deno.Command(Deno.execPath(), { args: [ "eval", "setTimeout(console.log, 1e8)", @@ -289,7 +289,7 @@ Deno.test( stdout: "null", stderr: "null", }); - child.spawn(); + const child = command.spawn(); queueMicrotask(() => ac.abort()); const status = await child.status; assertEquals(status.success, false); @@ -735,11 +735,12 @@ Deno.test( const programFile = "unref.ts"; const program = ` -const child = await new Deno.Command(Deno.execPath(), { +const command = await new Deno.Command(Deno.execPath(), { cwd: Deno.args[0], stdout: "piped", args: ["run", "-A", "--unstable", Deno.args[1]], -});child.spawn(); +}); +const child = command.spawn(); const readable = child.stdout.pipeThrough(new TextDecoderStream()); const reader = readable.getReader(); // set up an interval that will end after reading a few messages from stdout, diff --git a/cli/tsc/diagnostics.rs b/cli/tsc/diagnostics.rs index 05502dca46..4413398c24 100644 --- a/cli/tsc/diagnostics.rs +++ b/cli/tsc/diagnostics.rs @@ -31,6 +31,7 @@ const UNSTABLE_DENO_PROPS: &[&str] = &[ "umask", "spawnChild", "Child", + "ChildProcess", "spawn", "spawnSync", "SpawnOptions", diff --git a/cli/tsc/dts/lib.deno.unstable.d.ts b/cli/tsc/dts/lib.deno.unstable.d.ts index eaa40abc56..ce609736b0 100644 --- a/cli/tsc/dts/lib.deno.unstable.d.ts +++ b/cli/tsc/dts/lib.deno.unstable.d.ts @@ -1639,14 +1639,14 @@ declare namespace Deno { * ], * stdin: "piped", * }); - * command.spawn(); + * const child = command.spawn(); * * // open a file and pipe the subprocess output to it. - * command.stdout.pipeTo(Deno.openSync("output").writable); + * child.stdout.pipeTo(Deno.openSync("output").writable); * * // manually close stdin - * command.stdin.close(); - * const status = await command.status; + * child.stdin.close(); + * const status = await child.status; * ``` * * ```ts @@ -1678,13 +1678,6 @@ declare namespace Deno { * @category Sub Process */ export class Command { - get stdin(): WritableStream; - get stdout(): ReadableStream; - get stderr(): ReadableStream; - readonly pid: number; - /** Get the status of the child process. */ - readonly status: Promise; - constructor(command: string | URL, options?: CommandOptions); /** * Executes the {@linkcode Deno.Command}, waiting for it to finish and @@ -1711,8 +1704,27 @@ declare namespace Deno { /** * Spawns a streamable subprocess, allowing to use the other methods. */ - spawn(): void; + spawn(): ChildProcess; + } + /** **UNSTABLE**: New API, yet to be vetted. + * + * The interface for handling a child process returned from + * {@linkcode Deno.Command.spawn}. + * + * @category Sub Process + */ + export class ChildProcess { + get stdin(): WritableStream; + get stdout(): ReadableStream; + get stderr(): ReadableStream; + readonly pid: number; + /** Get the status of the child. */ + readonly status: Promise; + + /** Waits for the child to exit completely, returning all its output and + * status. */ + output(): Promise; /** Kills the process with given {@linkcode Deno.Signal}. Defaults to * `"SIGTERM"`. */ kill(signo?: Signal): void; diff --git a/runtime/js/40_spawn.js b/runtime/js/40_spawn.js index 8f44c89292..863063e3f6 100644 --- a/runtime/js/40_spawn.js +++ b/runtime/js/40_spawn.js @@ -277,136 +277,44 @@ }; } - class Command { - #command; - #options; + function createCommand(spawn, spawnSync, spawnChild) { + return class Command { + #command; + #options; - #child; + constructor(command, options) { + this.#command = command; + this.#options = options; + } - #consumed; - - constructor(command, options) { - this.#command = command; - this.#options = options; - } - - output() { - if (this.#child) { - return this.#child.output(); - } else { - if (this.#consumed) { - throw new TypeError( - "Command instance is being or has already been consumed.", - ); - } + output() { if (this.#options?.stdin === "piped") { throw new TypeError( "Piped stdin is not supported for this function, use 'Deno.Command.spawn()' instead", ); } - - this.#consumed = true; - return Deno.spawn(this.#command, this.#options); - } - } - - outputSync() { - if (this.#consumed) { - throw new TypeError( - "Command instance is being or has already been consumed.", - ); - } - if (this.#child) { - throw new TypeError("Was spawned"); - } - if (this.#options?.stdin === "piped") { - throw new TypeError( - "Piped stdin is not supported for this function, use 'Deno.Command.spawn()' instead", - ); + return spawn(this.#command, this.#options); } - this.#consumed = true; - return Deno.spawnSync(this.#command, this.#options); - } - - spawn() { - if (this.#consumed) { - throw new TypeError( - "Command instance is being or has already been consumed.", - ); + outputSync() { + if (this.#options?.stdin === "piped") { + throw new TypeError( + "Piped stdin is not supported for this function, use 'Deno.Command.spawn()' instead", + ); + } + return spawnSync(this.#command, this.#options); } - this.#consumed = true; - this.#child = Deno.spawnChild(this.#command, this.#options); - } - - get stdin() { - if (!this.#child) { - throw new TypeError("Wasn't spawned"); + spawn() { + return spawnChild(this.#command, this.#options); } - - return this.#child.stdin; - } - - get stdout() { - if (!this.#child) { - throw new TypeError("Wasn't spawned"); - } - - return this.#child.stdout; - } - - get stderr() { - if (!this.#child) { - throw new TypeError("Wasn't spawned"); - } - - return this.#child.stderr; - } - - get status() { - if (!this.#child) { - throw new TypeError("Wasn't spawned"); - } - - return this.#child.status; - } - - get pid() { - if (!this.#child) { - throw new TypeError("Wasn't spawned"); - } - - return this.#child.pid; - } - - kill(signo = "SIGTERM") { - if (!this.#child) { - throw new TypeError("Wasn't spawned"); - } - this.#child.kill(signo); - } - - ref() { - if (!this.#child) { - throw new TypeError("Wasn't spawned"); - } - - this.#child.ref(); - } - - unref() { - if (!this.#child) { - throw new TypeError("Wasn't spawned"); - } - - this.#child.unref(); - } + }; } window.__bootstrap.spawn = { Child, - Command, + ChildProcess: Child, + createCommand, createSpawn, createSpawnChild, createSpawnSync, diff --git a/runtime/js/90_deno_ns.js b/runtime/js/90_deno_ns.js index cd6c07464c..e3ccf1b6fd 100644 --- a/runtime/js/90_deno_ns.js +++ b/runtime/js/90_deno_ns.js @@ -144,6 +144,7 @@ funlock: __bootstrap.fs.funlock, funlockSync: __bootstrap.fs.funlockSync, Child: __bootstrap.spawn.Child, + ChildProcess: __bootstrap.spawn.ChildProcess, spawnChild: __bootstrap.spawn.spawnChild, spawn: __bootstrap.spawn.spawn, spawnSync: __bootstrap.spawn.spawnSync, diff --git a/runtime/js/99_main.js b/runtime/js/99_main.js index 2ea3504e2c..adfc0d3602 100644 --- a/runtime/js/99_main.js +++ b/runtime/js/99_main.js @@ -482,6 +482,14 @@ delete Intl.v8BreakIterator; }, }); + ObjectAssign(internals.nodeUnstable, { + Command: __bootstrap.spawn.createCommand( + internals.nodeUnstable.spawn, + internals.nodeUnstable.spawnSync, + internals.nodeUnstable.spawnChild, + ), + }); + const finalDenoNs = { core, internal: internalSymbol, @@ -513,6 +521,14 @@ delete Intl.v8BreakIterator; ops.op_net_listen_unixpacket, ), }); + + ObjectAssign(finalDenoNs, { + Command: __bootstrap.spawn.createCommand( + finalDenoNs.spawn, + finalDenoNs.spawnSync, + finalDenoNs.spawnChild, + ), + }); } // Setup `Deno` global - we're actually overriding already existing global @@ -617,6 +633,14 @@ delete Intl.v8BreakIterator; }, }); + ObjectAssign(internals.nodeUnstable, { + Command: __bootstrap.spawn.createCommand( + internals.nodeUnstable.spawn, + internals.nodeUnstable.spawnSync, + internals.nodeUnstable.spawnChild, + ), + }); + const finalDenoNs = { core, internal: internalSymbol, @@ -640,6 +664,13 @@ delete Intl.v8BreakIterator; ops.op_net_listen_unixpacket, ), }); + ObjectAssign(finalDenoNs, { + Command: __bootstrap.spawn.createCommand( + finalDenoNs.spawn, + finalDenoNs.spawnSync, + finalDenoNs.spawnChild, + ), + }); } ObjectDefineProperties(finalDenoNs, { pid: util.readOnly(runtimeOptions.pid),