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

sleep not work #10

Open
leonardyrj opened this issue Oct 13, 2023 · 6 comments
Open

sleep not work #10

leonardyrj opened this issue Oct 13, 2023 · 6 comments

Comments

@leonardyrj
Copy link

I added sleep according to the manual so it doesn't throw an exception and it's still throwing it. It's falling into this if.

protected function resolveLimits(): array { return [ Limit::allow(60)->everyMinute()->sleep() ]; }

` protected function handleExceededLimit(Limit $limit, PendingRequest $pendingRequest): void
{
if (! $limit->getShouldSleep()) {
$this->throwLimitException($limit);
}

    $existingDelay = $pendingRequest->delay()->get() ?? 0;
    $remainingMilliseconds = $limit->getRemainingSeconds() * 1000;

    $pendingRequest->delay()->set($existingDelay + $remainingMilliseconds);
}

`

@Sammyjo20
Copy link
Member

Hey @leonardyrj thank you for opening this issue - I'm very sorry for the slow reply on this one, been very busy lately!

Could this be throwing an exception because of the automatic 429 detection? Try disabling that and see if it works. You have to configure the limit to be slightly lower than the real API limit.

https://docs.saloon.dev/plugins/handling-rate-limits#429-too-many-attempts-detection

@ClaraLeigh
Copy link

ClaraLeigh commented Dec 5, 2023

Confirmed this is happening to me too. The stack trace shows that it is ignoring the sleep here. However if I dump and die the Limit at an earlier stage, it shows that sleep is set to true.

I don't know if I have time to work it out but I will need to figure this out before the xmas rush as its causing downtime for us when it gets busy.

image

Note: our threshold is set to 0.8, and the rate limit is 1000 per minute, so there should be about a 200 request buffer for us

@ClaraLeigh
Copy link

ClaraLeigh commented Dec 6, 2023

Ahhh so it is the fallback "too many attempts" limiter kicking in. I have no idea how its leaking an additional 200 requests and going over the defined limit, but for now this is my work around for making the sleep work on the custom limiter. Hopefully there is no knock-on issues from the change but for now this gets me out of the hole while I work out where the extra requests are going.

On the connector:

      /**
       * Get the limits for the rate limiter store
       *
       * @return array<Limit>
       * @throws LimitException
       */
      public function getLimits(): array
      {
          $tooManyAttemptsHandler = $this->detectTooManyAttempts === true ? $this->handleTooManyAttempts(...) : null;
  
          $limits = LimitHelper::configureLimits($this->resolveLimits(), $this->getLimiterPrefix(), $tooManyAttemptsHandler);
  
          foreach ($limits as $limit) {
              if (!$limit->getShouldSleep()) {
                  $limit->sleep();
              }
          }
  
          return $limits;
      }
    

@thisiskaden
Copy link

It happens me too.

@larskoole
Copy link

@ClaraLeigh bit late, but could the leak be because the requests are concurrent? 200 requests hitting an endpoint at exactly the same time would make them all read the same rate limit right?

But then again, we hit a 429 too and we're not doing anything concurrent on our end, just a command with a loop doing requests.

@maximehinnekens
Copy link

I tested it, it only works when hitting the manual limits, not when the 429 is detected. See #Sammyjo20/saloon-docs#72

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

6 participants