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

AbortController (in Node), integration with CAF? #21

Closed
benjamingr opened this issue Feb 3, 2021 · 10 comments
Closed

AbortController (in Node), integration with CAF? #21

benjamingr opened this issue Feb 3, 2021 · 10 comments

Comments

@benjamingr
Copy link

Hey Kyle 👋

Node recently(ish) shipped AbortController/AbortSignal (I believe I pinged you on one of the issues).

I just wanted to ask if there is anything Node.js should be doing differently with regards to our AbortController/AbortSignal or our usage of them in APIs or if you have an opinion regarding anything else we should do to improve the cancellation story.

@getify
Copy link
Owner

getify commented Feb 3, 2021

Thanks for the ping. I think I did see that recently, yes. Very glad to see it happening.

The thing that comes to mind first is the need to be able to easily get a promise that resolves (or rejects!) when an a specific abort token is triggered.

CAF implements this as a .pr property on the signal object, so you can "subscribe" to that. But it would be nice if there was some idiomatic and clean way to take a signal and get a Promise for its firing.

Do you have any thoughts on that?

@getify
Copy link
Owner

getify commented Feb 3, 2021

Also, more broadly, I'd like to see more (ideally, all someday) of Node's asynchronous APIs taking the signal as a param, similar to fetch(..) and addEventListener(..) in the web, to force deep cancellation of any async tasks.

For example:

fs.writeFile("/path/to/file.txt", someBigContents, { encoding: "utf-8", signal });

One place that might be easy to start in terms of rolling out that support might be the Timers APIs, for example:

setTimeout(
   function whatever() {
      // ..
   },
   {
      delay: 2000,
      signal
   }
);

@getify getify changed the title AbortController AbortController (in Node), integration with CAF? Feb 3, 2021
@benjamingr
Copy link
Author

Thanks for the detailed answer and feedback!

The thing that comes to mind first is the need to be able to easily get a promise that resolves (or rejects!) when an a specific abort token is triggered.

CAF implements this as a .pr property on the signal object, so you can "subscribe" to that. But it would be nice if there was some idiomatic and clean way to take a signal and get a Promise for its firing.

Do you have any thoughts on that?

Unfortunately I don't think we are allowed (spec wise) to add methods to AbortSignal.

We do have a utility in events that makes this easier:

const { once } = require('events');
const ac = new AbortController();
const { signal } = ac;

// some time later, we want to wait for the signal to abort
await once(signal, 'abort'); // Works with both EventTarget and EventEmitter, fulfills with `undefined` once the event fired

One possible downside for this is that it doesn't fire if the signal already fired so the actual code would have to look like:

await (signal.aborted || once(signal, 'abort'));

Which might not be the most intuitive to users so the ergonomics do leave room for improvement.

I have collaborated with whatwg before on related modifications (adding AbortSignal support to EventTarget for example) so I think adding .aborted Promise (or similar) to AbortSignal should be possible if we can show a compelling enough use case for it (I think it's probably doable).

Additionally we can (should we?) expose a utility for this in Node.js (we are allowed to do that without violating the spec). It can look something like:

await aborted(signal); // fulfills the promise when the signal fires `abort` (or immediately if it is already aborted 

(As a fun tidbit - once actually takes a signal third parameter itself to allow cancelling waiting for the event)


Also, more broadly, I'd like to see more (ideally, all someday) of Node's asynchronous APIs taking the signal as a param, similar to fetch(..) and addEventListener(..) in the web, to force deep cancellation of any async tasks.

I am very much in favour!

We have been rapidly adding support for AbortSignals in core APIs in recent versions (15.3+). I am happy to say your fs.writeFile example already works today on the latest v15 release 😄 .

This is also true for the promises version of fs.writeFile. We are at a point where most APIs are covered:

  • stream - all readable and writeable streams take a signal parameter in their constructor to destroy the stream. An additional addAbortSignal utility method is exposed to enable use-cases where you don't control stream construction.
  • fs - readFile, writeFile as well as anything with streams like fs.createReadStream, there are open PRs for fs.watch and the promises version.
  • http - requests using http.request take an AbortSignal parameter.
  • http2 - requests using http2.request take an AbortSignal parameter.
  • events - the events.on and events.once` methods take an AbortSignal parameter.
  • net - it is possible to stop a net.Server with an AbortSignal.
  • dgram - it is possible to stop a udp.createSocket with an AbortSignal.

One place that might be easy to start in terms of rolling out that support might be the Timers APIs, for example:

Also happy to tell you that this already works (with setInterval landing less than 6 hours ago 🎉 ):

import { setInterval, setTimeout /*, setImmediate*/ } from 'timers/promises'; // with "type": "module"
// const { setInterval, setTimeout, setImmediate } = require('timers/promises');

const ac = new AbortController();
const { signal } = ac;
setTimeout(500).then(() => ac.abort());
(async () => { // setInterval example
  for await (const tick of setInterval(100, null, { signal })) {
    console.log("interval ping")
  }
})();
(async () => { // setTimeout example
  console.log('before setTimeout');
  try {
    await setTimeout(1000, null, { signal });
    console.log('timeout elapsed');
  } catch (e) {
    console.log('got an:', e.name);
  }
  console.log('after setTimeout');
})();
// Logs
// before setTimeout
// interval ping
// interval ping
// interval ping
// interval got an: AbortError
// timeout got an: AbortError
// after setTimeout

Interesting times :]

@getify
Copy link
Owner

getify commented Feb 3, 2021

Hmm... question about the timers thing... it's passed as the third parameter? In the web, setTimeout/setInterval take optional third (and further) params and pass them as arguments to the callback. Does that break here?

@benjamingr
Copy link
Author

@getify it does break here - but whatwg seemed optimistic regarding adding a (future) new promises-based API and even said (and I quote):

For Chrome/V8, I can say that we're not interested in any version without AbortSignal integration, or any version that does not use the same specification infrastructure as setTimeout.

So it seems like if (when) a promises version of setTimeout will reach the DOM it'll have AbortSignal support and no collision with the "regular" setTimeout (either with an import or with another name).

That said: Node timers and Web timers don't implement the same specification (or rather - Node timers don't follow the specification and return objects and not numbers) anyway :]

(The main reason that the web timers promises version hasn't progressed is people dedicating time to working on it, I am pretty confident that if the spec was written and agreed on - browsers would implement)

@getify
Copy link
Owner

getify commented Feb 3, 2021

Ahhh... I hadn't looked at Node's "timers/promises" before. Yeah, the signature is different: since there's no callback there's no need to pass args. And instead, there's just a single value argument to resolve the promise with. Makes more sense.

@getify
Copy link
Owner

getify commented Feb 3, 2021

I like this a lot. This would be great if Node could consider adding it:

await aborted(signal);

The name aborted is pretty generic and likely to cause lexical conflicts. Wondering what we can do to make it easy to use but not likely to collide lexically.

@getify
Copy link
Owner

getify commented Feb 3, 2021

if we can show a compelling enough use case for it (I think it's probably doable).

As for "compelling use case", basically I do this (via CAF) almost all the time:

Promise.race([
   // some work
   
   // abort signal promise
])

@benjamingr
Copy link
Author

I've opened a post regarding aborted - in the meantime I'll try to push (internal) weak event handlers in (so that aborted does not leak memory and holds the callback "weakly" on the object).

@getify
Copy link
Owner

getify commented Oct 9, 2024

Seems like this has stalled indefinitely, so closing (for now).

@getify getify closed this as completed Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants