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.

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

Support AbortSignals in async API #31971

Closed
martinheidegger opened this issue Feb 26, 2020 · 26 comments
Closed

Support AbortSignals in async API #31971

martinheidegger opened this issue Feb 26, 2020 · 26 comments
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@martinheidegger
Copy link

AbortSignals are used in fetch to abort a request. They are useful to close requests if they don't happen to be necessary. AbortSignals are not widely used yet in the JavaScript ecosystem but they seem like they could be useful if they would find a wider adoption.

A node.js API that comes to mind is if a web framework would provide an abort signal that this could be comfortable to be used as a means to abort a file stream:

createReadStream('file.bin', { signal: req.signal }).pipe(res)

Other places this might be if there was an abort signal for the process that could be used instead of process.on('SIGTERM', ...), ex.:

await readFile('file.bin', { signal: process.abortSignal })

Which could be an easy means to stop allocating money if a CLI tool is used to read a very large file by accident.

At the current point I am not sure at how many places this would be useful, or how useful it would be at each case, but I think opening the discussion has merit.

@joyeecheung joyeecheung added the feature request Issues that request new features to be added to Node.js. label Feb 26, 2020
@bnoordhuis
Copy link
Member

I can see how a generalized cancellation mechanism would be useful; that's essentially what require('domain') tried to do (albeit with flaws.)

Probably the best way to get the conversation going is by opening a pull request that adds AbortSignal support in a few places, let's say net.connect(), http.get() and fs.createReadStream().

The exact set of methods doesn't really matter as long as it's diverse enough to get a feel for the necessary changes.


process.abortSignal - can you explain why you suggest a global AbortSignal? It doesn't seem useful, it's a one-shot thing, right?

@devsnek
Copy link
Member

devsnek commented Feb 27, 2020

There is has been work in TC39 to provide cancellation and cleanup protocols within the language, which is why I've avoided pushing too much on this. Using the web API might be fine, I just want to make sure we don't feel forced to pick that specific API.

@martinheidegger
Copy link
Author

I have been experimenting a bit recently with abort signals and the usefulness it could provide (it is pretty hard actually). A few interesting examples are listed below:

can you explain why you suggest a global AbortSignal? It doesn't seem useful, it's a one-shot thing, right?

In the app I also made use of a "home-made", process-abort-signal here:

https://github.com/consento-org/dat-ssg/blob/6bc7007fe6d109f3535497300370caa9ffff5d3f/lib/abort.js#L61-L65

It serves the purpose that I can use the same "abort" logic that is used in a generic fashion also in a cli-context:

https://github.com/consento-org/dat-ssg/blob/6bc7007fe6d109f3535497300370caa9ffff5d3f/bin/dat-ssg#L23

Resulting in a CLI tool that allows graceful abortion of sub-processes in any given situation.

@martinheidegger
Copy link
Author

@devsnek What work of the TC39 are you referring to?

@devsnek
Copy link
Member

devsnek commented Mar 11, 2020

@martinheidegger for example https://github.com/tc39/proposal-cancellation

@ptomato
Copy link

ptomato commented Apr 3, 2020

Hi, I'd like to work on this!

Probably the best way to get the conversation going is by opening a pull request that adds AbortSignal support in a few places, let's say net.connect(), http.get() and fs.createReadStream().

That sounds good, I'll start out with fs.createReadStream() as local file operations would seem the easiest place to work out initial problems, then work on the other two.

@jimmywarting
Copy link

fyi, whatwg streams use AbortSignal also - not just only fetch.

pipeThrough({ writable, readable }, { preventClose, preventAbort, preventCancel, signal } = {})
pipeTo(dest, { preventClose, preventAbort, preventCancel, signal } = {})

anyhow - maybe we should focus on EventTarget first?

@mcollina
Copy link
Member

mcollina commented May 6, 2020

I'd be in support of adding a cancellable primitive to some base operations such as readFile or writeFile and other callback-based APIs. I kindly ask to have it support both EventTarget and EventEmitter, as Node.js is EventEmitter based.

Is EventTarget needed at all?


I'd need to know more about adding it to streams and http.

Note that:

createReadStream('file.bin', { signal: req.signal }).pipe(res)

