saloonphp / saloon

🤠 Build beautiful API integrations and SDKs with Saloon
https://docs.saloon.dev
MIT License
2.09k stars 107 forks source link

Rate Limit Plugin: Issue with 'allow(1)->everySeconds(1)->sleep()' Exceeding One Request per Second #392

Open VidalCamargos opened 8 months ago

VidalCamargos commented 8 months ago

I'm encountering an issue with the rate limit plugin in my project. Specifically, when using the resolveLimits function with the sequence Limit::allow(1)->everySeconds(1)->sleep(), the plugin doesn't consistently adhere to the specified rate limit of one request per second. Occasionally, it exceeds this limit and makes multiple requests within a single second.

Upon investigating the plugin's code, I noticed a potential issue within the handleExceededLimit function. There seems to be a scenario where the $limit->getRemainingSeconds value reaches zero, possibly leading to the delay not being properly applied. While I can't confirm if this is the root cause, I made a modification to this method within my connector, and it appears to have mitigated the issue.

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

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

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

In production, with the limit of one request per second and without changing anything in the handleExceededLimit method:

image

In localhost, setting the delay in handleExceededLimit as 0:

image

In localhost, setting the delay in handleExceededLimit to 1 if it $limit->getRemainingSeconds reaches 0:

image

My current connector:

<?php

namespace Connector;

use App\Exceptions\UnauthorizedSetup;
use App\Http\Middleware\Saloon\ResponseLogger;
use App\Models\Store;
use App\Traits\InteractsWithSaloonResponses;
use GuzzleHttp\RequestOptions;
use GuzzleHttp\TransferStats;
use Illuminate\Support\Facades\Cache;
use MyIntegration\Http\Pagination\Paged;
use Saloon\Http\Connector;
use Saloon\Http\PendingRequest;
use Saloon\Http\Request;
use Saloon\Http\Response;
use Saloon\PaginationPlugin\Contracts\HasPagination;
use Saloon\RateLimitPlugin\Contracts\RateLimitStore;
use Saloon\RateLimitPlugin\Limit;
use Saloon\RateLimitPlugin\Stores\LaravelCacheStore;
use Saloon\RateLimitPlugin\Stores\MemoryStore;
use Saloon\RateLimitPlugin\Traits\HasRateLimits;
use Saloon\Traits\Plugins\AcceptsJson;
use Saloon\Traits\Plugins\AlwaysThrowOnErrors;
use Saloon\Traits\Plugins\HasTimeout;
use Symfony\Component\HttpFoundation\Response as ResponseCode;

class IntegrationConnector extends Connector implements HasPagination
{
    use AcceptsJson;
    use AlwaysThrowOnErrors;
    use HasRateLimits {
        hasRateLimits::handleTooManyAttempts as protected traitHandleTooManyAttempts;
    }
    use HasTimeout;
    use InteractsWithSaloonResponses;

    public ?string $requestTime = null;
    protected int $requestTimeout = 10;
    public ?int $tries = 3;
    public ?bool $useExponentialBackoff = true;

    private const LIMITER = 'integration-connector';

    public function __construct(private readonly Store $store)
    {
        $oAuth = $this->store->oAuth()->active()->first();

        if (is_null($oAuth)) {
            throw new UnauthorizedSetup('Could not find active oAuth to use.', fatal: true);
        }

        $this->middleware()->onRequest(
            static fn (PendingRequest $pendingRequest) => $pendingRequest->headers()
                ->add('Authorization', 'Bearer '.$oAuth->refresh()->access_token),
        );

        $this->middleware()->onResponse(new ResponseLogger($store));

        $this->retryInterval = app()->environment('production') ? 2000 : null;
    }

    public function resolveBaseUrl(): string
    {
        return 'integrationUrl';
    }

    protected function defaultConfig(): array
    {
        $time = &$this->requestTime;

        return [
            RequestOptions::VERIFY => false,
            RequestOptions::ON_STATS => function (TransferStats $stats) use (&$time) {
                $time = (string) $stats->getTransferTime();
            },
        ];
    }

    public function paginate(Request $request): Paged
    {
        return new Paged($this, $request);
    }

    protected function resolveLimits(): array
    {
        return [
            Limit::allow(1)->everySeconds(1)->name('second')->sleep(),
            Limit::allow(60)->everyMinute()->name('minute')->sleep(),
            Limit::allow(290)->everyFiveMinutes()->name('five-minutes')->sleep(),
            Limit::allow(118_000)->everyDay()->name('daily'),
        ];
    }

    protected function resolveRateLimitStore(): RateLimitStore
    {
        if (! app()->environment('production')) {
            return new MemoryStore();
        }

        return new LaravelCacheStore(Cache::store('redis'));
    }

    protected function handleTooManyAttempts(Response $response, Limit $limit): void
    {
        if ($response->status() === ResponseCode::HTTP_TOO_MANY_REQUESTS) {
            $this->responseLogger($this->store, $response);
        }

        $this->traitHandleTooManyAttempts($response, $limit);
    }

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

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

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

    protected function getLimiterPrefix(): ?string
    {
        return self::LIMITER;
    }
}

I'm seeking guidance on identifying the underlying cause of this inconsistency and exploring potential solutions. Could you please provide insights into what might be causing this behavior and suggest possible remedies?