laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.38k stars 10.98k forks source link

the RateLimiter `Limit::perSecond` not working as expected for the queue jobs #53157

Closed amir9480 closed 1 week ago

amir9480 commented 1 week ago

Laravel Version

11.27.2

PHP Version

9.3.12

Database Driver & Version

No response

Description

I tried to use the new Laravel jobs per second rate limiter as described by @timacdonald in #48498. we are using a third-party web service with the following rate limits:

I used the following config for the rate limit:

RateLimiter::for('webservice_api', fn () => [
    Limit::perMinute(30),
    Limit::perSecond(1),
]);

Expected output: Limit job 1/second and 30/minute.

Actual output: The job is always rate-limited and does not execute at all.

Steps To Reproduce

  1. Create a fresh Laravel app and add the following rate limiting config to AppServiceProvider:

    public function boot(): void
    {
    RateLimiter::for('webservice_api', fn () => [
        Limit::perMinute(30),
        Limit::perSecond(1),
    ]);
    }
  2. Create a new job with the following code:

    
    <?php

namespace App\Jobs;

use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Queue\Queueable; use Illuminate\Queue\Middleware\RateLimited; use Illuminate\Support\Facades\Http; use Illuminate\Support\Facades\Log;

class SendToWebService implements ShouldQueue { use Queueable;

public function handle(): void
{
    Http::post('https://httpbin.org/post');
    Log::debug('Done');
}

public function middleware(): array
{
    return [new RateLimited('webservice_api')];
}

}


3. Dispatch the job multiple times:
```php
for ($i = 0; $i < 10; $i++) {
    dispatch(new \App\Jobs\SendToWebService());
}
  1. And, run the php artisan queue:work

Expected behavior: Execute one job per second.

Actual behavior: Not executing at all and always rate limited.

timacdonald commented 1 week ago

Seems to be a conflict when combining the rate limiters. In isolation, I can see each working as expected. When combined into a single limiter, they are conflicting somewhere.

Extracting two individual limiters and applying them together is working as expected.

RateLimiter::for('webservice_api_1', fn () => [
    Limit::perSecond(1),
]);

RateLimiter::for('webservice_api_2', fn () => [
    Limit::perMinute(30),
]);

Looking into this one.

timacdonald commented 1 week ago

Got to the bottom of this one. The issue is that both rate limits are using an identical key. In this case the key is an empty string. You need to call the by method and specify a unique value for each limit.

Screenshot 2024-10-15 at 10 06 40

Could you give this another try with the following and let me know if you have any issues.

RateLimiter::for('webservice_api', fn () => [
    Limit::perMinute(30)->by('minute'),
    Limit::perSecond(1)->by('second'),
]);

I'm going to see if we can handle this internally when there is no by set or at least make it more clear in the docs that you will have bad time without it.

amir9480 commented 1 week ago

@timacdonald Why not simply prefix array keys into actual generated keys? Also, I used the following page as a reference for the rate limiting and nothing was mentioned to define the by keys explicitly

https://laravel.com/docs/rate-limiting

timacdonald commented 6 days ago

@amir9480, prefixing array keys will mean if you change the order of the limits it will break existing rate limit hits - which is very unexpected and a subtle bug that could break systems.

Pretty much every fix for this breaks something, but I've found one fix that I think fixes this and introduces another caveat that is more acceptable.

timacdonald commented 6 days ago

p.s., that page does not document how to create named rate limiters or using a list of limits. That documentation is on the routing page.