Is extremely far from what anybody that serve files with Node.js core would do. Check out https://github.com/pillarjs/send to know more (at the time of this writing, it's 1129 lines of code).

The snipped above works only because req and res are connected and they would be destroyed at the same time. If we stream to another location, it will leak a file descriptor and memory:

createReadStream('file.bin', { signal: req.signal }).pipe(uploadToSomewhereElse)

This API needs to be designed correctly to be able to work with on streams. As an example, this would be able to tear down the full pipe on abort:

createReadStream('file.bin').pipe(uploadToSomewhereElse, { signal: req.signal })

Note that all the above is already solved in Node.js with pipeline(), which supports async generators and iterators as well. It's based on the standardization of the stream.destroy(err) operation which is effectively the "abort mechanism" for streams, so the groundwork for adding signals is already done.
There is also the question if we should not just add something like:

destroyOnAbort({ stream, signal })

To achieve our goal.

I would like to see some forward-thinking on how this API will be used by the Node and JS ecosystem and the interaction with the rest of the Node.js Stream libraries.
Would you mind to articulate a bit more on what type of use cases would you like to solve with these and do several examples?

It might well be that we should create a repo and go through some code example before settling into how this API would work.

cc @nodejs/streams @nodejs/http

@littledan
Copy link

@mcollina Just about the EventTarget/EventEmitter part: When you construct an AbortController, that will create an AbortSignal, which would be some kind of event thing. I was imagining that this would initially be an EventEmitter, but if we could make it also act as an EventTarget, then this could improve Node/Web compatibility: for the case where you write some code that's cancellable and want to listen for the cancellation being triggered, you'd use .on() or .addEventListener() depending on whether it's an EventEmitter or EventTarget.

@ronag
Copy link
Member

ronag commented May 6, 2020

I don't think we should expand on the .pipe(...) or .read(...) API. I would rather we look into adding this to pipeline(...)

await pipeline(
  createReadStream('file.bin'),
  uploadToSomewhereElse,
  { signal: req.signal }
)

@mcollina
Copy link
Member

mcollina commented May 6, 2020

Just about the EventTarget/EventEmitter part: When you construct an AbortController, that will create an AbortSignal, which would be some kind of event thing. I was imagining that this would initially be an EventEmitter, but if we could make it also act as an EventTarget, then this could improve Node/Web compatibility: for the case where you write some code that's cancellable and want to listen for the cancellation being triggered, you'd use .on() or .addEventListener() depending on whether it's an EventEmitter or EventTarget.

I'm generically ok with the above, I'd like to see an implementation of this and how it could be used in core.

(We might work on this in a fork of Node.js similarly to how quic and esm have been developed).

@ptomato
Copy link

ptomato commented May 8, 2020

For a proof of concept, I'm intending to use a hacky, approximate, non-compliant implementation of AbortSignal that uses EventEmitter. Basically just this:

class AbortSignal extends EventEmitter {
  constructor() {
    super();
    this.aborted = false;
  }
}

class AbortController {
  constructor() {
    this.signal = new AbortSignal();
  }
  abort() {
    if (this.signal.aborted) return;
    this.signal.aborted = true;
    this.signal.emit('abort');
  }
}

The reason is to make this experiment agnostic to the question of whether the implementation in the end should accept AbortSignals that are EventTargets, EventEmitters, or both.

Disclaimer, my judgement and experience with what should be in Node core and what shouldn't, is much less than probably anyone else on this thread. But it seems reasonable to me to treat passed-in AbortSignal objects as if they were EventTargets (that is, they need to have an addEventListener() method even if they are EventEmitters), and make sure that the AbortSignal that ships in Node core provides both APIs.

@jasnell
Copy link
Member

jasnell commented Jun 18, 2020

Update on this... AbortController, AbortSignal, EventTarget, and Event have been added as experimental APIs in master. The EventTarget implementation has a number of open PRs that need to land and we need to bring in the web platform tests for these, however, we can start adding AbortSignal support where it makes sense.

@ptomato
Copy link

ptomato commented Jun 18, 2020

Coincidence, I am just now resuming work on my branch and rebasing it on top of your work. If it's considered wanted to split out mergeable parts of the work-in-progress, then I can soon make a PR that adds a wrapper to FSReq for uv_cancel() and adds an ERR_CANCELLED error type.

