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

[8.x] Make sure availableIn returns positive values #37809

Merged
merged 2 commits into from
Jun 24, 2021

Conversation

rickycheers
Copy link
Contributor

Problem

When running a job on Laravel Vapor if a rate limiter middleware is added to that job, like below

public function middleware()
{
    RateLimiter::for('name', function ($job) {
        return Limit::perMinute(400);
    });

    return [new RateLimited('name')];
}

The following error message from SQS pops up in the logs intermittently.

<?xml version="1.0"?><ErrorResponse xmlns="http://queue.amazonaws.com/doc/2012-11-05/"><Error><Type>Sender</Type><Code>I (truncated...)
 InvalidParameterValue (client): Value -1624201534 for parameter DelaySeconds is invalid. Reason: DelaySeconds must be >= 0 and <= 900. - <?xml version="1.0"?><ErrorResponse xmlns="http://queue.amazonaws.com/doc/2012-11-05/"><Error><Type>Sender</Type><Code>InvalidParameterValue</Code><Message>Value -1624201534 for parameter DelaySeconds is invalid. Reason: DelaySeconds must be &gt;= 0 and &lt;= 900.</Message><Detail/></Error><RequestId>...</RequestId></ErrorResponse>

Aws_Sqs_Exception_SqsException_-All_Events-Discover-bold-brush-cronos-_Sentry-2

I tracked down the problem to Illuminte\Cache\RateLimiter::availableIn where it can some times return negative values apparently when a "timer" value is not yet set in the cache. (I guess this only happens under heavy loads)

I was able to spot one in the wild when that happens. Here's an example:

Screenshot_6_24_21__4_15_PM


Solution

Make sure availableIn always return a positive value. This way Illuminate\Queue\Middleware\RateLimited::getTimeUntilNextRetry can return an appropriate value.

Perhaps Illuminate\Queue\Middleware\RateLimitedWithRedis::getTimeUntilNextRetry requires a similar prevention? Let me know if you think it does and I can work on it as well.

@rickycheers rickycheers changed the title Make sure availableIn returns positive values [8.x] Make sure availableIn returns positive values Jun 24, 2021
@taylorotwell taylorotwell merged commit 7a8b3a1 into laravel:8.x Jun 24, 2021
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 this pull request may close these issues.

2 participants