timers: introduce setInterval async iterator #35841

fabiancook · 2020-10-28T01:54:01Z

Utilises Symbol.asyncIterator to provide an iterator compatible with for await

const {
  setInterval
} = require('timers/promises');

const ac = new AbortController()
const signal = ac.signal

setTimeout(() => ac.abort(), 500)

for await (const value of setInterval(interval, null, { ref: false, signal })) {

}

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

lib/timers.js

devsnek · 2020-10-28T03:41:45Z

lib/timers/promises.js

+        deferred;
+      let active = true;
+      const iterator = {
+        next() {


this timing logic is incorrect. some examples:

const it = setInterval(100); it.next(); // should resolve after 100ms it.next(); // should resolve after 200ms

const it = setInterval(100); await delay(50); it.next(); // should resolve after 50ms it.next(); // should resolve after 150ms

const controller = new AbortController(); const it = setInterval(100, { signal: controller.signal }); it.next(); // below should reject this promise controller.abort();

This may have to be an option to enable one or the other timing..

Here is an example that brings the idea behind using timing for each .next()

const iterable = setInterval(100); const iterator = iterable[Symbol.asyncIterator](); await iterator.next() await delay(150) await iterator.next() await delay(150)

This would be the same as

for await (const unused of setInterval(100)) { await delay(150) }

There is also a difference between the "async iterable" instance (that can be passed directly to for await) vs the iterator creation time, and then the next request to wait for the next interval

Timing given we're hooking onto the next occurring interval from iterator.next being called

setInterval call - 0ms next call - 0ms next resolve - 100ms delay call - 100ms delay resolve - 250ms next call - 250ms next resolve - 300ms <- we skipped 1 intervals, 200ms, was 50ms since `next` invoked delay call - 300ms delay resolve - 450ms next call - 450ms next resolve - 500ms <- we skipped 1 interval, 400ms, was 50ms since `next` invoked

Timing given we have one promise for each interval:

setInterval call - 0ms next call - 0ms next resolve - 100ms delay call - 100ms delay resolve - 250ms next call - 250ms next resolve - 250ms <- Because its now on the 200ms promise, was 0ms since `next` invoked delay call - 250ms delay resolve - 400ms next call - 400ms next resolve - 400ms <- Because we can never get back now, 300ms promise, was 0ms since `next` invoked

The current timing would be

setInterval call - 0ms next call - 0ms next resolve - 100ms delay call - 100ms delay resolve - 250ms next call - 250ms next resolve - 350ms <- 100ms since `next` invoked delay call - 350ms delay resolve - 500ms next call - 500ms next resolve - 600ms <- 100ms since `next` invoked

const iterable = setInterval(100); const iterator = iterable[Symbol.asyncIterator](); const first = iterator.next() const second = iterator.next()

The above code seems a bit confusing... we would need to "queue" those resolves then, maybe thats fine?

const defers = [] setInterval(() => { const next = defers.shift() next.resolve({ done: false, value }) }, after) /* ... */ next() { /* ... */ const deferred = /* ... */ defers.push(deferred) return deferred.promise }, return() { /* ... */ const result = { done: true, value: undefined } defers.forEach(deferred => deferred.resolve(result)) return result }

I think one, or the other, or both sets of logic will be good, just need to know what one

Im in favour of having:

Timeouts triggered within next as the default behaviour (keeping consistent behaviour across all use cases)

A single interval for all iterators as an option

A single interval for each iterator as an option (as it would be if we implemented like timers: introduce setInterval async iterator #35841 (comment))

If we add the two additional options, this would definitely add to the overall complexity for sure, I would think we would need three completely seperate implementations for this.

I believe next should be asserted to being called only after the previous promise was resolved as well (e.g. deferred not existing in the current implementation), rather than leaving this open to interpretation, else we will most likely need to queue multiple promises to be resolved for each iterator at the same time, rather than one in flight per iterator

A single interval for each iterator as an option (as it would be if we implemented like #35841 (comment))

Isn't that the mental model when someone does setInterval?

The disadvantage of my implementation in that comment is "what happens if the user 'misses' an interval?" The timing is right but the code should probably push the callbacks to an array (instead of replacing a reference) and fulfil them in the interval.

Also cc @ronag wdyt about what we should do with timers -> async iterators conversion in terms of behavior?

Reading the HTML spec for setInterval, it runs the setTimeout steps, with a "repeat" flag that repeats the steps again post task (which for the setInterval, it is a sync task)

https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#dom-setinterval

The setInterval() method must return the value returned by the timer initialization steps, passing them the method's arguments, the object on which the method for which the algorithm is running is implemented (a Window or WorkerGlobalScope object) as the method context, and the repeat flag set to true.

The repeating happens after the related tasks function has been invoked (which in our case is the external code executing, the stuff between next calls) and for any errors to be caught

Run the appropriate set of steps from the following list:

If the first method argument is a Function
Invoke the Function. Use the third and subsequent method arguments (if any) as the arguments for invoking the Function. Use method context proxy as the callback this value. If this throws an exception, catch it, and report the exception.

Then

If the repeat flag is true, then call timer initialization steps again, passing them the same method arguments, the same method context, with the repeat flag still set to true, and with the previous handle set to handler.

The running of the task now is async, we tell the consumer to start the task by resolving the task, and the consumer signifies the task is complete by calling next again (or return)

Aside from the above, we could warn if we skip intervals and leave it at that, but then this seems like inconsistent behaviour, if I had a 30 minute interval going, happened to take 31 minutes for my task, and now have to wait for 29 minutes till the next interval.... leading to the intervals actually being once an hour

Conceptually I would imagine it to be equivalent to adding an await on timer._onTimeout() here:

node/lib/internal/timers.js

Lines 550 to 562 in bec918f

start = getLibuvNow();

try {

const args = timer._timerArgs;

if (args === undefined)

timer._onTimeout();

else

timer._onTimeout(...args);

} finally {

if (timer._repeat && timer._idleTimeout !== -1) {

timer._idleTimeout = timer._repeat;

insert(timer, timer._idleTimeout, start);

} else if (!timer._idleNext && !timer._idlePrev && !timer._destroyed) {

Ah! As I say that, I see start is set before _onTimeout is invoked 🤔

Reading the HTML spec for setInterval, it runs the setTimeout steps, with a "repeat" flag that repeats the steps again post task (which for the setInterval, it is a sync task)

Well, two things:

That's just an easy way to specify things and doesn't actually indicate when it runs.

We don't follow that (the web timers) spec (even remotely) though if we built Node today then we probably would.

What I meant is just "the standard queueing trick" something like:

async function* iteratorSetInterval(ms, options = {}) { // validateOptions // validateAbortSignal let callbacks = [], pending = 0; let aborted = options && options.signal && options.signal.aborted; if (options && options.signal) { options.signal.addEventListener('abort', () => { aborted = true; }); } const timerRef = setInterval(() => { if (callbacks.length > 0) callbacks.shift()(); else pending++; }, ms); if (options && options.ref === false) { timerRef.unref(); } try { while (!aborted) { yield await new Promise((resolve) => { if (pending > 0) { resolve(); pending--; } ; else callbacks.push(resolve); }); } } finally { clearInterval(timerRef); } }

benjamingr · 2020-10-28T08:18:54Z

lib/timers/promises.js

@@ -122,7 +124,110 @@ function setImmediate(value, options = {}) {
  });
 }

+function setInterval(after, value, options = {}) {
+  const args = value !== undefined ? [value] : value;
+  if (options == null || typeof options !== 'object') {


Can options == null ever happen here given it has a default value of {} ?

function f(o = {}) { console.log({ o }) } f(null)

Outputs

{ o: null }

This is a copy from setTimeout

Ah so it can be null but not undefined - you can check === in that case probably

benjamingr · 2020-10-28T08:20:29Z

lib/timers/promises.js

@@ -122,7 +124,110 @@ function setImmediate(value, options = {}) {
  });
 }

+function setInterval(after, value, options = {}) {
+  const args = value !== undefined ? [value] : value;


Nit: for validation prefer using internal/validators (e.g. for validating an object you can call validateObject) etc

benjamingr · 2020-10-28T08:21:29Z

lib/timers/promises.js

+    (signal === null ||
+      typeof signal !== 'object' ||
+      !('aborted' in signal))) {
+    throw new ERR_INVALID_ARG_TYPE(


Ditto regarding validateAbortSignal and internal/validators

benjamingr · 2020-10-28T08:22:34Z

lib/timers/promises.js

+      function onAbort() {
+        if (deferred) {
+          deferred.reject(
+            lazyDOMException('The operation was aborted', 'AbortError')


Async iterators don't need to reject on cancellation or expose the abort error - it's enough to .return on the iterator which would cause whatever is wrapping it (for await, stream etc) to close.

This is definitely a question I hoped to come up in review.

@/jasnell had suggested endOnSignal to be an option to either reject or resolve for this.

cc @jasnell - i think the mental model is that streams and iterators have a concept of "abort" built in unlike promises - so it's better to use it probably :]

I can see it both ways really. An option makes it more complicated, but gives users the flexibility. I'm honestly not sure which should be the default here.

I avoid doing cancellation "the regular JS way" with .return and not with a rejection/error unless we have a very compelling reason not to do the "standard" thing.

Note that if we implement this with an async generator this solution will work "automatically" by putting cleanup in a finally block

benjamingr · 2020-10-28T08:24:21Z

Hey, thanks for tackling this! This has some more work before it can be merged but good start!

The biggest issue like devsnek said is probably the timing logic - the second issue is how to close the iterator and what cancellation signifies.

doc/api/timers.md

benjamingr · 2020-10-28T08:28:04Z

Side note #2 - the implementation can be much simpler by using an async iterator and taking the same approach Node takes for stream async iterators.

benjamingr · 2020-10-28T08:36:18Z

Something like:

async function* iteratorSetInterval(ms, options = {}) {
  // validateOptions
  // validateAbortSignal

  let aborted = options && options.signal && options.signal.aborted;
  if (options && options.signal) {
    options.signal.addEventListener('abort', () => {
      aborted = true;
    });
  }
  
  const timerRef = setInterval(() => {
    callback();
  }, ms);
  if (options && options.ref === false) {
    timerRef.unref();
  }

  try {
    while (!aborted) {
      yield await new Promise((resolve) => callback = resolve);
    }
  } finally {
    clearInterval(timerRef);
  }
}

Though it's probably better to consider how setInterval behaves in regards to "backpressure" and the consumer not yet being "ready" to process the interval. (Gus's comment basically?)

Trott · 2020-10-28T13:46:01Z

If this lands, should it land as an experimental feature? @nodejs/timers

benjamingr · 2020-10-28T14:28:06Z

@Trott I think everything under timers/promises is experimental anyway - so this would be adding a method to an experimental module

fabiancook · 2020-10-29T07:51:43Z

RE argument validation, the assertions are the same as from setTimeout and setImmediate, should we roll up these all into a single set of functions?

The difference is though, the other functions return rejected promises with the errors, while setInterval is throwing them.

Utilises Symbol.asyncIterator to provide an iterator compatible with `for await` ``` const { setInterval } = require('timers/promises'); const ac = new AbortController() const signal = ac.signal setTimeout(() => ac.abort(), 500) for await (const value of setInterval(interval, null, { ref: false, signal })) { } ```

Remove custom promisify from setInterval

--- This is still WIP and I will replace this commit with more progress, currently waiting for the branch to build locally.. ---

ronag · 2020-12-17T12:39:30Z

I haven't dug into this but I'm a little concerned in regards to how back pressure is handled in this case?

This is a common cause for problems and confusion for e.g. rxjs operators.

The timing and backpressure behavior I think needs more explicit documentation to properly define expectations.

ronag

Adding a "request changes" for better documentation of timings and backpressure.

fabiancook · 2020-12-17T12:43:05Z

RE the most recent commit, I haven’t yet completed the changes I am working on. I had pushed it to remote as it was a chunk of changes that I didn’t just want locally, still a bunch to work through and re-test, then document.

aduh95 · 2021-01-18T20:07:24Z

@fabiancook Hey! Are you still working on this?

aduh95 · 2021-02-03T16:54:28Z

Closing in favor of #37153.

benjamingr · 2021-02-03T17:00:00Z

@fabiancook note you are credited as Co-Authored-By on that PR. If you would prefer it if we used a different email please let us know :)

nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Oct 28, 2020

fabiancook commented Oct 28, 2020

View reviewed changes

lib/timers.js Outdated Show resolved Hide resolved

devsnek suggested changes Oct 28, 2020

View reviewed changes

benjamingr reviewed Oct 28, 2020

View reviewed changes

doc/api/timers.md Show resolved Hide resolved

fabiancook added 3 commits December 17, 2020 23:31

fixup! timers: introduce setInterval async iterator

e5496f2

Remove custom promisify from setInterval

fixup! timers: introduce setInterval async iterator

b71c61f

--- This is still WIP and I will replace this commit with more progress, currently waiting for the branch to build locally.. ---

ronag requested changes Dec 17, 2020

View reviewed changes

Linkgoron mentioned this pull request Jan 31, 2021

timers: introduce setInterval async iterator #37153

Closed

4 tasks

aduh95 closed this Feb 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

timers: introduce setInterval async iterator #35841

timers: introduce setInterval async iterator #35841

fabiancook commented Oct 28, 2020

devsnek Oct 28, 2020 •

edited

Loading

fabiancook Oct 28, 2020 •

edited

Loading

fabiancook Oct 29, 2020

benjamingr Oct 29, 2020

fabiancook Oct 29, 2020

fabiancook Oct 29, 2020

fabiancook Oct 29, 2020

benjamingr Oct 29, 2020

benjamingr Oct 28, 2020

fabiancook Oct 28, 2020

benjamingr Oct 28, 2020

benjamingr Oct 28, 2020

benjamingr Oct 28, 2020

benjamingr Oct 28, 2020

fabiancook Oct 28, 2020

benjamingr Oct 29, 2020

jasnell Oct 29, 2020

benjamingr Oct 29, 2020

benjamingr commented Oct 28, 2020

benjamingr commented Oct 28, 2020

benjamingr commented Oct 28, 2020

Trott commented Oct 28, 2020

benjamingr commented Oct 28, 2020

fabiancook commented Oct 29, 2020

ronag commented Dec 17, 2020

ronag left a comment

fabiancook commented Dec 17, 2020 via email •

edited

Loading

aduh95 commented Jan 18, 2021

aduh95 commented Feb 3, 2021

benjamingr commented Feb 3, 2021

	start = getLibuvNow();

	try {
	const args = timer._timerArgs;
	if (args === undefined)
	timer._onTimeout();
	else
	timer._onTimeout(...args);
	} finally {
	if (timer._repeat && timer._idleTimeout !== -1) {
	timer._idleTimeout = timer._repeat;
	insert(timer, timer._idleTimeout, start);
	} else if (!timer._idleNext && !timer._idlePrev && !timer._destroyed) {

timers: introduce setInterval async iterator #35841

timers: introduce setInterval async iterator #35841

Conversation

fabiancook commented Oct 28, 2020

Checklist

devsnek Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

fabiancook Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamingr commented Oct 28, 2020

benjamingr commented Oct 28, 2020

benjamingr commented Oct 28, 2020

Trott commented Oct 28, 2020

benjamingr commented Oct 28, 2020

fabiancook commented Oct 29, 2020

ronag commented Dec 17, 2020

ronag left a comment

Choose a reason for hiding this comment

fabiancook commented Dec 17, 2020 via email • edited Loading

aduh95 commented Jan 18, 2021

aduh95 commented Feb 3, 2021

benjamingr commented Feb 3, 2021

devsnek Oct 28, 2020 •

edited

Loading

fabiancook Oct 28, 2020 •

edited

Loading

fabiancook commented Dec 17, 2020 via email •

edited

Loading