ExitTimerCallback has potential flaw
LampGenie opened this issue · 5 comments
After examining this model, I was able to cause:
Parameter name: dueTime:
at System.Threading.Timer.Change(Int32 dueTime, Int32 period)
at ExitTimerCallback(Object state)
I believe items in your queue may expire between peeks, causing the unchecked value to go negative. You may want to compare it to logic that simply uses the last known unexpired exittime:
int exitTime;
var exitTimeValid = _exitTimes.TryPeek(out exitTime);
while (exitTimeValid) {
if (unchecked(exitTime - Environment.TickCount) > 0) {
break;
}
_semaphore.Release();
_exitTimes.TryDequeue(out exitTime);
exitTimeValid = _exitTimes.TryPeek(out exitTime);
}
// we are already holding the next item from the queue, do not peek again
// although this exit time may have already passed by this stmt.
var timeUntilNextCheck = exitTimeValid
? Math.Max(0, exitTime - Environment.TickCount)
: TimeUnitMilliseconds;
_exitTimer.Change(timeUntilNextCheck, -1);
If you can provide an example (in gitgist for example) with an failing test case. I'd be happy to look into it.
The example is actually the current codebase. To cause the first flaw: force/fake Environment.TickCount to cycle back to Int.MinValue at RateGate.cs [line 110]. This happens every 24.9 days as .NET resets the TickCount.
To cause the second flaw: Set exitTime to anything less than Environment.TickCount at line 112 (before execution). This can happen if the runtime has paused the program between lines 104 to 112. (set breakpoint, wait for exitTime at 111 to be past-due).
So, no special code, just debugging & settting/observing variables. These are issues with state over time.
Once you examine it, hopefully you'll see how my proposed change (above) alleviates the above two issues.
I've also contacted Jack, the original author.
Cheers.
Either one of these scenarios causes the reported exception.
Ok, I spotted the potential race-condition as well. I've marked this issue. And ill look to fix this somewhere this weekend.
Thx for your input!
Was this issue resolved?
Nope. But your welcome to make a PR.