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

‘timersPromises.setInterval()’ doesn’t keep the event loop active #45224

Closed
leafac opened this issue Oct 28, 2022 · 6 comments
Closed

‘timersPromises.setInterval()’ doesn’t keep the event loop active #45224

leafac opened this issue Oct 28, 2022 · 6 comments

Comments

@leafac
Copy link

leafac commented Oct 28, 2022

Version

v18.12.0

Platform

Darwin leafac--mac-mini.lan 21.6.0 Darwin Kernel Version 21.6.0: Mon Aug 22 20:20:05 PDT 2022; root:xnu-8020.140.49~2/RELEASE_ARM64_T8101 arm64

Subsystem

No response

What steps will reproduce the bug?

As far as I understand from having read the documentation, timersPromises.setInterval() was supposed to keep the event loop active, in much the same way that other timers including setInterval() do. That’s why one of the possible options is ref.

But that doesn’t seem to be working.

Here’s an example program:

// index.mjs
import timers from "node:timers/promises";
timers.setInterval(1000);

This program exits immediately. I tried changing the ref option, including setting it to true explicitly, to no effect.

Here’s a similar program using setInterval(), which does keep the event loop active and does not terminate right away:

// index.mjs
setInterval(() => {});

This may seem like a silly program, but sometimes it’s actually useful to have a dummy timer like that to exert more control over the event loop and when it terminates.

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior?

No response

What do you see instead?

Additional information

No response

@mscdex
Copy link
Contributor

mscdex commented Oct 28, 2022

Don't you need to await the returned value in order for the delay to occur?

@theanarkh
Copy link
Contributor

theanarkh commented Oct 28, 2022

timers.setInterval(1000).next() works. The timer is not created until next is executed.

@leafac
Copy link
Author

leafac commented Oct 28, 2022

@mscdex Thanks for the response.

No, I don’t think that awaiting for the promise should make any difference. I think that the timer should have been registered and kept the event loop active, in much the same way that setInterval() does.

Note that the desired behavior I’m describing is what occurs with timers.setTimeout(), even if you don’t await for the promise. The following program, for example, takes one second to terminate:

// index.mjs
import timers from "node:timers/promises";
timers.setTimeout(1000);

@theanarkh Thanks for the response.

Great idea. I can confirm that timers.setInterval(1000).next() does keep the event loop active.

I think it’s a bit weird that it appears as if the timer is only being registered if you query for a value. I think that the timer should have been registered as soon as timers.setInterval() is called.

Do you agree?

Or do you think I’m misunderstanding something?

@theanarkh
Copy link
Contributor

theanarkh commented Oct 28, 2022

Yeah, because this usage is different from global.setInterval, so it may be a bit misleading, but the documentation mentioned that timers.setInterval return an async iterator, so we may need to understand how the async iterator works.

@leafac
Copy link
Author

leafac commented Oct 29, 2022

@theanarkh So I suppose we have an opportunity to either (in order of preference):

  1. Modify timers.setInterval() so that the timer is created and the event loop kept active as soon as possible, even if you don’t try to iterate over the iterator.

  2. Explain this behavior in more detail in the documentation.

What do y’all think we should do?


Also, for what it’s worth, it seems like TypeScript doesn’t know about the fact that you may call .next() directly on the AsyncIterable returned by timers.setInterval(). To make this work in TypeScript I had to jump through the hoop of getting the AsyncIterator underneath:

// index.mts
import timers from "node:timers/promises";
timers.setInterval(1000)[Symbol.asyncIterator]().next();

@theanarkh
Copy link
Contributor

I think we can explain this in more detail in the documentation :-).

nodejs-github-bot pushed a commit that referenced this issue Nov 6, 2022
PR-URL: #45232
Refs: #45224
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
lucshi pushed a commit to lucshi/node that referenced this issue Nov 9, 2022
PR-URL: nodejs#45232
Refs: nodejs#45224
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
RafaelGSS pushed a commit that referenced this issue Nov 10, 2022
PR-URL: #45232
Refs: #45224
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit that referenced this issue Dec 30, 2022
PR-URL: #45232
Refs: #45224
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit that referenced this issue Dec 30, 2022
PR-URL: #45232
Refs: #45224
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit that referenced this issue Jan 3, 2023
PR-URL: #45232
Refs: #45224
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
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

3 participants