From 531f5e53c418c8d91a207d4946a13b6c6b3802ac Mon Sep 17 00:00:00 2001 From: Moshe Brodsky <44633930+moshe-autoleadstar@users.noreply.github.com> Date: Mon, 12 May 2025 17:44:45 +0300 Subject: [PATCH 1/7] Add deleteWhen for throttle exceptions job middleware --- .../Queue/Middleware/ThrottlesExceptions.php | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/src/Illuminate/Queue/Middleware/ThrottlesExceptions.php b/src/Illuminate/Queue/Middleware/ThrottlesExceptions.php index 68017795655c..66056e2a5a21 100644 --- a/src/Illuminate/Queue/Middleware/ThrottlesExceptions.php +++ b/src/Illuminate/Queue/Middleware/ThrottlesExceptions.php @@ -57,6 +57,13 @@ class ThrottlesExceptions */ protected $whenCallback; + /** + * The callbacks that determine if the job should be deleted. + * + * @var callable[] + */ + protected array $deleteWhenCallbacks = []; + /** * The prefix of the rate limiter key. * @@ -111,6 +118,10 @@ public function handle($job, $next) report($throwable); } + if ($this->shouldDelete($throwable)) { + return $job->delete(); + } + $this->limiter->hit($jobKey, $this->decaySeconds); return $job->release($this->retryAfterMinutes * 60); @@ -130,6 +141,40 @@ public function when(callable $callback) return $this; } + /** + * Add a callback that should determine if the job should be deleted. + * + * @param callable|string $callback + * @return $this + */ + public function deleteWhen(callable|string $callback) + { + if (is_string($callback)) { + $this->deleteWhenCallbacks[] = fn (Throwable $e) => $e instanceof $callback; + } else { + $this->deleteWhenCallbacks[] = $callback; + } + + return $this; + } + + /** + * Run the delete callbacks to see if the job should be deleted for the given exception. + * + * @param Throwable $throwable + * @return bool + */ + protected function shouldDelete(Throwable $throwable): bool + { + foreach ($this->deleteWhenCallbacks as $callback) { + if (call_user_func($callback, $throwable)) { + return true; + } + } + + return false; + } + /** * Set the prefix of the rate limiter key. * From f618dd1d77ce2dbed422097751edb36b9d546796 Mon Sep 17 00:00:00 2001 From: Moshe Brodsky Date: Mon, 12 May 2025 17:48:56 +0300 Subject: [PATCH 2/7] adding for redis and adding a test --- .../ThrottlesExceptionsWithRedis.php | 4 +++ .../Queue/ThrottlesExceptionsTest.php | 25 +++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/src/Illuminate/Queue/Middleware/ThrottlesExceptionsWithRedis.php b/src/Illuminate/Queue/Middleware/ThrottlesExceptionsWithRedis.php index e5b79a7d67ed..8c6b78912b0d 100644 --- a/src/Illuminate/Queue/Middleware/ThrottlesExceptionsWithRedis.php +++ b/src/Illuminate/Queue/Middleware/ThrottlesExceptionsWithRedis.php @@ -58,6 +58,10 @@ public function handle($job, $next) report($throwable); } + if ($this->shouldDelete($throwable)) { + return $job->delete(); + } + $this->limiter->acquire(); return $job->release($this->retryAfterMinutes * 60); diff --git a/tests/Integration/Queue/ThrottlesExceptionsTest.php b/tests/Integration/Queue/ThrottlesExceptionsTest.php index e559c41b7f8f..75a9fcd16f4c 100644 --- a/tests/Integration/Queue/ThrottlesExceptionsTest.php +++ b/tests/Integration/Queue/ThrottlesExceptionsTest.php @@ -40,6 +40,11 @@ public function testCircuitResetsAfterSuccess() $this->assertJobWasReleasedWithDelay(CircuitBreakerTestJob::class); } + public function testJobIsDeleted() + { + $this->assertJobWasDeleted(CircuitBreakerTestJob::class); + } + protected function assertJobWasReleasedImmediately($class) { $class::$handled = false; @@ -82,6 +87,26 @@ protected function assertJobWasReleasedWithDelay($class) $this->assertFalse($class::$handled); } + protected function assertJobWasDeleted($class) + { + $class::$handled = false; + $instance = new CallQueuedHandler(new Dispatcher($this->app), $this->app); + + $job = m::mock(Job::class); + + $job->shouldReceive('hasFailed')->once()->andReturn(false); + $job->shouldReceive('delete')->once(); + $job->shouldReceive('isDeleted')->andReturn(true); + $job->shouldReceive('isDeletedOrReleased')->once()->andReturn(true); + $job->shouldReceive('uuid')->andReturn('simple-test-uuid'); + + $instance->call($job, [ + 'command' => serialize($command = new $class), + ]); + + $this->assertTrue($class::$handled); + } + protected function assertJobRanSuccessfully($class) { $class::$handled = false; From cd0cfadd474dd8d51ae9f14e3a360ba6981880d4 Mon Sep 17 00:00:00 2001 From: Moshe Brodsky Date: Mon, 12 May 2025 17:54:35 +0300 Subject: [PATCH 3/7] attempt to fix test --- .../Queue/ThrottlesExceptionsTest.php | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/tests/Integration/Queue/ThrottlesExceptionsTest.php b/tests/Integration/Queue/ThrottlesExceptionsTest.php index 75a9fcd16f4c..be7228d46d32 100644 --- a/tests/Integration/Queue/ThrottlesExceptionsTest.php +++ b/tests/Integration/Queue/ThrottlesExceptionsTest.php @@ -42,7 +42,7 @@ public function testCircuitResetsAfterSuccess() public function testJobIsDeleted() { - $this->assertJobWasDeleted(CircuitBreakerTestJob::class); + $this->assertJobWasDeleted(CircuitBreakerTestDeleteWhenJob::class); } protected function assertJobWasReleasedImmediately($class) @@ -339,6 +339,25 @@ public function middleware() } } +class CircuitBreakerTestDeleteWhenJob +{ + use InteractsWithQueue, Queueable; + + public static $handled = false; + + public function handle() + { + static::$handled = true; + + throw new Exception; + } + + public function middleware() + { + return [(new ThrottlesExceptions(2, 10 * 60))->deleteWhen(Exception::class)]; + } +} + class CircuitBreakerSuccessfulJob { use InteractsWithQueue, Queueable; From 20bf76fa7ea2cddb1db97b68252848947144508e Mon Sep 17 00:00:00 2001 From: Moshe Brodsky Date: Mon, 12 May 2025 17:57:52 +0300 Subject: [PATCH 4/7] add expectation for isReleased check --- tests/Integration/Queue/ThrottlesExceptionsTest.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/Integration/Queue/ThrottlesExceptionsTest.php b/tests/Integration/Queue/ThrottlesExceptionsTest.php index be7228d46d32..275febbea83f 100644 --- a/tests/Integration/Queue/ThrottlesExceptionsTest.php +++ b/tests/Integration/Queue/ThrottlesExceptionsTest.php @@ -97,6 +97,7 @@ protected function assertJobWasDeleted($class) $job->shouldReceive('hasFailed')->once()->andReturn(false); $job->shouldReceive('delete')->once(); $job->shouldReceive('isDeleted')->andReturn(true); + $job->shouldReceive('isReleased')->once()->andReturn(false); $job->shouldReceive('isDeletedOrReleased')->once()->andReturn(true); $job->shouldReceive('uuid')->andReturn('simple-test-uuid'); From c59b8681007054cf57b19b347ff2cde64b359158 Mon Sep 17 00:00:00 2001 From: Moshe Brodsky Date: Mon, 12 May 2025 17:59:37 +0300 Subject: [PATCH 5/7] ok so its called twice --- tests/Integration/Queue/ThrottlesExceptionsTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Integration/Queue/ThrottlesExceptionsTest.php b/tests/Integration/Queue/ThrottlesExceptionsTest.php index 275febbea83f..c3d9350e72c8 100644 --- a/tests/Integration/Queue/ThrottlesExceptionsTest.php +++ b/tests/Integration/Queue/ThrottlesExceptionsTest.php @@ -97,7 +97,7 @@ protected function assertJobWasDeleted($class) $job->shouldReceive('hasFailed')->once()->andReturn(false); $job->shouldReceive('delete')->once(); $job->shouldReceive('isDeleted')->andReturn(true); - $job->shouldReceive('isReleased')->once()->andReturn(false); + $job->shouldReceive('isReleased')->twice()->andReturn(false); $job->shouldReceive('isDeletedOrReleased')->once()->andReturn(true); $job->shouldReceive('uuid')->andReturn('simple-test-uuid'); From 5cc88932935fbab188dfe7d16c9857e74360fd1a Mon Sep 17 00:00:00 2001 From: Moshe Brodsky Date: Tue, 13 May 2025 14:19:35 +0300 Subject: [PATCH 6/7] change terminology from delete to skip --- .../Queue/Middleware/ThrottlesExceptions.php | 20 +++++++++---------- .../ThrottlesExceptionsWithRedis.php | 2 +- ...ptionsTest.php => ThrottlesExceptions.php} | 8 ++++---- 3 files changed, 15 insertions(+), 15 deletions(-) rename tests/Integration/Queue/{ThrottlesExceptionsTest.php => ThrottlesExceptions.php} (97%) diff --git a/src/Illuminate/Queue/Middleware/ThrottlesExceptions.php b/src/Illuminate/Queue/Middleware/ThrottlesExceptions.php index 66056e2a5a21..0124d28aecdb 100644 --- a/src/Illuminate/Queue/Middleware/ThrottlesExceptions.php +++ b/src/Illuminate/Queue/Middleware/ThrottlesExceptions.php @@ -58,11 +58,11 @@ class ThrottlesExceptions protected $whenCallback; /** - * The callbacks that determine if the job should be deleted. + * The callbacks that determine if the job should be skipped / deleted. * * @var callable[] */ - protected array $deleteWhenCallbacks = []; + protected array $skipWhenCallbacks = []; /** * The prefix of the rate limiter key. @@ -118,7 +118,7 @@ public function handle($job, $next) report($throwable); } - if ($this->shouldDelete($throwable)) { + if ($this->shouldSkip($throwable)) { return $job->delete(); } @@ -142,31 +142,31 @@ public function when(callable $callback) } /** - * Add a callback that should determine if the job should be deleted. + * Add a callback that should determine if the job should be skipped / deleted. * * @param callable|string $callback * @return $this */ - public function deleteWhen(callable|string $callback) + public function skipWhen(callable|string $callback) { if (is_string($callback)) { - $this->deleteWhenCallbacks[] = fn (Throwable $e) => $e instanceof $callback; + $this->skipWhenCallbacks[] = fn (Throwable $e) => $e instanceof $callback; } else { - $this->deleteWhenCallbacks[] = $callback; + $this->skipWhenCallbacks[] = $callback; } return $this; } /** - * Run the delete callbacks to see if the job should be deleted for the given exception. + * Run the skip / delete callbacks to see if the job should be deleted for the given exception. * * @param Throwable $throwable * @return bool */ - protected function shouldDelete(Throwable $throwable): bool + protected function shouldSkip(Throwable $throwable): bool { - foreach ($this->deleteWhenCallbacks as $callback) { + foreach ($this->skipWhenCallbacks as $callback) { if (call_user_func($callback, $throwable)) { return true; } diff --git a/src/Illuminate/Queue/Middleware/ThrottlesExceptionsWithRedis.php b/src/Illuminate/Queue/Middleware/ThrottlesExceptionsWithRedis.php index 8c6b78912b0d..430ce80e2fb8 100644 --- a/src/Illuminate/Queue/Middleware/ThrottlesExceptionsWithRedis.php +++ b/src/Illuminate/Queue/Middleware/ThrottlesExceptionsWithRedis.php @@ -58,7 +58,7 @@ public function handle($job, $next) report($throwable); } - if ($this->shouldDelete($throwable)) { + if ($this->shouldSkip($throwable)) { return $job->delete(); } diff --git a/tests/Integration/Queue/ThrottlesExceptionsTest.php b/tests/Integration/Queue/ThrottlesExceptions.php similarity index 97% rename from tests/Integration/Queue/ThrottlesExceptionsTest.php rename to tests/Integration/Queue/ThrottlesExceptions.php index c3d9350e72c8..2eedf2d0f8ed 100644 --- a/tests/Integration/Queue/ThrottlesExceptionsTest.php +++ b/tests/Integration/Queue/ThrottlesExceptions.php @@ -40,9 +40,9 @@ public function testCircuitResetsAfterSuccess() $this->assertJobWasReleasedWithDelay(CircuitBreakerTestJob::class); } - public function testJobIsDeleted() + public function testCircuitCanSkipJob() { - $this->assertJobWasDeleted(CircuitBreakerTestDeleteWhenJob::class); + $this->assertJobWasDeleted(CircuitBreakerSkipJob::class); } protected function assertJobWasReleasedImmediately($class) @@ -340,7 +340,7 @@ public function middleware() } } -class CircuitBreakerTestDeleteWhenJob +class CircuitBreakerSkipJob { use InteractsWithQueue, Queueable; @@ -355,7 +355,7 @@ public function handle() public function middleware() { - return [(new ThrottlesExceptions(2, 10 * 60))->deleteWhen(Exception::class)]; + return [(new ThrottlesExceptions(2, 10 * 60))->skipWhen(Exception::class)]; } } From f75d9071e657a711bbca520c7bc8a74c4b4a989f Mon Sep 17 00:00:00 2001 From: Taylor Otwell Date: Tue, 13 May 2025 08:42:29 -0500 Subject: [PATCH 7/7] formatting --- .../Queue/Middleware/ThrottlesExceptions.php | 24 +++++++++---------- .../ThrottlesExceptionsWithRedis.php | 2 +- ...ptions.php => ThrottlesExceptionsTest.php} | 2 +- 3 files changed, 13 insertions(+), 15 deletions(-) rename tests/Integration/Queue/{ThrottlesExceptions.php => ThrottlesExceptionsTest.php} (99%) diff --git a/src/Illuminate/Queue/Middleware/ThrottlesExceptions.php b/src/Illuminate/Queue/Middleware/ThrottlesExceptions.php index 0124d28aecdb..5c5c7d04a7ed 100644 --- a/src/Illuminate/Queue/Middleware/ThrottlesExceptions.php +++ b/src/Illuminate/Queue/Middleware/ThrottlesExceptions.php @@ -58,11 +58,11 @@ class ThrottlesExceptions protected $whenCallback; /** - * The callbacks that determine if the job should be skipped / deleted. + * The callbacks that determine if the job should be deleted. * * @var callable[] */ - protected array $skipWhenCallbacks = []; + protected array $deleteWhenCallbacks = []; /** * The prefix of the rate limiter key. @@ -118,7 +118,7 @@ public function handle($job, $next) report($throwable); } - if ($this->shouldSkip($throwable)) { + if ($this->shouldDelete($throwable)) { return $job->delete(); } @@ -142,31 +142,29 @@ public function when(callable $callback) } /** - * Add a callback that should determine if the job should be skipped / deleted. + * Add a callback that should determine if the job should be deleted. * * @param callable|string $callback * @return $this */ - public function skipWhen(callable|string $callback) + public function deleteWhen(callable|string $callback) { - if (is_string($callback)) { - $this->skipWhenCallbacks[] = fn (Throwable $e) => $e instanceof $callback; - } else { - $this->skipWhenCallbacks[] = $callback; - } + $this->deleteWhenCallbacks[] = is_string($callback) + ? fn (Throwable $e) => $e instanceof $callback + : $callback; return $this; } /** - * Run the skip / delete callbacks to see if the job should be deleted for the given exception. + * Run the skip / delete callbacks to determine if the job should be deleted for the given exception. * * @param Throwable $throwable * @return bool */ - protected function shouldSkip(Throwable $throwable): bool + protected function shouldDelete(Throwable $throwable): bool { - foreach ($this->skipWhenCallbacks as $callback) { + foreach ($this->deleteWhenCallbacks as $callback) { if (call_user_func($callback, $throwable)) { return true; } diff --git a/src/Illuminate/Queue/Middleware/ThrottlesExceptionsWithRedis.php b/src/Illuminate/Queue/Middleware/ThrottlesExceptionsWithRedis.php index 430ce80e2fb8..8c6b78912b0d 100644 --- a/src/Illuminate/Queue/Middleware/ThrottlesExceptionsWithRedis.php +++ b/src/Illuminate/Queue/Middleware/ThrottlesExceptionsWithRedis.php @@ -58,7 +58,7 @@ public function handle($job, $next) report($throwable); } - if ($this->shouldSkip($throwable)) { + if ($this->shouldDelete($throwable)) { return $job->delete(); } diff --git a/tests/Integration/Queue/ThrottlesExceptions.php b/tests/Integration/Queue/ThrottlesExceptionsTest.php similarity index 99% rename from tests/Integration/Queue/ThrottlesExceptions.php rename to tests/Integration/Queue/ThrottlesExceptionsTest.php index 2eedf2d0f8ed..19f525afcc8d 100644 --- a/tests/Integration/Queue/ThrottlesExceptions.php +++ b/tests/Integration/Queue/ThrottlesExceptionsTest.php @@ -355,7 +355,7 @@ public function handle() public function middleware() { - return [(new ThrottlesExceptions(2, 10 * 60))->skipWhen(Exception::class)]; + return [(new ThrottlesExceptions(2, 10 * 60))->deleteWhen(Exception::class)]; } }