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

util: add util.sleep() #30784

Closed
wants to merge 2 commits into from
Closed

util: add util.sleep() #30784

wants to merge 2 commits into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Dec 4, 2019

The first commit is the libuv 1.34.0 upgrade (#30783).

The second commit adds a util.sleep() function.

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

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 nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 4, 2019
@devsnek
Copy link
Member

devsnek commented Dec 4, 2019

is there motivation for util.sleep? since we have timeouts it seems like more of an antipattern to me.

@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 4, 2019

is there motivation for util.sleep?

A couple of things I had in mind were: #30785 and #30787. This is definitely not something I'd use in server code, but I think it has use cases.

@devsnek
Copy link
Member

devsnek commented Dec 4, 2019

those don't need it to be exposed on util though right?

@mscdex
Copy link
Contributor

mscdex commented Dec 4, 2019

I would much rather keep this as an internal function only.

@Trott Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 4, 2019
@Trott
Copy link
Member

Trott commented Dec 4, 2019

Since there's hesitation about exposing this, maybe this can go in as an internal-only function first, used by recursive rmdir and (with // Flags: --expose-internals) in the small number of tests that need it. We can then later expose it as an experimental API if there's a compelling reason to make it available to end users.

@@ -161,6 +161,12 @@ static void SetHiddenValue(const FunctionCallbackInfo<Value>& args) {
args.GetReturnValue().Set(maybe_value.FromJust());
}

static void Sleep(const FunctionCallbackInfo<Value>& args) {
Copy link
Member

Choose a reason for hiding this comment

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

This should have an env->PrintSyncTrace() call, imo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with that.

@santigimeno
Copy link
Member

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.

@loretoparisi
Copy link

loretoparisi commented Dec 4, 2019

@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...

@addaleax
Copy link
Member

addaleax commented Dec 4, 2019

@loretoparisi Why was your use case not fulfilled by setTimeout() here? That might be the interesting bit. In particular, having a promisified version of sleep in your code seems a bit surprising?

@loretoparisi
Copy link

loretoparisi commented Dec 4, 2019

@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 setTimeout cannot do without explicitly hanging the main thread...repeat my self it's terribile, but at the end it worked so far :) cannot wait the get util.sleep. Also, I did not tried to replace all of this with node worker threads, maybe that would work properly :)

NOTE. so to be clear I call those sleepP within a Promise chain. The effect is that for a given huge file to parse make the jobs.

@tniessen
Copy link
Member

tniessen commented Dec 4, 2019

save a bit cpu time among processes, that setTimeout cannot do without explicitly hanging the main thread

I am confused. Your code actively uses CPU time instead of saving it, and blocks the main thread. setTimeout would save CPU time and would not block the thread.

@targos
Copy link
Member

targos commented Dec 4, 2019

I think we should have a public promisified wrapper around setTimeout instead.
Maybe require('timers').wait(ms) ?

@loretoparisi
Copy link

loretoparisi commented Dec 4, 2019

@tniessen right, I need to block that process every N steps let's say, while running M processes in parallel with Promise.all. I actually run this with other functions, that maybe gives you more details like this one:

    /**
     * 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);
    }, //promiseAllP

so in the block I call sleepP. By the way, just wanted to say: welcome to util.sleep ❤️
Thank you guys!

@jasnell
Copy link
Member

jasnell commented Dec 4, 2019

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?

@Snapstromegon
Copy link

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.

@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 4, 2019

I think we should have a public promisified wrapper around setTimeout instead.

That would be nice to have, but It's worth noting that wouldn't actually be synchronous.

significant concerns around a public api that allows intentionally blocking the main event loop thread for arbitrarily long periods of time

That is the whole point, but we also have worker threads these days.

Would it be reasonable to enforce a conservative upper limit to the sleep time?

I don't see a point to that TBH.

If util.sleep gets added, I recommend adding examples of better solutions for sleeping to its documentation.

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.

@jasnell
Copy link
Member

jasnell commented Dec 4, 2019

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.

@Snapstromegon
Copy link

Snapstromegon commented Dec 4, 2019

I do think it's interesting that most other languages have some implementation of this though

The thing is, that setTimeout is the JS way of doing exactly this.
The JS language architecture doesn't intend anything to block the event loop doing nothing.

@tjcrowder
Copy link

tjcrowder commented Dec 4, 2019

If people are really bothered about it being misused, perhaps a scary long-winded name? blockEventLoop, blockEventLoopAndSleep, sleepWhileBlockingEventLoop, suspendAllWorkOnThread ... :-) (Combined with a scary warning in the documentation.) Or even more like React's dangerouslySetInnerHTML: sleepKnowingTheConsequences. (Just a joke: sleepAndToHellWithTheEventLoop.)

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 sleep was a reasonable and appropriate solution to a problem (as in #30785).

I've also corrected more than a dozen people on SO writing the CPU-burning busywait version of sleep in JavaScript. Given that, it seems unlikely to me that people aren't writing it in Node.js code in the wild...

So, mild thumb's up on the idea here, FWIW. Perhaps with a silly name. :-) (But for me, sleep is just fine.)

@Snapstromegon
Copy link

@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.
The respondends seem to like the idea of having a util like this:

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.
Maybe we could provide util.sleep with an non blocking implementation and a blocking one with another name.

Sidenote:
blockEventLoop is not a great name, since also the microtask queue and other execution of the same event gets blocked. Maybe something like blockThread? - naming this is hard.

@abualsamid
Copy link

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.
Specifically we are pulling data from one source (database) and sending it to another (rate limited API). There are a lot of moving parts but I will spare you the details.
What is a straight forward implementation in most languages, turned out to be a mess in node.

@tjcrowder
Copy link

tjcrowder commented Dec 4, 2019

@Snapstromegon - Thanks. Since sleep is an established name for the standard thread-suspension function in multiple environments, I wouldn't use it for the Promise-based wrapper around setTimeout, even if not using it for the established function.

@Snapstromegon
Copy link

@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.

@addaleax
Copy link
Member

addaleax commented Dec 4, 2019

@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 util.sleep(ms) that wouldn’t be much better solved by await timers.wait(ms)? Is there any?

But yes, if we do add this, then I’d really be in favour of a name that makes clear that using .sleep() is almost certainly a bad idea.

@tjcrowder
Copy link

Name suggestion: waitSync

The advantage to waitSync is that the Node.js community is already well aware of the xyz vs. xyzSync thing and the message about not using xyzSync in server code, etc. is already pushed in the documentation and elsewhere. The same reasons apply to wait/waitSync.

(It would also argue for the Promise-based wait in util rather than timers, because waitSync wouldn't make sense in timers.)

Counter-argument: This pattern is used in the callback APIs, but wait would be Promise-based.

@Snapstromegon
Copy link

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.
Specifically we are pulling data from one source (database) and sending it to another (rate limited API). There are a lot of moving parts but I will spare you the details.
What is a straight forward implementation in most languages, turned out to be a mess in node.

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 sleep and sleepSync as viabel names.

@abualsamid
Copy link

This seems like a problem which is solveable with a small existing npm package or with about 30 LoC.

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.

@tniessen
Copy link
Member

tniessen commented Dec 4, 2019

I think we should focus on @addaleax's question:

what is a use case solved by util.sleep(ms) that wouldn’t be much better solved by await timers.wait(ms)?

Copy link
Contributor

@mscdex mscdex left a comment

Choose a reason for hiding this comment

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

Making my concerns more official for the current state of the PR

@Trott
Copy link
Member

Trott commented Dec 5, 2019

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 rmdir()).

@mantou132
Copy link

https://www.npmjs.com/package/sleep

function msleep(n) {
  Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, n);
}
function sleep(n) {
  msleep(n*1000);
}

@reshadi
Copy link

reshadi commented Dec 5, 2019

Isn't it scary to know you import a third party library and it can totally hang your event loop in some corner case?
why not:
const sleep = require('util').promisify(setTimeout);
....
await sleep(1000); //Instead of blocking the whole thread
....

@tjcrowder
Copy link

@mantou132 OMG, I was going to mention Atomics.wait but I assumed Node.js disallowed it on the main thread, like browsers do (e.g., that it would only work on worker threads). But...it doesn't. Wow.

@tjcrowder
Copy link

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:

  1. @cjihrig needed a sleep function for code that one could see needing to write in userland (indeed, there was/is a rimraf package, just without the retry delay in the sync version).
  2. There was enough need for a sleep function to justify an npm package adding a C++ module to do it which has 28.8k weekly downloads.
  3. You can sleep the main thread (less efficiently) via Atomics.wait (which that npm package promotes as an alternative).
  4. People who don't "get" why you shouldn't suspend the thread in most cases will happily write the CPU-burning busywait, not understanding why that is a bad idea, too.
  5. If the function is in the API, the documentation can educate (some of) those who don't realize it's a bad idea in most cases.
  6. If the function is in the API, linters can watch for it and flag up its use as potentially problematic.

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.

@tniessen
Copy link
Member

tniessen commented Dec 5, 2019

@tjcrowder Thank you for the detailed summary! I think what you wrote is factually correct, but your arguments lead me to the opposite conclusion:

  1. @cjihrig needed a sleep function for code that one could see needing to write in userland (indeed, there was/is a rimraf package, just without the retry delay in the sync version).

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.

  1. There was enough need for a sleep function to justify an npm package adding a C++ module to do it which has 28.8k weekly downloads.

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).

  1. You can sleep the main thread (less efficiently) via Atomics.wait (which that npm package promotes as an alternative).

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, Atomics.wait means that Node.js already provides a "solution". That solution might not be intuitive or trivial to use, but that should make people think twice about whether they want to use it.

  1. People who don't "get" why you shouldn't suspend the thread in most cases will happily write the CPU-burning busywait, not understanding why that is a bad idea, too.

True, but that is bound to happen whether we have util.sleep or not. It likely happens in just about every programming language, no matter whether the language / runtime provides means of putting a thread to sleep.

  1. If the function is in the API, the documentation can educate (some of) those who don't realize it's a bad idea in most cases.
  2. If the function is in the API, linters can watch for it and flag up its use as potentially problematic.

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 Atomics.wait is probably good enough.

@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 5, 2019

@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 common.busyLoop() PR. I don't think it makes a ton of sense to do in this PR since it would essentially result in adding an internal API that isn't being used at all 😄

@tjcrowder
Copy link

@tniessen - I thought it might be useful in that way, too. :-)

  1. ...

There was enough desire, but is there need?

I think given 28.8k/week downloads, clearly there's a need.

  1. ...

I don't think efficiency should be a primary concern...

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.

From my perspective, Atomics.wait means that Node.js already provides a "solution".

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)?

4., 5., 6. ...

That's right, but that is only necessary if we add the API.

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 Atomics.wait or worse, a busywait) without being educated by anyone why that would probably be a bad idea.

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!

@addaleax
Copy link
Member

addaleax commented Dec 5, 2019

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)?

I mean, there aren’t really any guarantees but I’d certainly be against it.

@tniessen
Copy link
Member

tniessen commented Dec 5, 2019

I understand your point, @tjcrowder! :)

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...?

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.

@devsnek
Copy link
Member

devsnek commented Dec 5, 2019

I want to clarify that I was joking in that IRC log, I am also against disabling Atomics.wait in the main thread.

@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 7, 2019

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 cjihrig closed this Dec 7, 2019
@cjihrig cjihrig deleted the sleep branch December 7, 2019 03:20
@loretoparisi
Copy link

loretoparisi commented Dec 12, 2019

@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 sleep. My simple opinion is that language primitive should be provided, then programmers can use it or not. This applies to the amazing node threads as well. At some point this was needed in the browser, and now it is in node. But node it's not the browser, and there could be patterns where programmers could safely call sleep if they may need it.

@tniessen
Copy link
Member

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 sleep.

The point is that we haven't found any legitimate use cases. If you have one, we will happily consider adding this API.

@tjcrowder
Copy link

tjcrowder commented Dec 12, 2019

@loretoparisi -

My simple opinion is that language primitive should be provided...

The language does provide it (in the standard library defined in the spec): Atomics.wait. (See example sleep/msleep here.)

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 Atomics.wait.

@loretoparisi
Copy link

@tjcrowder thank you, I was referring to NodeJS. I have actually never used ECMA17 Atomics in the browser in practice since the support is only Chrome/Firefox.

@tjcrowder
Copy link

@loretoparisi The relevant thing is that it's supported in Node.js. :-)

(But re support on browsers:

  • Firefox doesn't support it at the moment [because they disabled SharedArrayBuffer and haven't re-enabled it (yet?)].
  • Chrome and Chromium do, as you said.
  • Safari does (desktop, not iOS).
  • Brave (a fork of Chrome) does.
  • Edge will next month when the V8 version is out (it works in beta).

)

@Turbine1991
Copy link

Turbine1991 commented Apr 3, 2023

Member

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.

@loretoparisi
Copy link

Member

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 async/await there's no doubt that a await util.sleep(5) has the advantage of readability first and it simplifies therefore the control logic as well!
That said I have lines of code using oldschool wait s that I'm not going to change at all! 🙃

@Turbine1991
Copy link

Member

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 async/await there's no doubt that a await util.sleep(5) has the advantage of readability first and it simplifies therefore the control logic as well! That said I have lines of code using oldschool wait s that I'm not going to change at all! upside_down_face

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.