RateLimiting icon indicating copy to clipboard operation
RateLimiting copied to clipboard

ExitTimerCallback has potential flaw

Open LampGenie opened this issue 11 years ago • 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);

LampGenie avatar Nov 20 '13 23:11 LampGenie

If you can provide an example (in gitgist for example) with an failing test case. I'd be happy to look into it.

Danthar avatar Nov 21 '13 17:11 Danthar

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.

LampGenie avatar Nov 21 '13 17:11 LampGenie

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!

Danthar avatar Nov 21 '13 18:11 Danthar

Was this issue resolved?

tinonetic avatar Mar 20 '19 09:03 tinonetic

Nope. But your welcome to make a PR.

Danthar avatar Mar 20 '19 17:03 Danthar