Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG (1.7.34) possible endless loop if Burst rate is reached: WaitForPermittedRequest - "RequestsUsed >= Burst" replenish token (RateLimit.cs) #815

Open
sat-it opened this issue Jan 8, 2025 · 5 comments

Comments

@sat-it
Copy link

sat-it commented Jan 8, 2025

Hello,
first of all, happy new year.
thank you very much for this project and also the other supporters!

maybe it's already in the previous version. everything was ok in 1.7.27.
maybe case is same issue: #812 GetItemOffersBatch seems to be broken with the 1.7.34 update

for example:
getOrder has Burst rate of "20".
if 20 is reached: "WaitForPermittedRequest" - "while (RequestsUsed >= Burst)" in combination with else "//replenish token IncrementAvailableTokens(debugMode)" => endless loop
*** documented in the code below
maybe issue is in "IncrementAvailableTokens(debugMode)"

Can anybody fix it?

Thanks,
Marco

RateLimit.cs


  1. WaitForPermittedRequest
    public async Task WaitForPermittedRequest(CancellationToken cancellationToken = default, bool debugMode = true)
    {
    if (RequestsUsed < 0)
    {
    RequestsUsed = 0;
    }

    int ratePeriodMs = GetRatePeriodMs();
    var requestsUsedAtOnset = RequestsUsed;

    // when requests used more than zero, replenish according to time elapsed
    IncrementAvailableTokens(debugMode);

    var nextRequestsSent = RequestsUsed + 1;
    var nextRequestsSentTxt = (nextRequestsSent > Burst) ? "FULL" : nextRequestsSent.ToString();

    WriteDebug($"[RateLimits ,{this.RateLimitType,15}]: {DateTime.UtcNow.ToString(),10}\t Request/Burst: {nextRequestsSentTxt}/{Burst}\t Rate: {Rate}/{ratePeriodMs}ms", debugMode);

    var requestIsPermitted = false;

    while (!requestIsPermitted)
    {
    // if bucket is currently empty, enter wait loop for replenishment
    while (RequestsUsed >= Burst)
    {
    ***!!! endless loop if RequestsUsed is >=20 !!!!

          var currentRequestsUsed = RequestsUsed;
         // wait until predicted next token available
         var incomingRequestTokenTime = LastRequestReplenished.AddMilliseconds(ratePeriodMs);
         if (incomingRequestTokenTime > DateTime.UtcNow)
         {
             WriteDebug($"Next token expected at {incomingRequestTokenTime}, waiting", debugMode);
             await Task.Delay(100);
             continue;
         }
         else
         {
             // replenish token
             IncrementAvailableTokens(debugMode);
    	       
         }
    
         if (RequestsUsed <= 0)
         {
             RequestsUsed = 0;
             break;
         }
    
    ***!!!  endless loop if RequestsUsed is >=20  !!!!
     }
    
     // now remove token from bucket for pending request
     requestIsPermitted = TryDecrementAvailableTokens(debugMode);
    

    }
    }


  1. IncrementAvailableTokens(bool isDebug)
    // increments available tokens, unless another thread has already incremented them.
    private void IncrementAvailableTokens(bool isDebug)
    {
    WriteDebug($"Attempting to increment tokens", isDebug);
    lock (_locker)
    {
    var ratePeriodMilliseconds = GetRatePeriodMs();
    var requestsToReplenish = ratePeriodMilliseconds == 0 ? 0 : (DateTime.UtcNow - LastRequestReplenished).Milliseconds / ratePeriodMilliseconds;
    WriteDebug($"{requestsToReplenish} tokens to replenish since {LastRequestReplenished}", isDebug);
    if (requestsToReplenish == 0 || RequestsUsed == 0)
    {
    return;
    }

             RequestsUsed = Math.Max(RequestsUsed - requestsToReplenish, 0);
             LastRequestReplenished = DateTime.UtcNow;
    
             WriteDebug($"Incremented tokens by {requestsToReplenish}", isDebug);
         }
     }
    
@abuzuhri
Copy link
Owner

abuzuhri commented Jan 8, 2025

