diff --git a/cli/tests/integration/run_tests.rs b/cli/tests/integration/run_tests.rs index 26aacc6fdc..e6ea85da45 100644 --- a/cli/tests/integration/run_tests.rs +++ b/cli/tests/integration/run_tests.rs @@ -3435,6 +3435,11 @@ itest!(test_and_bench_are_noops_in_run { output_str: Some(""), }); +itest!(spawn_kill_permissions { + args: "run --quiet --unstable --allow-run=deno spawn_kill_permissions.ts", + output_str: Some(""), +}); + itest!(followup_dyn_import_resolved { args: "run --unstable --allow-read run/followup_dyn_import_resolves/main.ts", output: "run/followup_dyn_import_resolves/main.ts.out", diff --git a/cli/tests/testdata/spawn_kill_permissions.ts b/cli/tests/testdata/spawn_kill_permissions.ts new file mode 100644 index 0000000000..e0c1b7bfdb --- /dev/null +++ b/cli/tests/testdata/spawn_kill_permissions.ts @@ -0,0 +1,6 @@ +const child = new Deno.Command("deno", { + args: ["eval", "await new Promise(r => setTimeout(r, 2000))"], + stdout: "null", + stderr: "null", +}).spawn(); +child.kill("SIGTERM"); diff --git a/cli/tests/unit/command_test.ts b/cli/tests/unit/command_test.ts index 0763a7ac68..198f94aedb 100644 --- a/cli/tests/unit/command_test.ts +++ b/cli/tests/unit/command_test.ts @@ -867,3 +867,21 @@ Deno.test( } }, ); + +Deno.test( + { permissions: { run: true, read: true } }, + async function commandKillAfterStatus() { + const command = new Deno.Command(Deno.execPath(), { + args: ["help"], + stdout: "null", + stderr: "null", + }); + const child = command.spawn(); + await child.status; + assertThrows( + () => child.kill(), + TypeError, + "Child process has already terminated.", + ); + }, +); diff --git a/runtime/js/40_process.js b/runtime/js/40_process.js index 2a5ac86bf2..664a4b303d 100644 --- a/runtime/js/40_process.js +++ b/runtime/js/40_process.js @@ -200,6 +200,7 @@ function collectOutput(readableStream) { class ChildProcess { #rid; #waitPromiseId; + #waitComplete = false; #unrefed = false; #pid; @@ -268,8 +269,8 @@ class ChildProcess { const waitPromise = core.opAsync("op_spawn_wait", this.#rid); this.#waitPromiseId = waitPromise[promiseIdSymbol]; this.#status = PromisePrototypeThen(waitPromise, (res) => { - this.#rid = null; signal?.[abortSignal.remove](onAbort); + this.#waitComplete = true; return res; }); } @@ -317,10 +318,10 @@ class ChildProcess { } kill(signo = "SIGTERM") { - if (this.#rid === null) { + if (this.#waitComplete) { throw new TypeError("Child process has already terminated."); } - ops.op_kill(this.#pid, signo, "Deno.Child.kill()"); + ops.op_spawn_kill(this.#rid, signo); } ref() { diff --git a/runtime/ops/process.rs b/runtime/ops/process.rs index d991c961f2..76db23d029 100644 --- a/runtime/ops/process.rs +++ b/runtime/ops/process.rs @@ -2,6 +2,7 @@ use super::check_unstable; use crate::permissions::PermissionsContainer; +use deno_core::error::type_error; use deno_core::error::AnyError; use deno_core::op; use deno_core::serde_json; @@ -106,6 +107,7 @@ deno_core::extension!( op_spawn_child, op_spawn_wait, op_spawn_sync, + op_spawn_kill, deprecated::op_run, deprecated::op_run_status, deprecated::op_kill, @@ -115,7 +117,9 @@ deno_core::extension!( }, ); -struct ChildResource(tokio::process::Child); +/// Second member stores the pid separately from the RefCell. It's needed for +/// `op_spawn_kill`, where the RefCell is borrowed mutably by `op_spawn_wait`. +struct ChildResource(RefCell, u32); impl Resource for ChildResource { fn name(&self) -> Cow { @@ -302,7 +306,9 @@ fn spawn_child( .take() .map(|stderr| state.resource_table.add(ChildStderrResource::from(stderr))); - let child_rid = state.resource_table.add(ChildResource(child)); + let child_rid = state + .resource_table + .add(ChildResource(RefCell::new(child), pid)); Ok(Child { rid: child_rid, @@ -328,17 +334,18 @@ async fn op_spawn_wait( state: Rc>, rid: ResourceId, ) -> Result { + #![allow(clippy::await_holding_refcell_ref)] let resource = state .borrow_mut() .resource_table - .take::(rid)?; - Rc::try_unwrap(resource) - .ok() - .unwrap() - .0 - .wait() - .await? - .try_into() + .get::(rid)?; + let result = resource.0.try_borrow_mut()?.wait().await?.try_into(); + state + .borrow_mut() + .resource_table + .close(rid) + .expect("shouldn't have closed until now"); + result } #[op] @@ -366,6 +373,19 @@ fn op_spawn_sync( }) } +#[op] +fn op_spawn_kill( + state: &mut OpState, + rid: ResourceId, + signal: String, +) -> Result<(), AnyError> { + if let Ok(child_resource) = state.resource_table.get::(rid) { + deprecated::kill(child_resource.1 as i32, &signal)?; + return Ok(()); + } + Err(type_error("Child process has already terminated.")) +} + mod deprecated { use super::*;