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

timers: introduce setInterval async iterator #37153

Closed
wants to merge 1 commit into from

Conversation

Linkgoron
Copy link
Member

@Linkgoron Linkgoron commented Jan 31, 2021

Added setInterval async generator to timers\promises. Utilises async generators to provide an iterator compatible with
for await. This is a continuation of initial work by Fabian Cook #35841 (who is also marked as co-author in this PR). The async-generator throws an AbortError when aborted.

Note that there are some decisions I've made regarding (so-called) backpressure. If multiple iterations were completed before a value is yielded, the next iteration won't wait delay long, but yield the next value in the next tick. In addition, if there was backpressure before the controller was aborted, the generator will emit all of the passed intervals, and only then finish the iteration. I'd be happy to change this behavior if this is not what's expected (i.e. change the generator to stop producing immediately).

See the previous PR for more information and discussions.

Example:

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

const ac = new AbortController();
const { signal } = ac;
const interval = 10;
setTimeout(() => ac.abort(), 500);

let i=0;
for await (const value of setInterval(interval, undefined, { ref: false, signal })) {
  console.log(Date.now(), i);
  if(i++>=5) {
     break;
  }
}
  • 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

@nodejs-github-bot nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Jan 31, 2021
@Linkgoron Linkgoron force-pushed the timers-set-interval-async branch 3 times, most recently from 45584a1 to 42a7f52 Compare January 31, 2021 00:32
@Linkgoron
Copy link
Member Author

cc @benjamingr

