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

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

Closed
amir9480 opened this issue Oct 14, 2024 · 5 comments · Fixed by #53177
Closed

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

amir9480 opened this issue Oct 14, 2024 · 5 comments · Fixed by #53177

Comments

@amir9480
Copy link
Contributor

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:

  • 1 request/second
  • 30 requests/minute

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),
    ]);
}
  1. 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')];
    }
}
  1. Dispatch the job multiple times:
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
Copy link
Member

timacdonald commented Oct 14, 2024

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
Copy link
Member

timacdonald commented Oct 14, 2024

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
Copy link
Contributor Author

@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
Copy link
Member

@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
Copy link
Member

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.

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

Successfully merging a pull request may close this issue.

2 participants