From e7de9e10304e718a54d4dfbd04bdf224fec26c82 Mon Sep 17 00:00:00 2001 From: teandr Date: Sun, 17 Aug 2025 15:08:43 +0700 Subject: [PATCH 1/5] feat: create RetryMiddleware --- src/Downloader/Middleware/RetryMiddleware.php | 84 +++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 src/Downloader/Middleware/RetryMiddleware.php diff --git a/src/Downloader/Middleware/RetryMiddleware.php b/src/Downloader/Middleware/RetryMiddleware.php new file mode 100644 index 0000000..d94100f --- /dev/null +++ b/src/Downloader/Middleware/RetryMiddleware.php @@ -0,0 +1,84 @@ +getRequest(); + + /** @var int $retryCount */ + $retryCount = $request->getMeta('retry_count', 0); + + /** @var list $retryOnStatus */ + $retryOnStatus = $this->option('retryOnStatus'); + + /** @var int $maxRetries */ + $maxRetries = $this->option('maxRetries'); + + if (\in_array($response->getStatus(), $retryOnStatus, true) && $retryCount < $maxRetries) { + /** @var int $initialDelay */ + $initialDelay = $this->option('initialDelay'); + + /** @var float $delayMultiplier */ + $delayMultiplier = $this->option('delayMultiplier'); + + $delay = (int) ($initialDelay * ($delayMultiplier ** $retryCount)); + + $this->logger->info( + 'Retrying request', + [ + 'uri' => $request->getUri(), + 'status' => $response->getStatus(), + 'retry_count' => $retryCount + 1, + 'delay_ms' => $delay, + ], + ); + + $retryRequest = $request + ->withMeta('retry_count', $retryCount + 1) + ->addOption('delay', $delay); + + $this->scheduler->schedule($retryRequest); + + return $response->drop('Request being retried'); + } + + return $response; + } + + private static function defaultOptions(): array + { + return [ + 'retryOnStatus' => [500, 502, 503, 504], + 'maxRetries' => 3, + 'initialDelay' => 1000, + 'delayMultiplier' => 2.0, + ]; + } +} From 3bed2b6a1d4cc629018c2f07dea5c8f0704cbb6b Mon Sep 17 00:00:00 2001 From: teandr Date: Sun, 17 Aug 2025 15:12:24 +0700 Subject: [PATCH 2/5] fix: make RequestSchedulerInterface a singleton RequestSchedulerInterface was not registered as singleton. Engine and RetryMiddleware received different scheduler instances. This resulted in separate queues instead of a shared one. --- src/Core/DefaultContainer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Core/DefaultContainer.php b/src/Core/DefaultContainer.php index ef4bb7a..00e88a2 100644 --- a/src/Core/DefaultContainer.php +++ b/src/Core/DefaultContainer.php @@ -72,7 +72,7 @@ private function registerDefaultBindings(): void EventDispatcher::class, ); $this->container->add(ClockInterface::class, SystemClock::class); - $this->container->add( + $this->container->addShared( RequestSchedulerInterface::class, /** @phpstan-ignore return.type */ fn (): RequestSchedulerInterface => $this->container->get(ArrayRequestScheduler::class), From 626f9af69f4ecdff4a6f97b94640d8b056c6dce1 Mon Sep 17 00:00:00 2001 From: teandr Date: Sun, 17 Aug 2025 15:16:28 +0700 Subject: [PATCH 3/5] test: create RetryMiddlewareTest --- .../Middleware/RetryMiddlewareTest.php | 117 ++++++++++++++++++ 1 file changed, 117 insertions(+) create mode 100644 tests/Downloader/Middleware/RetryMiddlewareTest.php diff --git a/tests/Downloader/Middleware/RetryMiddlewareTest.php b/tests/Downloader/Middleware/RetryMiddlewareTest.php new file mode 100644 index 0000000..c17abeb --- /dev/null +++ b/tests/Downloader/Middleware/RetryMiddlewareTest.php @@ -0,0 +1,117 @@ +scheduler = new ArrayRequestScheduler($this->createMock(ClockInterface::class)); + $this->logger = new FakeLogger(); + $this->middleware = new RetryMiddleware($this->scheduler, $this->logger); + } + + public function testDoesNotRetrySuccessfulResponse(): void + { + $response = $this->makeResponse(status: 200); + $this->middleware->configure([]); + + $result = $this->middleware->handleResponse($response); + + self::assertSame($response, $result); + self::assertFalse($result->wasDropped()); + self::assertCount(0, $this->scheduler->forceNextRequests(10)); + } + + public function testDoesNotRetryNonRetryableErrorResponse(): void + { + $response = $this->makeResponse(status: 404); + $this->middleware->configure(['retryOnStatus' => [500]]); + + $result = $this->middleware->handleResponse($response); + + self::assertSame($response, $result); + self::assertFalse($result->wasDropped()); + self::assertCount(0, $this->scheduler->forceNextRequests(10)); + } + + public function testRetriesARetryableResponse(): void + { + $request = $this->makeRequest('https://example.com'); + $response = $this->makeResponse(request: $request, status: 503); + $this->middleware->configure([ + 'retryOnStatus' => [503], + 'maxRetries' => 2, + 'initialDelay' => 500, + ]); + + $result = $this->middleware->handleResponse($response); + + self::assertTrue($result->wasDropped()); + + $retriedRequests = $this->scheduler->forceNextRequests(10); + self::assertCount(1, $retriedRequests); + + $retriedRequest = $retriedRequests[0]; + self::assertSame(1, $retriedRequest->getMeta('retry_count')); + self::assertSame('https://example.com', $retriedRequest->getUri()); + self::assertSame(500, $retriedRequest->getOptions()['delay']); + } + + public function testStopsRetryingAfterMaxRetries(): void + { + $request = $this->makeRequest()->withMeta('retry_count', 3); + $response = $this->makeResponse(request: $request, status: 500); + $this->middleware->configure(['maxRetries' => 3]); + + $result = $this->middleware->handleResponse($response); + + self::assertSame($response, $result); + self::assertFalse($result->wasDropped()); + self::assertCount(0, $this->scheduler->forceNextRequests(10)); + } + + public function testCalculatesExponentialBackoffCorrectly(): void + { + $request = $this->makeRequest()->withMeta('retry_count', 2); + $response = $this->makeResponse(request: $request, status: 500); + $this->middleware->configure([ + 'initialDelay' => 1000, // 1s + 'delayMultiplier' => 2.0, + ]); + + $this->middleware->handleResponse($response); + + // initialDelay * (delayMultiplier ^ retry_count) + // 1000 * (2.0 ^ 2) = 1000 * 4 = 4000ms + $retriedRequest = $this->scheduler->forceNextRequests(10)[0]; + self::assertSame(4000, $retriedRequest->getOptions()['delay']); + } +} From 69dd20004a34fe15baad3356bb275cdb45ea6256 Mon Sep 17 00:00:00 2001 From: teandr Date: Wed, 20 Aug 2025 17:49:32 +0700 Subject: [PATCH 4/5] refactor: refactor RetryMiddleware to use backoff option --- src/Downloader/Middleware/RetryMiddleware.php | 81 ++++++++++++------- 1 file changed, 54 insertions(+), 27 deletions(-) diff --git a/src/Downloader/Middleware/RetryMiddleware.php b/src/Downloader/Middleware/RetryMiddleware.php index d94100f..db96a4f 100644 --- a/src/Downloader/Middleware/RetryMiddleware.php +++ b/src/Downloader/Middleware/RetryMiddleware.php @@ -13,6 +13,7 @@ namespace RoachPHP\Downloader\Middleware; +use InvalidArgumentException; use Psr\Log\LoggerInterface; use RoachPHP\Http\Response; use RoachPHP\Scheduling\RequestSchedulerInterface; @@ -31,45 +32,72 @@ public function __construct( public function handleResponse(Response $response): Response { $request = $response->getRequest(); - + /** @var int $retryCount */ $retryCount = $request->getMeta('retry_count', 0); /** @var list $retryOnStatus */ $retryOnStatus = $this->option('retryOnStatus'); - + /** @var int $maxRetries */ $maxRetries = $this->option('maxRetries'); - if (\in_array($response->getStatus(), $retryOnStatus, true) && $retryCount < $maxRetries) { - /** @var int $initialDelay */ - $initialDelay = $this->option('initialDelay'); - - /** @var float $delayMultiplier */ - $delayMultiplier = $this->option('delayMultiplier'); - - $delay = (int) ($initialDelay * ($delayMultiplier ** $retryCount)); - - $this->logger->info( - 'Retrying request', - [ - 'uri' => $request->getUri(), - 'status' => $response->getStatus(), - 'retry_count' => $retryCount + 1, - 'delay_ms' => $delay, - ], + if (!\in_array($response->getStatus(), $retryOnStatus, true) || $retryCount >= $maxRetries) { + return $response; + } + + $delay = $this->getDelay($retryCount); + + $this->logger->info( + 'Retrying request', + [ + 'uri' => $request->getUri(), + 'status' => $response->getStatus(), + 'retry_count' => $retryCount + 1, + 'delay_ms' => $delay, + ], + ); + + $retryRequest = $request + ->withMeta('retry_count', $retryCount + 1) + ->addOption('delay', $delay); + + $this->scheduler->schedule($retryRequest); + + return $response->drop('Request being retried'); + } + + private function getDelay(int $retryCount): int + { + /** @var int|list $backoff */ + $backoff = $this->option('backoff'); + + if (\is_int($backoff)) { + return $backoff * 1000; + } + + if (!\is_array($backoff)) { + throw new InvalidArgumentException( + 'backoff must be an integer or array, ' . \gettype($backoff) . ' given.', ); + } - $retryRequest = $request - ->withMeta('retry_count', $retryCount + 1) - ->addOption('delay', $delay); + if ([] === $backoff) { + throw new InvalidArgumentException('backoff array cannot be empty.'); + } - $this->scheduler->schedule($retryRequest); + $nonIntegerValues = \array_filter($backoff, static fn ($value) => !\is_int($value)); - return $response->drop('Request being retried'); + if ([] !== $nonIntegerValues) { + throw new InvalidArgumentException( + 'backoff array must contain only integers. Found: ' . + \implode(', ', \array_map('gettype', $nonIntegerValues)), + ); } - return $response; + $delay = $backoff[$retryCount] ?? $backoff[\array_key_last($backoff)]; + + return $delay * 1000; } private static function defaultOptions(): array @@ -77,8 +105,7 @@ private static function defaultOptions(): array return [ 'retryOnStatus' => [500, 502, 503, 504], 'maxRetries' => 3, - 'initialDelay' => 1000, - 'delayMultiplier' => 2.0, + 'backoff' => [1, 5, 10], ]; } } From 5f19f528eab0cb9453d5736f1fe5297c109a7c09 Mon Sep 17 00:00:00 2001 From: teandr Date: Wed, 20 Aug 2025 17:53:10 +0700 Subject: [PATCH 5/5] refactor: refactor RetryMiddlewareTest to use backoff option --- .../Middleware/RetryMiddlewareTest.php | 66 +++++++++++++++---- 1 file changed, 54 insertions(+), 12 deletions(-) diff --git a/tests/Downloader/Middleware/RetryMiddlewareTest.php b/tests/Downloader/Middleware/RetryMiddlewareTest.php index c17abeb..c2b1b68 100644 --- a/tests/Downloader/Middleware/RetryMiddlewareTest.php +++ b/tests/Downloader/Middleware/RetryMiddlewareTest.php @@ -13,11 +13,12 @@ namespace RoachPHP\Tests\Downloader\Middleware; +use InvalidArgumentException; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; use RoachPHP\Downloader\Middleware\RetryMiddleware; use RoachPHP\Scheduling\ArrayRequestScheduler; use RoachPHP\Scheduling\Timing\ClockInterface; -use RoachPHP\Scheduling\Timing\FakeClock; use RoachPHP\Testing\Concerns\InteractsWithRequestsAndResponses; use RoachPHP\Testing\FakeLogger; @@ -69,7 +70,7 @@ public function testRetriesARetryableResponse(): void $this->middleware->configure([ 'retryOnStatus' => [503], 'maxRetries' => 2, - 'initialDelay' => 500, + 'backoff' => [1, 2, 3], ]); $result = $this->middleware->handleResponse($response); @@ -82,14 +83,14 @@ public function testRetriesARetryableResponse(): void $retriedRequest = $retriedRequests[0]; self::assertSame(1, $retriedRequest->getMeta('retry_count')); self::assertSame('https://example.com', $retriedRequest->getUri()); - self::assertSame(500, $retriedRequest->getOptions()['delay']); + self::assertSame(1000, $retriedRequest->getOptions()['delay']); } public function testStopsRetryingAfterMaxRetries(): void { $request = $this->makeRequest()->withMeta('retry_count', 3); $response = $this->makeResponse(request: $request, status: 500); - $this->middleware->configure(['maxRetries' => 3]); + $this->middleware->configure(['maxRetries' => 3, 'backoff' => [1, 2, 3]]); $result = $this->middleware->handleResponse($response); @@ -98,20 +99,61 @@ public function testStopsRetryingAfterMaxRetries(): void self::assertCount(0, $this->scheduler->forceNextRequests(10)); } - public function testCalculatesExponentialBackoffCorrectly(): void + public function testUsesBackoffArrayForDelay(): void { $request = $this->makeRequest()->withMeta('retry_count', 2); $response = $this->makeResponse(request: $request, status: 500); - $this->middleware->configure([ - 'initialDelay' => 1000, // 1s - 'delayMultiplier' => 2.0, - ]); + $this->middleware->configure(['backoff' => [1, 5, 10]]); + + $this->middleware->handleResponse($response); + + $retriedRequest = $this->scheduler->forceNextRequests(10)[0]; + self::assertSame(10000, $retriedRequest->getOptions()['delay']); + } + + public function testUsesLastBackoffValueIfRetriesExceedBackoffCount(): void + { + $request = $this->makeRequest()->withMeta('retry_count', 5); + $response = $this->makeResponse(request: $request, status: 500); + $this->middleware->configure(['backoff' => [1, 5, 10], 'maxRetries' => 6]); + + $this->middleware->handleResponse($response); + + $retriedRequest = $this->scheduler->forceNextRequests(10)[0]; + self::assertSame(10000, $retriedRequest->getOptions()['delay']); + } + + public function testUsesIntegerBackoffForDelay(): void + { + $request = $this->makeRequest()->withMeta('retry_count', 2); + $response = $this->makeResponse(request: $request, status: 500); + $this->middleware->configure(['backoff' => 5]); $this->middleware->handleResponse($response); - // initialDelay * (delayMultiplier ^ retry_count) - // 1000 * (2.0 ^ 2) = 1000 * 4 = 4000ms $retriedRequest = $this->scheduler->forceNextRequests(10)[0]; - self::assertSame(4000, $retriedRequest->getOptions()['delay']); + self::assertSame(5000, $retriedRequest->getOptions()['delay']); + } + + public static function invalidBackoffProvider(): array + { + return [ + 'empty array' => [[], 'backoff array cannot be empty.'], + 'array with non-int' => [[1, 'a', 3], 'backoff array must contain only integers. Found: string'], + 'string' => ['not-an-array', 'backoff must be an integer or array, string given.'], + 'float' => [1.23, 'backoff must be an integer or array, double given.'], + ]; + } + + #[DataProvider('invalidBackoffProvider')] + public function testThrowsExceptionOnInvalidBackoff(mixed $backoff, string $expectedMessage): void + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage($expectedMessage); + + $response = $this->makeResponse(status: 500); + $this->middleware->configure(['backoff' => $backoff]); + + $this->middleware->handleResponse($response); } }