lib/timers.js Outdated Show resolved Hide resolved
lib/timers/promises.js Outdated Show resolved Hide resolved
lib/timers/promises.js Outdated Show resolved Hide resolved
test/parallel/test-timers-promisified.js Outdated Show resolved Hide resolved
test/parallel/test-timers-promisified.js Outdated Show resolved Hide resolved
lib/timers/promises.js Outdated Show resolved Hide resolved
Comment on lines 160 to 137
if (abortCallback) {
abortCallback(new AbortError());
passCallback = undefined;
abortCallback = undefined;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assigning passCallback and abortCallback seems unnecessary here.

Suggested change
if (abortCallback) {
abortCallback(new AbortError());
passCallback = undefined;
abortCallback = undefined;
}
abortCallback?.(new AbortError());

Copy link
Member Author

@Linkgoron Linkgoron Jan 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the undefinds are there because otherwise we sometimes get an error during the tests because of multiple resolves (as we could resolve the promise and then someone aborts the iterator).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move those to after the await new Promise to avoid the repetition? No strong feeling though, if you prefer as it is now, feel free to disregard, I'd personally find it easier to follow if it were down there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, that's where I originally put it. However, it's incorrect because you could still resolve and reject before the await yields, and get multiple resolves.

Comment on lines 158 to 147
// eslint-disable-next-line no-undef
clearInterval(interval);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we import this from timers?

Copy link
Member Author

@Linkgoron Linkgoron Jan 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could, I just saw that clearTimeout also wasn't imported (I actually copied the exact same comment from that one)

Comment on lines 148 to 140
if (passCallback) {
passCallback();
passCallback = undefined;
abortCallback = undefined;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, assigning the variables to undefined seems unnecessary

Suggested change
if (passCallback) {
passCallback();
passCallback = undefined;
abortCallback = undefined;
}
passCallback?.();

Copy link
Member Author

@Linkgoron Linkgoron Jan 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the undefinds are there because otherwise we sometimes get an error during the tests because of multiple resolves (as we could resolve the promise and then someone aborts the iterator)

}

while (!signal?.aborted) {
if (notYielded === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get rid of the if statement by moving the for loop above the await new Promise?

Copy link
Member Author

@Linkgoron Linkgoron Jan 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we would get an issue like the following:
suppose that the promise was fulfilled, and before we got back to it the interval was somehow aborted. we'll go back to the while condition and get out of the while loop, instead of yielding the value.

@Linkgoron Linkgoron force-pushed the timers-set-interval-async branch 2 times, most recently from 2cb80b2 to 1f8b21c Compare January 31, 2021 03:06
doc/api/timers.md Outdated Show resolved Hide resolved
lib/timers/promises.js Outdated Show resolved Hide resolved
lib/timers/promises.js Outdated Show resolved Hide resolved
lib/timers/promises.js Outdated Show resolved Hide resolved
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job, left a few comments!

@targos
Copy link
Member

targos commented Jan 31, 2021

Is it possible to break out of the loop to stop the interval (there doesn't seem to be a test for that case)?

@benjamingr
Copy link
Member

Is it possible to break out of the loop to stop the interval

A test for that case is a good idea, I think indeed a finally clause clearing the interval is missing here.

@benjamingr
Copy link
Member

@Linkgoron I think what Michael meant is that you can break inside a for await loop and that calls .return on the async generator.

await new Promise((resolve, reject) => {
passCallback = resolve;
abortCallback = reject;
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
});
});
passCallback = undefined;
abortCallback = undefined;

Comment on lines 160 to 137
if (abortCallback) {
abortCallback(new AbortError());
passCallback = undefined;
abortCallback = undefined;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move those to after the await new Promise to avoid the repetition? No strong feeling though, if you prefer as it is now, feel free to disregard, I'd personally find it easier to follow if it were down there.

@Linkgoron
Copy link
Member Author

@Linkgoron I think what Michael meant is that you can break inside a for await loop and that calls .return on the async generator.

Oh. I didn't know that! Anyway, I'll update the PR soon.

@Linkgoron Linkgoron force-pushed the timers-set-interval-async branch 2 times, most recently from 4db2941 to bc95c21 Compare January 31, 2021 23:25
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good job. I think this is good enough to land (the module is experimental) and iron out small nits later :]

@targos
Copy link
Member

targos commented Feb 1, 2021

Just a thought: could we easily make the iterated values resolve to the number of iterations?

@benjamingr
Copy link
Member

@targos I would prefer that as well - but James raised the point in the original PR that setTimeout from timers/promises already does this with the value parameter.

@benjamingr benjamingr added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 1, 2021
@nodejs-github-bot
Copy link
Collaborator

@Linkgoron Linkgoron force-pushed the timers-set-interval-async branch 2 times, most recently from 3508413 to a651ce4 Compare February 1, 2021 15:51
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@benjamingr
Copy link
Member

Ping @devsnek @ronag @jasnell @aduh95 any chance this could get a second review to not have to wait a week :]?

@benjamingr
Copy link
Member

Landed in fefc639 🎉

@benjamingr benjamingr closed this Feb 3, 2021
benjamingr pushed a commit that referenced this pull request Feb 3, 2021
Added setInterval async generator to timers\promises.
Utilises async generators to provide an iterator compatible with
`for await`.

Co-Authored-By: Fabian Cook <[email protected]>

fix message

PR-URL: #37153
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Comment on lines +352 to +373
{
// Check that if we abort when we have some callbacks left,
// we actually call them.
const controller = new AbortController();
const { signal } = controller;
const delay = 10;
let totalIterations = 0;
const timeoutLoop = runInterval(async (iterationNumber) => {
if (iterationNumber === 2) {
await setTimeout(delay * 2);
controller.abort();
}
if (iterationNumber > totalIterations) {
totalIterations = iterationNumber;
}
}, delay, signal);

timeoutLoop.catch(common.mustCall(() => {
assert.ok(totalIterations >= 3, `iterations was ${totalIterations} < 3`);
}));
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this test is unreliable and is failing in CI on Raspberry Pi devices. I also can make it fail trivially on my macOS laptop by running tools/test.py -j96 --repeat=192 test/parallel/test-timers-promisified.js. I haven't looked closely (yet) but in my experience, having a magic number delay variable like this in a timers test indicates an assumption that the host isn't so slow that (say) a 10ms second timer won't fire 50ms later. This assumption is, of course, incorrect if the machine has low CPU/memory (like a Raspberry Pi device) or if there are a lot of other things competing for the machine's resources (-j96 --repeat=192).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for causing an issue - I'll take a look and fix it

Copy link
Member Author

@Linkgoron Linkgoron Feb 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When implementing this, I was under the assumption that if we I have an interval and a timeout where the interval starts before a timeout and is supposed to execute before the timeout executes, the interval's callback would always execute before the timeout's callback. Either this assumption was wrong, or something in my setInterval implementation is incorrect. (i.e. I thought that the setTimeout would always run after the internal setInterval)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

danielleadams pushed a commit that referenced this pull request Feb 16, 2021
Added setInterval async generator to timers\promises.
Utilises async generators to provide an iterator compatible with
`for await`.

Co-Authored-By: Fabian Cook <[email protected]>

fix message

PR-URL: #37153
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams added a commit that referenced this pull request Feb 16, 2021
Notable Changes:

* crypto:
  * add keyObject.export() 'jwk' format option (Filip Skokan) #37081
* deps:
  * upgrade to libuv 1.41.0 (Colin Ihrig) #37360
* doc:
  * add dmabupt to collaborators (Xu Meng) #37377
  * refactor fs docs structure (James M Snell) #37170
* fs:
  * add fsPromises.watch() (James M Snell) #37179
  * use a default callback for fs.close() (James M Snell) #37174
  * add AbortSignal support to watch (Benjamin Gruenbaum) #37190
* perf_hooks:
  * introduce createHistogram (James M Snell) #37155
* stream:
  * improve Readable.from error handling (Benjamin Gruenbaum) #37158
* timers:
  * introduce setInterval async iterator (linkgoron) #37153
* tls:
  * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070
This was referenced Feb 16, 2021
danielleadams added a commit that referenced this pull request Feb 17, 2021
Notable Changes:

* crypto:
  * add keyObject.export() 'jwk' format option (Filip Skokan) #37081
* deps:
  * upgrade to libuv 1.41.0 (Colin Ihrig) #37360
* doc:
  * add dmabupt to collaborators (Xu Meng) #37377
  * refactor fs docs structure (James M Snell) #37170
* fs:
  * add fsPromises.watch() (James M Snell) #37179
  * use a default callback for fs.close() (James M Snell) #37174
  * add AbortSignal support to watch (Benjamin Gruenbaum) #37190
* perf_hooks:
  * introduce createHistogram (James M Snell) #37155
* stream:
  * improve Readable.from error handling (Benjamin Gruenbaum) #37158
* timers:
  * introduce setInterval async iterator (linkgoron) #37153
* tls:
  * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070
danielleadams added a commit that referenced this pull request Feb 17, 2021
Notable Changes:

* crypto:
  * add keyObject.export() jwk format option (Filip Skokan) #37081
* deps:
  * upgrade to libuv 1.41.0 (Colin Ihrig) #37360
* doc:
  * add dmabupt to collaborators (Xu Meng) #37377
  * refactor fs docs structure (James M Snell) #37170
* fs:
  * add fsPromises.watch() (James M Snell) #37179
  * use a default callback for fs.close() (James M Snell) #37174
  * add AbortSignal support to watch (Benjamin Gruenbaum) #37190
* perf_hooks:
  * introduce createHistogram (James M Snell) #37155
* stream:
  * improve Readable.from error handling (Benjamin Gruenbaum) #37158
* timers:
  * introduce setInterval async iterator (linkgoron) #37153
* tls:
  * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070
danielleadams added a commit that referenced this pull request Feb 17, 2021
PR-URL: #37406

Notable Changes:

* crypto:
  * add keyObject.export() jwk format option (Filip Skokan) #37081
* deps:
  * upgrade to libuv 1.41.0 (Colin Ihrig) #37360
* doc:
  * add dmabupt to collaborators (Xu Meng) #37377
  * refactor fs docs structure (James M Snell) #37170
* fs:
  * add fsPromises.watch() (James M Snell) #37179
  * use a default callback for fs.close() (James M Snell) #37174
  * add AbortSignal support to watch (Benjamin Gruenbaum) #37190
* perf_hooks:
  * introduce createHistogram (James M Snell) #37155
* stream:
  * improve Readable.from error handling (Benjamin Gruenbaum) #37158
* timers:
  * introduce setInterval async iterator (linkgoron) #37153
* tls:
  * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070
danielleadams added a commit that referenced this pull request Feb 18, 2021
PR-URL: #37406

Notable Changes:

* crypto:
  * add keyObject.export() jwk format option (Filip Skokan) #37081
* deps:
  * upgrade to libuv 1.41.0 (Colin Ihrig) #37360
* doc:
  * add dmabupt to collaborators (Xu Meng) #37377
  * refactor fs docs structure (James M Snell) #37170
* fs:
  * add fsPromises.watch() (James M Snell) #37179
  * use a default callback for fs.close() (James M Snell) #37174
  * add AbortSignal support to watch (Benjamin Gruenbaum) #37190
* perf_hooks:
  * introduce createHistogram (James M Snell) #37155
* stream:
  * improve Readable.from error handling (Benjamin Gruenbaum) #37158
* timers:
  * introduce setInterval async iterator (linkgoron) #37153
* tls:
  * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070
@Linkgoron Linkgoron deleted the timers-set-interval-async branch February 26, 2021 14:27
@targos
Copy link
Member

targos commented Jul 19, 2021

I'm marking this dont-land-on-v14.x because timers/promises is not available on v14.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants