getify / CAF

Cancelable Async Flows (CAF)
MIT License
1.34k stars 46 forks source link

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

Closed benjamingr closed 1 month ago

benjamingr commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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
   }
);
benjamingr commented 3 years ago

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:

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 1 month ago

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