Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(process) support AbortSignal in run #11579

Closed
wants to merge 3 commits into from
Closed

feat(process) support AbortSignal in run #11579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac30b4b
feat(process) support AbortSignal in run
benjamingr Aug 4, 2021
9006d06
fixup
benjamingr Aug 4, 2021
abd07d1
clean leak to save time
benjamingr Aug 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions cli/dts/lib.deno.ns.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2020,6 +2020,11 @@ declare namespace Deno {
stdout?: "inherit" | "piped" | "null" | number;
stderr?: "inherit" | "piped" | "null" | number;
stdin?: "inherit" | "piped" | "null" | number;
/**
* An AbortSignal that allows closing the process using the corresponding
* AbortController by sending the process a SIGTERM signal.
*/
signal?: AbortSignal;
}

/** Spawns new subprocess. RunOptions must contain at a minimum the `opt.cmd`,
Expand Down
21 changes: 21 additions & 0 deletions cli/tests/unit/process_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,3 +510,24 @@ unitTest({ perms: { run: true, read: true } }, function killFailed(): void {

p.close();
});

unitTest(
{ perms: { run: true, read: true } },
async function runAbortSignal(): Promise<void> {
const ac = new AbortController();
const p = Deno.run({
cmd: [
Deno.execPath(),
"eval",
"setTimeout(console.log, 1e8)",
],
signal: ac.signal,
});
queueMicrotask(() => ac.abort());
const status = await p.status();
assertEquals(status.success, false);
assertEquals(status.code, 143);
assertEquals(status.signal, Deno.Signal.SIGTERM);
p.close();
},
);
13 changes: 12 additions & 1 deletion runtime/js/40_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@
stdout = "inherit",
stderr = "inherit",
stdin = "inherit",
signal,
}) {
if (cmd[0] != null) {
cmd[0] = pathFromURL(cmd[0]);
Expand All @@ -119,7 +120,17 @@
stdoutRid: isRid(stdout) ? stdout : 0,
stderrRid: isRid(stderr) ? stderr : 0,
});
return new Process(res);
const p = new Process(res);
if (signal) {
const stopper = () => {
p.kill(Deno.Signal.SIGTERM);
};
Comment on lines +125 to +127
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Process.kill is an unstable API (https://doc.deno.land/builtin/unstable#Deno.kill) so we can't add it like this to stable Deno.run. Also I'm a bit worried about bifurcation that sending SIGTERM could be done with two different APIs.

We need to discuss this change more in-depth before merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bartlomieju that is acceptable to me, I am in no rush to make these changes. The idea is to add a web-standard-capability way to cancel operations so that you use Deno APIs and web APIs similarly and can abort your sub-process, event handlers and fetch with the same AbortSignal.

I am happy to be part of that discussion though I am also fine with you having this discussion without me and just letting me know.

Note that adding this API to Node.js involved some discussion - the major points are:

  • Would users be confused by the parameter being called signal since in non-web-standards land signal means something else for processes? (Node decided to prefer web-standards)
  • Would users be confused by this sending a signal to the process? Which signal should it send? (In node that is configurable as killSignal and whatever stops the process elsewhere works here).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Also the technical limitation (process.kill being unstable) seems like the smaller concern here and what API users should get seems like the much bigger one. I am happy to refactor this PR to use whatever machinary .kill is using directly in order to bypass the unstability of .kill it's not a big deal)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benjamingr sorry for late reply, I missed your comments.

that is acceptable to me, I am in no rush to make these changes. The idea is to add a web-standard-capability way to cancel operations so that you use Deno APIs and web APIs similarly and can abort your sub-process, event handlers and fetch with the same AbortSignal.

This is a good argument.

I agree that signal might be a confusing name, my first thought when I saw it was OS signal - let's bikeshed on this.

(Also the technical limitation (process.kill being unstable) seems like the smaller concern here and what API users should get seems like the much bigger one. I am happy to refactor this PR to use whatever machinary .kill is using directly in order to bypass the unstability of .kill it's not a big deal)

Actually looking at it I don't think there are any blockers to stabilize this API so that would allows us to move forward with this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with the name signal is that the web standards require:

Any web platform API using promises to represent operations that can be aborted must adhere to the following:

 - Accept AbortSignal objects through a signal dictionary member.
 - Convey that the operation got aborted by rejecting the promise with an "AbortError" DOMException.
 - Reject immediately if the AbortSignal's aborted flag is already set, otherwise:
 - Use the abort algorithms mechanism to observe changes to the AbortSignal object and do so in a manner that does not lead to clashes with other observers.

Note that this isn't actually a web API so Deno doesn't have to use the same web standard conventions but it is probably preferable to do so if web-familiarity and compatibility is a goal. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I'm not sure, it's quite confusing, I will confer with other team members on this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, let me know :)

signal.addEventListener("abort", stopper);
p.status().then(() => {
signal.removeEventListener("abort", stopper);
});
}
return p;
}

window.__bootstrap.process = {
Expand Down