lib: add Timer/Immediate to promisifyed setTimer/setImmediate

[everett1992]

The current implementation of the promisified setTimeout and
setImmediate does not make the Timeout or Immediate available, so they
cannot be canceled or unreffed.

The docs and code follow the pattern established by child_process

• lib/child_process.js#L150-L171
• doc/api/child_process.md#L217-L222

There are two issues with this implementation

• it conflicts with timeout or immediate properties that exist or may exist on other promise implementations or future promise implementations. Looks like bluebird promises include a .timeout method. http://bluebirdjs.com/docs/api/timeout.html
• if the timeout/immediate is cleared the promise will never settle.
  • this issue could be addressed by a special case in the Timer or Immediate class
  • alternatively we could add clearTimeout() or clearImmediate() properties that clears the timer and rejects the promise. I can't currently think of a use case for calling unref, unref, or refresh on a promised timer so it may not be necessary to expose the whole timer instance.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[Fishrock123]

Frankly, if you think you might ever need to do a cancel, then Promises are just the wrong abstraction.

If you await such a thing, what is the expected result? An error? Cancelable promises don't exist so the base expectations are not even clear.

[everett1992]

I wrote this PR because I wrote some code like this

const deadline = util.promisify(setTimeout)(TIMEOUT);

deadline.then(() => {
  throw Error('Task took too long');
});

const task = doWork();
promise.race([task, deadline]);

If you can't cancel the timeout then deadline will always resolve and throw an error, even when doWork completes first.

With this proposal you can write

const deadline = util.promisify(setTimeout)(TIMEOUT);

deadline.then(() => {
  throw Error('Task took too long');
});

const task = doWork();
promise.race([task, deadline]).finally(() => {
  clearTimeout(deadline.timeout);
})

[devsnek]

to be fair it doesn't really matter if it rejects, race will already have resolved.

[joyeecheung]

An alternative for implementing cancellation might be implementing the AbortController API by accepting an AbortSignal in util.promisify. In that case if the signal is aborted before e.g. the timer expires it should raise an AbortError.

[jasnell]

Going the AbortController option route would be preferable in general I think, but I would be concerned that it isn't something we could support generically across the board. That is, util.promisify(fn, abortController); abortController.abort() would likely only be able to work in a handful of core API cases. We don't, for instance, have the ability currently to cancel a fs.read() operation, promisified or not. We'd end up in a situation where the AbortController only works sometimes and the user would have to know explicitly when and where it worked.

An alternative approach could be to attach an AbortController to the returned promisified function or Promise... or for the promisified function itself to be an AbortController...

// In this option, the AbortController would abort all Promises
// created by the sleep function.
const sleep = util.promisify(setTimeout)
sleep(10)
sleep(20)
sleep.abort()  // cancel both sleep promises

// or

sleep.abortController.abort() // cancel both sleep promises

Or...

const sleep = util.promisify(setTimeout)
const abortable1 = sleep(10)
const abortable2 = sleep(20)
abortable1.abort()
abortable2.abort()

In the second example, the Promise returned by sleep would essentially implement the AbortController interface. The presence of the abort() function would be used as a way of signaling that the Promise is abortable.

Obviously the second option has the downside of extending the Promise API which would cause problems down the road if TC39 decides to add it's own abort or cancel method.

Bottom line, however, is that the promisified function should provide a way of allowing the user to discover if the Promise is abortable as opposed to the user just passing in an AbortController argument and hoping for the best.

[joyeecheung]

Bottom line, however, is that the promisified function should provide a way of allowing the user to discover if the Promise is abortable as opposed to the user just passing in an AbortController argument and hoping for the best.

Since we implement util.promisify with special symbols on different APIs, I think we can just whitelist the APIs that support those, or use another symbol to add abort algorithms for specific APIs. Then util.promisify can just throw if an abort signal is passed in through an option but the API being promisified does not have an abort algorithm available.

[jasnell]

that could work also but ends up with the requirement that util.promisify() calls would need to be defensively wrapped in try/catch if the user isn't sure that the function being passed is abortable or not...

let fn;
try {
  fn = util.promisify(otherFn, abortController)
  // cancelable
} catch {
  // not cancelable
}

vs.

const fn = util.promisify(otherFn)
if (typeof fn.abort === 'function') {
  // cancelable
} else {
  // cancelable
}

Stylistically I prefer the latter over the former.

[everett1992]

to be fair it doesn't really matter if it rejects, race will already have resolved.

It matters if the deadline callbacks have side-effects. In my case I was emitting a metric.

[everett1992]

Bottom line, however, is that the promisified function should provide a way of allowing the user to discover if the Promise is abortable as opposed to the user just passing in an AbortController argument and hoping for the best.

Since we implement util.promisify with special symbols on different APIs, I think we can just whitelist the APIs that support those, or use another symbol to add abort algorithms for specific APIs. Then util.promisify can just throw if an abort signal is passed in through an option but the API being promisified does not have an abort algorithm available.

This discussion makes me question the point of having custom util.promisify in the core library, instead of adding promise native functions to the libraries, like fs/promises. Then users don't have to question if a util.promisify'ed function supports abort controllers or not.

