feat(process) support AbortSignal in run

[benjamingr]

Support AbortSignal in Deno.run

This continues the work in #10943 and adds AbortSignal support to Deno.run. Closes #11574

cc @lucacasonato @bartlomieju

[bartlomieju]

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.

[benjamingr]

@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).

[benjamingr]

(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)

[bartlomieju]

@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.

[benjamingr]

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?

[bartlomieju]

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

[benjamingr]

Thanks, let me know :)

[crowlKats]

It might make more sense to not add any new features to Deno.run since there is the plan to soon deprecate it with the new subprocess API, and just directly implement it there

[sant123]

See #11016

[crowlKats]

And #11618

[CLAassistant]

All committers have signed the CLA.

[benjamingr]

I think the CLA bot is broken?

[bartlomieju]

I think the CLA bot is broken?

Ryan changed email assigned to CLA bot so it's requesting signing from everyone again 😅

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.