More counter fixes #3
Conversation
`count' was both an attribute and a prototype of Counter. You can't have both (count() never worked anyway), and there's not much sense in providing an accessor here, so it's been removed.
This patch adds some additional checking for counters: * Wrap counters around at 2^32. Most graphing software (RRDtool etc.) can deal with this properly. Unfortunately wrapping at 2^64 isn't possible because JS has a maximum integer resolution of 2^53. * Don't allow counters to be decremented below 0.
| @@ -8,16 +14,18 @@ var Counter = module.exports = function Counter() { | |||
|
|
|||
| Counter.prototype.inc = function(val) { | |||
| if (!val) { val = 1; } | |||
| this.count += val; | |||
| if (this.count === MAX_COUNTER_VALUE) { | |||
There was a problem hiding this comment.
Shouldn't this be >= instead of ===? If the counter is incremented by something other than 1, it may skip over this value.
There was a problem hiding this comment.
I see the bug you're talking about, but your suggested fix isn't quite right. It should be something like:
if (val < 1) {
return;
}
if (this.count === MAX_COUNTER_VALUE) {
this.count = -1;
}
this.count += val;There was a problem hiding this comment.
What about the case where this.count is MAX_COUNTER_VALUE - 1, and then we get inc(100)?
> var MAX_COUNTER_VALUE = Math.pow(2, 32)
> MAX_COUNTER_VALUE
4294967296
> var count = MAX_COUNTER_VALUE - 1
> count += 100
4294967395
> count === MAX_COUNTER_VALUE
false
Seem like
> count >= MAX_COUNTER_VALUE
true
Fixes it.
There was a problem hiding this comment.
Your test case was not a good simulation.
In my proposal, the value is temporarily set to -1, not MAX_COUNTER_VALUE - 1. Testing for count >= MAX_COUNTER_VALUE is incorrect because we want to wrapped result to compensate for the delta (e.g., if the previous value was MAX_COUNTER_VALUE and we are incrementing by 10, we want the result to be 9).
* Handle counter wrapping properly when val > 1. * Prevent counter from being decremented below 0.
Counter fixes - overflow on integers > 2^32. Don't decrement below 0.
|
Yeah, this looks better. Thanks guys! |
|
Actually, there's an off-by-one bug here. Please hold off on disting a new release. :) |
See commits for details.