That said, users know if util.promisify supports an abort controller by reading the documentation, or thru static types. @types/node knows that util.promisify(child_process.execFile) returns promises with a .child property.

const proc = util.promisify(child_process.execFile)('cat');
proc.then(({stdout}) =>  console.log('output', stdout));
proc.child.stdin.write('hello world');
proc.child.stdin.end();

child_process has a better use case for exposing the child property than setTimeout has to expose timeout, you need a reference to the process to write to it's stdin, an about controller isn't enough. One might want to unref a promised timer. Node can exit if there are pending promises right?

[Fishrock123]

We could make Timeout objects "thenable", where an error in the callback bubbles to a then handler if there was one?

const deadline = util.promisify(setTimeout)(TIMEOUT);

deadline.then(() => {
  throw Error('Task took too long');
});

const task = doWork();
promise.race([task, deadline]);

Then likely becomes

const deadline = setTimeout(TIMEOUT, () => {
  throw Error('Task took too long');
});

const task = doWork();
promise.race([task, deadline]).finally(() => {
  clearTimeout(deadline);
})

[jasnell]

+1 to that @Fishrock123 ... Making the timeout object a thenable makes a lot of sense. The on clearTimeout(), the thenable can reject if it hasn't already resolved. Makes for a nice elegant solution.

Just thinking about setInterval()... it would be interesting to make the interval object an async iterator so one could do...

async foo() {
  const interval = setInterval(fn, 1000)
  for await (const m of interval) {
    /* ... */
    clearInterval(interval) // would end the iterator...
  }
)

[Fishrock123]

@everett1992 yes, I will correct that.

Doing that also makes the async iterator story more streamlined, since both could just be directly on the timeout object.

[devsnek]

@nodejs/open-standards btw

[devsnek]

would be nice to see if its possible to say in sync with the dom too: whatwg/html#617

[Fishrock123]

The dom has a bunch of problem which we frankly don't have (because someone had a good idea and returned a timer object rather than a number). I don't think we should follow them, it's very hard to trust a good api design to come out of that, considering what the dom wants compared to us.

[devsnek]

@Fishrock123 i'm not saying we have to, but i think we should at least explore it. I'm planning to work on the dom api some time in the nearish future, so it shouldn't be too hard to keep node in the loop.

[everett1992]

for what it's worth the DOM issue discusses adding a new function, not extending setTimeout. This feels like an opportunity to design a good api for dom and node.

[Fishrock123]

I have written my thoughts into a somewhat long post over on the whatwg thread: whatwg/html#617 (comment)

[mcollina]

I'm in favor of using thenables and an async iterators.

[jasnell]

Looks like the response on the other thread @Fishrock123 points to is that the whatwg approach is going to use AbortController no matter what. We can go with that approach also as an option, but it will be difficult to implement consistently, so I'm definitely still in favor of making the object returned by setTimeout and setInterval a thenable and async iterator, then having clearTimeout and clearInterval cause the thenable to be rejected with an error that signals that the timer was cleared early.

[benjamingr]

Thanks a lot for doing this and for taking a stab at it. I agree with the other comments that this is rather footfun-y :/

[devsnek]

@jasnell there is also work at tc39 to standardize a cancellation api that would drop in with AbortController and could therefore interoperate with node. All I'm asking is that we slow down a little bit here :) I'm going to be working on async dom timers at some point in the coming months and as someone who also maintains node I'm inclined to at least try to match up the apis.

[jasnell]

@devnek ... no worries on being patient on it. I'm hesitant to do anything on cancelable promises until there's an official direction on it anyway. That said, would there be harm in experimenting with a couple of different approaches if those are clearly marked experimental? For instance, code such as the following could easily emit a process warning...

const sleep = setTimeout(fn, 100);
sleep.catch( /* ... */ ) 
clearTimeout(sleep)

[Fishrock123]

@devsnek Fwiw, as noted in my post to whatwg, cancellation does not cover all use cases for persistents.

[benjamingr]

I would be fine however with supporting AbortController as we think fetch is something we will want to support anyway apparently (or at least that was the consensus in the summit).

[benjamingr]

@Fishrock123 just to be clear, we cannot:

We could make Timeout objects "thenable", where an error in the callback bubbles to a then handler if there was one?

That would break with async functions which always return a native promise, the proposal to let us do that was rejected 3 years ago in tc39 (compositional functions).

[ljharb]

I would strongly suggest not supporting AbortController outside of fetch; it's not a great name, and it doesn't imo fit with Promises in a general sense.

[Fishrock123]

@benjamingr I do not understand what you are referring to.

I have a patch in progress which appears to work fine -- do you mean having the timeout callback be an async function? (That would be a separate issue...?)

[Fishrock123]

@ljharb You may want to leave that feedback on whatwg/html#617 or else whatwg will likely implement it for other apis too.

[ljharb]

They’ll likely do it regardless of the feedback they get.

[devsnek]

@Fishrock123 i think Benjamin is referring to something like this:

async function x() {
  return setTimeout(() => {}, 10);
}

x().then((timer) => {
  clearTimeout(timer);
});