@ptomato
Copy link

ptomato commented Jun 23, 2020

I've pushed to my branch (https://github.com/nodejs/node/compare/master...ptomato:31971-abortcontroller?expand=1) and it now uses the built-in AbortController in the examples.

I think the first two commits (exposing uv_cancel() as a method on FSReqBase) could be separated out into a first pull request but perhaps @jasnell or someone else would like to take a look and see if you agree with the general approach before I go and write tests for this?

ptomato added a commit to ptomato/node that referenced this issue Jun 27, 2020
ptomato added a commit to ptomato/node that referenced this issue Aug 4, 2020
GetDOMException() was a private function in node_messaging, but we would
like to use it to throw AbortErrors when cancelling something via an
AbortController. Move it into environment.cc to be in the same place as
GetPerContextExports(), and expose it internally in node_internals.h.

Refs: nodejs#31971
ptomato added a commit to ptomato/node that referenced this issue Aug 4, 2020
This adds a FSReqBase::Cancel() method which calls uv_cancel(). This
shows up in JS as a cancel() method on FSReqCallback and FSReqPromise.

Refs: nodejs#31971
@benjamingr
Copy link
Member

Just an update here that AbortController and Event are now live and mostly whatwg compatible (we're adding final tests and fixing things). Some Node.js APIs have already gotten AbortSignal support (like EventEmitters and promises versions of timers).

This is a good area to contribute in :]

@jimmywarting
Copy link

uh? have have timers gotten support for abort signal?

can you now do:

setTimeout(() => {

}, 1000, { signal })

have never heard of "promises versions of timers"

@martinheidegger
Copy link
Author

@jimmywarting this would be in conflict with JavaScript's setTimeout API:

setTimeout(foo => console.log(foo), 100, 'bar')

@jimmywarting
Copy link

yea, you are right!

i just found the promise version of timers here in https://nodejs.org/dist/latest-v15.x/docs/api/timers.html#timers_timers_promises_api
timersPromises.setTimeout(delay\[, value\[, options\]\])

node_fetch previously had a timeout option but was deprecated/removed in v3 in favor of abortSignal + timer was not a spec'ed
A Wishful replacement would be a timer that can return a signal too - something like

signal = some_node_util.timeoutSignal(1000) // or:
signal = AbortSignal.timeout(1000)
fetch(url {signal})

See this thread for more information whatwg/fetch#951 (comment)

@martinheidegger
Copy link
Author

Hmm, I prefer to see the difference in the error message and created recently a wrapTimeout util.

@benjamingr
Copy link
Member

@jimmywarting

uh? have have timers gotten support for abort signal?

They have in require("timers/promises") :]

Like Martin said - we can't do this for setTimeout to not break our own code :]

@benjamingr
Copy link
Member

@jimmywarting

A Wishful replacement would be a timer that can return a signal too - something like

Actually, having a helper on AbortControllers to abort automatically after a time could be interesting/useful though it's probably a good idea to ask that of whatwg (we can extend it and ship a subclass but would rather not until working with spec bodies hasn't worked - and so far whatwg have been very helpful when we presented compelling cases for APIs that would make sense in browsers as well to them).

@martinheidegger
Copy link
Author

Doing some more work on this, I summarized some findings here, it thought it may give some ideas on improved tooling: https://qiita.com/martinheidegger/items/3e6355e96e85fc1c841e

@benjamingr
Copy link
Member

We've landed a bunch of apis in fs/child_process/http/http2 with AbortSignal support since this btw, we also added signal support to addEventListener and are working on a way to chain AbortSignals correctly.

@benjamingr
Copy link
Member

Ooh, cool I just realized both your original examples already work in Node now :] :

// Your example here
createReadStream('file.bin', { signal: req.signal }).pipe(res)
// Becomes (and is working today in Node
new Readable({ signal , ... rest}).pipe(res); // or, if you don't control creation
addAbortSignal(signal, getReadableSomehow()).pipe(res);

// And your example here
await readFile('file.bin', { signal: process.abortSignal });
// just works as is with `fsPromises.readFile`

@jasnell
Copy link
Member

jasnell commented Jan 4, 2021

Given that progress on this is already well underway, I'm going to close this specific issue.

@jasnell jasnell closed this as completed Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

No branches or pull requests