Please take look to this change @sat-it , see if you can fix other wise maybe we will rollback this change

#802

@abuzuhri
Copy link
Owner

abuzuhri commented Jan 8, 2025

@FinnReilly We need urgent recheck on this

@FinnReilly
Copy link

@abuzuhri have submitted a PR - have included tests to demonstrate that the fixed code works as intended

@pandapknaepel
Copy link

pandapknaepel commented Jan 20, 2025

Hi,

I was wondering why my production Order API has been running at over 90% CPU usage for days and stopped executing threads entirely. I’ve been trying to track down the issue for several days and finally found the root cause.

The problem seems to be related to extreme memory allocation in the Small Object Heap, caused predominantly by String objects. This issue appears to originate from the rate-limiting mechanism (WaitForPermittedRequest and IncrementAvailableTokens), which seems to lead to excessive string allocations.

Here is an example from our diagnostics (rider):

Small Object Heap: code that allocates a lot of memory in SOH
Allocated object type: String
Last observation: 20.01.2025 10:58 Panda.Api.Order
Allocated size: 60,724.3 MB

at String.Ctor(ReadOnlySpan)
at Span<Char>.ToString()
at String.FormatHelper(IFormatProvider, String, ReadOnlySpan)
at String.Format(String, Object, Object)
at RateLimits.IncrementAvailableTokens(bool)
at RateLimits.WaitForPermittedRequest(CancellationToken, bool)
at AsyncMethodBuilderCore.Start(ref )
at AsyncTaskMethodBuilder.Start(ref )
at RateLimits.WaitForPermittedRequest(CancellationToken, bool)
at RateLimitingHandler.WaitForLimitTypeAsync(AmazonCredential, RateLimitType, CancellationToken)
at AsyncMethodBuilderCore.Start(ref )
at AsyncTaskMethodBuilder.Start(ref )
at RateLimitingHandler.WaitForLimitTypeAsync(AmazonCredential, RateLimitType, CancellationToken)
at RateLimitingHandler+<SafelyExecuteRequestAsync>d__2<__Canon>.MoveNext()
at AsyncMethodBuilderCore.Start(ref )
at AsyncTaskMethodBuilder<__Canon>.Start(ref )
at RateLimitingHandler.SafelyExecuteRequestAsync(IRestClient, RestRequest, AmazonCredential, RateLimitType, Action, CancellationToken)
at RequestService+<ExecuteRequestTry>d__41<__Canon>.MoveNext()
at AsyncMethodBuilderCore.Start(ref )
at AsyncTaskMethodBuilder<__Canon>.Start(ref )
at RequestService.ExecuteRequestTry(RateLimitType, CancellationToken)
at RequestService+<ExecuteRequestAsync>d__45<__Canon>.MoveNext()
at AsyncMethodBuilderCore.Start(ref )
at AsyncTaskMethodBuilder<__Canon>.Start(ref )
at RequestService.ExecuteRequestAsync(RateLimitType, CancellationToken)
at OrderService.GetOrderAsync(ParameterGetOrder, CancellationToken)
at AsyncMethodBuilderCore.Start(ref )
at AsyncTaskMethodBuilder<__Canon>.Start(ref )
at OrderService.GetOrderAsync(ParameterGetOrder, CancellationToken)
at AmazonConnectionService.GetOrderAsync(String, CancellationToken) in /Users/work/Projects/dotnet/services/Panda.Api.Order/Panda.Api.Order.Services/Amazon/AmazonConnectionService.cs:line 36 column 9
at AsyncMethodBuilderCore.Start(ref )
at AmazonConnectionService.GetOrderAsync(String, CancellationToken)

...

This behavior only occurs with version 1.7.34. With version 1.7.33, this problem does not happen. Based on the stack trace, it seems related to the rate-limiting logic and might also align with the potential endless loop described in this ticket.

This is a critical issue for us, as it completely destabilizes our system. Please let me know if additional details are needed or if there’s any recommended workaround for now.

@abuzuhri
Copy link
Owner

@FinnReilly I really hope we can do rollback for that change soon and later we can take deep look on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants