peterkhayes/rolling-rate-limiter

Patch version 0.2.12 introduces a breaking change

ckcr4lyf opened this issue · 5 comments

ckcr4lyf commented

The new version has an updated method to calculate timestamps: 6e6f214#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80R279

The final resultant current timestamp value is significantly different to the previous one. (Almost ~2000x smaller).

Current:

> hr = process.hrtime()
[ 791879, 55186464 ]
> (hr[0] * 1e9 + hr[1]) / 1e3
791879055186.464

Previous (loaded up the library functions already):

> delta = milliseconds * Math.pow(10, 3) - toMicroseconds(hrtime);
1637029532854004

On its own, from a "fresh start" this would seem fine. But in my use case, the rate limiter is based on redis and is separate from the node application. As a result, after this update, all rate limit checks failed, since at https://github.com/peterkhayes/rolling-rate-limiter/blob/master/src/index.ts#L109 , the calculated value ends up being negative.

This is due to redis storing an event from a previous "block" with the older (and numerically much larger) timestamp.

I can't really think of a good way to fix this, since there is no versioning in the redis entry, to actively convert from one format to the other.

Reverting #60 would fix it in some specific use cases, but of course it would mean someone on the newer timestamp who is relying on blocking on minDifference would face difficulties too...

peterkhayes commented

Thanks for identifying this. I apologize for the unintentional breaking change, and I should have verified that the PR removing the microtime library produced the same keys. I'll think about how to fix this; maybe some sort of "compatibility mode" flag that can detect timestamps with the wrong format and convert them?

peterkhayes commented

Oh wow - it's even worse. process.hrtime does not return a consistent value between different session runs. It's relative to "an arbitrary point in time". I'm going to revert now. I don't think there's any way to fix existing values.

peterkhayes commented

I reverted to use a library again and published as 0.2.13. The reason I didn't make a new minor version is that I don't want people to update to 0.2.12 by mistake.

Once again I'm sorry about this.

  • 👍2
huksley commented

oops thats unfortunate!
sorry my bad, I am original PR creator to use hrtime()

ckcr4lyf commented

Thanks for acknowledging and reverting the change. Cheers

  • 👍2