[benjamingr]

I would strongly suggest not supporting AbortController outside of fetch; it's not a great name, and it doesn't imo fit with Promises in a general sense.

@ljharb we should strive to do the same thing as the web platform and standard suggest rather than roll our own abstractions IMO. If the web platform has settled on AbortController that is what we should do in my opinion. Otherwise we're going against the web platform.

@benjamingr I do not understand what you are referring to.

There was a proposal ( https://github.com/jhusain/compositional-functions ) to make it possible to hook into await but it did not pass.

We can expose an async iterator for setInterval just fine :]

@jasnell - that would not work - util.promisify returns a function which means that the AbortController would have to be passed on the returned function rather than on the util.promisify call no?

[benjamingr]

Also - I am -1 on using thenables since they do not work well with our tooling. They are harder to inspect and debug and create assimilation hazards. I am also in favor of AbortController because it is the whatwg standard way to do cancellation.

That said, deep down I still believe that cancellable promises like other languages and ecosystems do are the correct solution to this problem. I do not want to pursue that because that is not the direction TC39 is giving us so I recommend we stick to the standards.

[benjamingr]

And just to clarify - I am +1 on an async iterator for setInterval regardless of what we do for setTimeout :]

[ljharb]

@benjamingr the language still has an active proposal for general promise cancellation; I’m suggesting waiting for that, even if it takes awhile.

[Fishrock123]

As previously noted, making the promise cancellable only solves one of the issues.

[benjamingr]

@ljharb

@benjamingr the language still has an active proposal for general promise cancellation; I’m suggesting waiting for that, even if it takes awhile.

Link?

[ljharb]

https://github.com/tc39/proposal-cancellation

[benjamingr]

Oh, that one - if I understand correctly that proposal is both inactive (stage 1 for years) and intends to be compatible (interop wise not api wise) with AbortController.

The bigger issue with AbortController is that it depends on EventTarget or at least that's why we were stuck last time. We can ship AbortController built on EventEmitter and adopt it to a regular Emitter when/if the emitter proposal lands.

Pinging @rbuckton - are you still pursuing that proposal?

[rbuckton]

Yes, I just haven't had much time to spend on it lately.

[benjamingr]

@rbuckton do you know when you will be presenting it next?

[rbuckton]

@benjamingr I'm focusing on advancing two of my other proposals at the moment. It likely won't be until next year that I can focus on the proposal again. Currently the proposal is focused on a "protocol" based approach using a Symbol-based API (similar to iteration), rather than an actual cancellation token implementation. The goal is that the ECMAScript spec would specify the protocol, which could be added to the existing AbortController/AbortSignal implementation, and that library authors and other runtimes (such as NodeJS) could also implement the protocol without needing to depend on AbortController (and thus EventTarget). However, since there would be no actual implementation in the ECMAScript spec, there need to be compelling use cases in ECMAScript spec for such a feature.

The upside of a protocol-based approach is that a library author could accept cancellation signals by depending on the protocol rather than needing to feature test for AbortController/CancellationToken/etc., which should help for interoperability. This means that a host or package could also implement their own version of AbortController/CancellationToken/etc. and attach the Symbol-based protocol to it to make it interoperable.

You can see an example of this protocol implemented in userland in @esfx/cancelable (source). That package doesn't itself provide an implementation of cancellation tokens, but merely the protocol. It does this in terms of a Cancelable.cancelSignal symbol (which in the current proposal would instead be Symbol.cancelSignal).

[benjamingr]

@rbuckton what do you think Node.js should do with its cancellation?

We have the following options:

• Ship AbortController - we are very hesitant about this for multiple reasons including EventTarget (especially now that there is a TC39 events proposal), API ergnomics and the fact the web platform has not adopted AbortController for many APIs.
• Ship our own minimalistic cancellation API - the biggest concern in this would be future proofing and web compatibiliity.
• Tag promises and implement a promise cancellation like feature - I think this would violate behavioural sub-typing since it would break invariants on promises currently provided by the platform.

Given that the current proposal is stage 1 and work likely won't happen on it for another year at which point it has to go through a risky process - what should Node.js do?

If we decide this is a priority for Node.js is there any way for our own TC39 representation (assuming they are interested) to help?

(and to be clear that is fair, you owe us nothing and I am not complaining and am thankful for your work)

[Fishrock123]

Note: the choice we make here will likely have implications on other non-web apis that node ships too.

[BridgeAR]

Is there any progress?

[Fishrock123]

So, Rust's async_std::futures::timeout shows a better way of solving this exceedingly silly problem.

https://github.com/http-rs/async-h1/blob/428fb9e8c6cb17e615f8fccd6fba1904934bb821/src/server/mod.rs#L61-L65

The answer is: have the timeout Promise wrap (be chained from) the Promise which could "cancel" it.

If the upstream needs to cancel, it can just resolve the promise, which should be able to cancel the timer promise's internal timer.

[jasnell]

Promisified timers can now be canceled #33833 ...
and a new timers/promises module that allows promisified timers to be unref'd is open here #33950

Given that there's been no further activity on this PR for over a year, closing.