STRML / node-toobusy

Don't let your Node.JS server fall over when it's too busy.
366 stars 36 forks source link

Using process.htrime instead of Date.now to avoid clock drifts. #19

Closed Kasher closed 7 years ago

amitkanfer commented 7 years ago

I would like this PR to be committed very much!!

amitkanfer commented 7 years ago

@STRML The advantage of this PR is that hrtime is not subject of clock drifts. If the machine has NTP running for example - you might get false positives. You'll also get false positives twice a year when daylight savings time kicks in.

Makes sense?

Kasher commented 7 years ago

@STRML - Fixed the PR. My bad. As @amitkanfer , new Date() may suffer from false positives due to clock drifts, while hrtime may not. Here is a nice article about this: https://blog.tompawlak.org/measure-execution-time-nodejs-javascript

STRML commented 7 years ago

I don't disagree that process.hrtime() is a more accurate way to make sub-ms relative measurements - but we're not making sub-ms measurements, and I need to set the facts straight.

"Clock drift" is a phenomenon that affects two different clocks, not the same one. A clock cannot drift relative to itself.

What could happen is an NTP adjustment, but the Linux kernel limits that to 500PPM, or 0.5ms, per second. That is barely enough to cause any actual difference in toobusy() under any reasonable settings.

Date.now() is time since the unix epoch in milliseconds. Daylight savings time does not affect it.

So the only thing this PR gets us is slightly better resolution, but we don't need the resolution as all of our units are in milliseconds. So unless there is a better argument for it, I'm going to close this as I'd rather not have people make the wrong assumptions about how clocks work by reading this code.

Edit: To drive home the point, we are rounding to the nearest millisecond in two places which makes ns precision useless.