util: add util.sleep() #30784
util: add util.sleep() #30784
Conversation
Notable changes: - Fix handling of large files in `uv_fs_copyfile()`. Fixes: nodejs#30085 - Fix Android build errors. - uv_sleep() has been added. - uv_interface_addresses() IPv6 netmask support has been fixed. Fixes: nodejs#30504 - uv_fs_mkstemp() has been added.
nodejs-github-bot
added
the
lib / src
|
is there motivation for util.sleep? since we have timeouts it seems like more of an antipattern to me. |
|
those don't need it to be exposed on util though right? |
|
I would much rather keep this as an internal function only. |
Trott
added
the
semver-minor
|
Since there's hesitation about exposing this, maybe this can go in as an internal-only function first, used by recursive |
| @@ -161,6 +161,12 @@ static void SetHiddenValue(const FunctionCallbackInfo<Value>& args) { | |||
| args.GetReturnValue().Set(maybe_value.FromJust()); | |||
| } | |||
|
|
|||
| static void Sleep(const FunctionCallbackInfo<Value>& args) { | |||
There was a problem hiding this comment.
This should have an env->PrintSyncTrace() call, imo
|
Isn't the need of a busy loop for some of the tests an indication of a valid use case? Someone could use if for the same purpose such as testing how an app would behave when performing a CPU intensive task. |
|
@cjihrig don't blame me...my ugly versions (that largely justifies util.sleep...) of sleep /**
* Sleep for time [msec]
* @param time int milliseconds
* @param block Block Delayed block
* @usage
sleep( 5 * 1000, function() {
// executes after 5 seconds, and blocks the (main) thread
});
*/
sleep: function (time, block) {
var stop = new Date().getTime();
while (new Date().getTime() < stop + time) {
;
}
block();
},
/**
* Sleep for time [msec]
* @param time int milliseconds
* @return Promise delayed resolve
* @usage
sleep(1000*3).then(()=>console.log("awake"))
*/
sleepP: function (time) {
return new Promise((resolve, reject) => {
var stop = new Date().getTime();
while (new Date().getTime() < stop + time) {
;
}
return resolve(true)
});
},My use case was that, despite of using throttling functions and exponential backoff approaches, I had to save some idle time before processing the next chunk... |
|
@loretoparisi Why was your use case not fulfilled by |
|
@addaleax yes I know...because I'm running CPU intensive tasks in parallel, and I need to save a bit cpu time among processes, that NOTE. so to be clear I call those |
I am confused. Your code actively uses CPU time instead of saving it, and blocks the main thread. |
|
I think we should have a public promisified wrapper around |
|
@tniessen right, I need to block that process every N steps let's say, while running M processes in parallel with /**
* Promise All
* @param block Function (item:object,index:int,resolve:Promise.resolve,reject:Promise.reject)
* @param timeout float Race timeout in milliseconds
* @return Promise
*/
promiseAllP: function (items, block) {
var promises = [];
items.forEach(function (item, index) {
promises.push(function (item, i) {
return new Promise(function (resolve, reject) {
return block.apply(this, [item, index, resolve, reject]);
});
}(item, index))
});
return Promise.all(promises);
}, //promiseAllPso in the |
|
I'm Ok with this as an internal utility but have significant concerns around a public api that allows intentionally blocking the main event loop thread for arbitrarily long periods of time. Would it be reasonable to enforce a conservative upper limit to the sleep time? |
|
I do see the reason for wanting a sleep function, which doesn't schedule a microtask or interacts with the eventloop to achieve more precise timing in use cases like implementing a lower level protocol (wether it would be wise to implement this in js with node is another discussion), which isn't influenced by other parts of the code and acts more or less atomic. But I wouldn't recommend exposing this via an core package since this would send the wrong message and people will start using util.sleep in ways it shouldn't be no matter the documentation around it. Also capping such a function at same point to have a "max sleep time" wouldn't be wise, since this would create unexpected behavior when using utils.sleep. Also at what point in time you don't need this type of sleeping anymore? If you break too early, you'd miss the point, too late and people will start using it at the wrong places. Overall I think that it's dangerous to plubicly embrace halting the event loop by publishing such a function and recommend to leave the edgecases to the already provided (ugly) JS workarounds. If util.sleep gets added, I recommend adding examples of better solutions for sleeping to its documentation. |
That would be nice to have, but It's worth noting that wouldn't actually be synchronous.
That is the whole point, but we also have worker threads these days.
I don't see a point to that TBH.
I'm fine with adding a big warning in the documentation. Ultimately, this isn't a hill I'm going to die on. I do think it's interesting that most other languages have some implementation of this though. |
|
Yes, other languages have it, and all share the same concerns around it. As I said, I've no issues with an internal utility. I'm not yet convinced that a public api is a good thing but I won't block -- I would only ask that strong warnings be placed in the documentation along with good examples of how it should be used. |
The thing is, that setTimeout is the JS way of doing exactly this. |
|
If people are really bothered about it being misused, perhaps a scary long-winded name? Not all Node.js programs are servers. For certain kinds of programs, blocking the main thread isn't a problem. I haven't written a lot of non-server Node.js code, but I can think of several times writing non-server code in other languages/environments where a short I've also corrected more than a dozen people on SO writing the CPU-burning busywait version of So, mild thumb's up on the idea here, FWIW. Perhaps with a silly name. :-) (But for me, |
|
@cjihrig tweeted about this PR asking for usecases Tweet and the responses show, that people think this sleep function would either be Promise or event based and would use it e.g. to wait for memory cleanup. function sleep(amount){
return new Promise(resolve => setTimeout(resolve, amount));
}I strongly support @tjcrowder 's idea of giving it another name to show, that it's an antipattern. Sidenote: |
|
consuming rate limited APIs currently requires a ton of orchestration if your code is "faster" than the rate limit. A sync sleep would be perfect for our use case. |
|
@Snapstromegon - Thanks. Since |
|
@tjcrowder - I interpreted sleep as an execution-suspension and under that point it would be okay to avoid the antipattern. But I can live with any name, as long as it's clearly stated in the docs how bad a blocking sleep can be and why it's an antipattern / how to avoid it. |
|
@abualsamid That sounds like a larger issue and not something that would ideally be solved with synchronous sleeps, honestly. Maybe to put it this way, what is a use case solved by But yes, if we do add this, then I’d really be in favour of a name that makes clear that using |
|
Name suggestion: The advantage to (It would also argue for the Promise-based Counter-argument: This pattern is used in the callback APIs, but |
This seems like a problem which is solveable with a small existing npm package or with about 30 LoC for a simple solution, which limits the call to a function by delaying each future call by some time. Nevertheless I support @tjcrowder by going with the postfix "Sync". I could also see |
This applies to almost everything in util :) I solved the problem, but I am in favor of a util method that helps in the future. |
|
I think we should focus on @addaleax's question:
|
|
Starting with internal-only doesn't preclude exposing it at a later date so I'd suggest we go ahead and do that to get it moving (and fix the problem in recursive |
|
https://www.npmjs.com/package/sleep |
|
Isn't it scary to know you import a third party library and it can totally hang your event loop in some corner case? |
|
@mantou132 OMG, I was going to mention |
|
My final $0.02, FWIW: I think the arguments for making it part of the API rather than hiding it away are much stronger than those for hiding it away:
It's not a function that will require much maintenance, it's very much write, document, and publish, then it just sit unobtrusively in the corner doing its job. |
|
@tjcrowder Thank you for the detailed summary! I think what you wrote is factually correct, but your arguments lead me to the opposite conclusion:
It only matters for the sync version, right? Which is generally an anti-pattern, and the package itself says: "But that's not so good. Use the async API. It's better." So we are talking about helping to implement anti-patterns again.
There was enough desire, but is there need? I don't know. Which brings us back to the question that @addaleax posed, whether there are legitimate use cases (that don't promote anti-patterns).
I don't think efficiency should be a primary concern when literally putting the entire application to a hold. If you want things to be efficient, don't sleep the main thread. From my perspective,
True, but that is bound to happen whether we have
That's right, but that is only necessary if we add the API. So we are on the same page here, the API is "a bad idea in most cases" and using it is "potentially problematic". And if that's the case, then having |
|
@Trott I don't want you to think I'm ignoring your comment about making this internal only. I'm going to implement that in the |
|
@tniessen - I thought it might be useful in that way, too. :-)
I think given 28.8k/week downloads, clearly there's a need.
It wasn't meant to be, I should have just left that parenthetical off. It doesn't put the entire app on hold, just one thread, but I can't imagine efficiency being a factor regardless.
In a sense, and if so, shouldn't cjihrig use it rather than adding a new internal? If it's good enough for userland code, then...? It's great it works even on the main thread. Is there any guarantee it won't be disallowed on the main thread at some point as it is in browsers (as is hinted at here)?
The common point of 4, 5 and 6 was that without it being in the API, the programmer will find another way to do it (such as I thought 6 was a particularly good point: having one official way of doing this makes linting for it much easier. Anyway, that's more than enough input from me! Thanks to all of you on the Node.js team for all the work you do! |
I mean, there aren’t really any guarantees but I’d certainly be against it. |
|
I understand your point, @tjcrowder! :)
I thought about that, too, and I agree that it sounds like a good idea if the alternative is a native binding or an internal API. |
|
I want to clarify that I was joking in that IRC log, I am also against disabling Atomics.wait in the main thread. |
|
I'm going to close this, as I don't feel strongly enough about it to argue. If anyone wants to take this over, feel free. |
|
@cjihrig I didn't get if the point was not blocking node main loop or that it was just another node-ist and c-ist holy war about |
The point is that we haven't found any legitimate use cases. If you have one, we will happily consider adding this API. |
The language does provide it (in the standard library defined in the spec): The question here was whether it was appropriate to provide a Node.js-specific API method to do it. After some discussion, the decision (at least, how I read it) was: No, if someone wants to do that, they can use |
|
@tjcrowder thank you, I was referring to NodeJS. I have actually never used ECMA17 |
|
@loretoparisi The relevant thing is that it's supported in Node.js. :-) (But re support on browsers:
) |
Sometimes setTimeout is an anti-pattern. When you're not doing oldschool callback style code everywhere. It's better to halt the entire execution in an async function rather than split it off into two streams using setTimeout. Thus simplifying the control logic. |
Yes in fact if your writing style is in |
Totally agree. It's a more modern design approach. I suppose they are afraid of adding something browser's don't have. But I say let them catch up. |
The first commit is the libuv 1.34.0 upgrade (#30783).
The second commit adds a
util.sleep()function.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes