Skip to content

Add Request/Response Hooks & Events System #70

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.

Sign up for GitHub

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

Closed
Copilot wants to merge 4 commits into from
Closed

Add Request/Response Hooks & Events System #70

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b84a464
Initial plan
Copilot Nov 28, 2025
3144534
Add Request/Response Hooks & Events System
Copilot Nov 28, 2025
8a1edf7
Fix code style issues
Copilot Nov 28, 2025
aa7fe26
Address code review feedback - extract retryable status codes to cons…
Copilot Nov 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 118 additions & 0 deletions src/Fetch/Concerns/ManagesRetries.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,14 @@

namespace Fetch\Concerns;

use Fetch\Events\ErrorEvent;
use Fetch\Events\RetryEvent;
use Fetch\Exceptions\RequestException as FetchRequestException;
use Fetch\Interfaces\ClientHandler;
use Fetch\Interfaces\Response as ResponseInterface;
use GuzzleHttp\Exception\ConnectException;
use InvalidArgumentException;
use Psr\Http\Message\RequestInterface;
use RuntimeException;
use Throwable;

Expand Down Expand Up @@ -261,4 +264,119 @@ protected function isRetryableError(Throwable $e): bool

return $isRetryableStatusCode || $isRetryableException;
}

/**
* Implement retry logic for the request with event dispatching.
*
* @param callable $request The request to execute
* @param RequestInterface $psrRequest The PSR-7 request for events
* @param string $correlationId The correlation ID for event tracking
* @return ResponseInterface The response after successful execution
*
* @throws FetchRequestException If the request fails after all retries
* @throws RuntimeException If something unexpected happens
*/
protected function retryRequestWithEvents(
callable $request,
RequestInterface $psrRequest,
string $correlationId
): ResponseInterface {
$attempts = $this->maxRetries ?? self::DEFAULT_RETRIES;
$delay = $this->retryDelay ?? self::DEFAULT_RETRY_DELAY;
$exceptions = [];

for ($attempt = 0; $attempt <= $attempts; $attempt++) {
Comment on lines +284 to +288
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The loop condition for ($attempt = 0; $attempt <= $attempts; $attempt++) creates a potential off-by-one error. When $attempts = 3, the loop runs for $attempt = 0, 1, 2, 3, which is 4 iterations. This means:

  • $attempts = 3 should mean "3 retries" (4 total attempts including the initial)
  • But the code treats it as "retry up to and including attempt 3" (4 attempts total)

The variable naming is ambiguous. Either:

  1. Rename $attempts to $maxAttempts or $maxRetries to clarify what it represents
  2. Change the loop to $attempt < $attempts if $attempts means total attempts
  3. Document clearly whether maxRetries means "number of retries after the initial attempt" or "total number of attempts"

This same issue exists in the original retryRequest method at line 156.

Copilot uses AI. Check for mistakes.
try {
// Execute the request
return $request();
} catch (Throwable $e) {
// Collect exception for later
$exceptions[] = $e;

// Get response from exception if available
$response = null;
if ($e instanceof FetchRequestException && $e->getResponse()) {
$response = $e->getResponse();
} elseif (method_exists($e, 'getResponse')) {
$response = $e->getResponse();
}

// If this was the last attempt, dispatch error event and break
if ($attempt === $attempts) {
// Dispatch error event
if (method_exists($this, 'dispatchEvent')) {
$this->dispatchEvent(new ErrorEvent(
$psrRequest,
$e,
$correlationId,
microtime(true),
$attempt + 1,
$response
));
}
break;
}

// Only retry on retryable errors
if (! $this->isRetryableError($e)) {
// Dispatch error event for non-retryable errors
if (method_exists($this, 'dispatchEvent')) {
$this->dispatchEvent(new ErrorEvent(
$psrRequest,
$e,
$correlationId,
microtime(true),
$attempt + 1,
$response
));
}
throw $e;
}

// Log the retry for debugging purposes
if (method_exists($this, 'logRetry')) {
$this->logRetry($attempt + 1, $attempts, $e);
}

// Calculate delay with exponential backoff and jitter
$currentDelay = $this->calculateBackoffDelay($delay, $attempt);

// Dispatch retry event
if (method_exists($this, 'dispatchEvent')) {
$this->dispatchEvent(new RetryEvent(
$psrRequest,
$e,
$attempt + 1,
$attempts,
$currentDelay,
$correlationId,
microtime(true)
));
}
Comment on lines +288 to +355
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the retryRequestWithEvents method, the attempt number passed to events uses $attempt + 1 to make it 1-based (lines 313, 329), but this is inconsistent because the loop variable $attempt already represents the actual attempt index (0 for first attempt). This means:

  • First attempt: $attempt = 0, events report attempt 1 ✓
  • Second attempt: $attempt = 1, events report attempt 2 ✓
  • Last attempt: $attempt = $attempts, events report attempt $attempts + 1 ✗

When $attempts = 3 (meaning max 3 retries, 4 total attempts), the final error event would report attempt 4 instead of 3. The loop condition $attempt <= $attempts combined with $attempt + 1 causes an off-by-one error in the reported attempt numbers.

Copilot uses AI. Check for mistakes.

// Sleep before the next retry
usleep($currentDelay * 1000); // Convert milliseconds to microseconds
}
}

// If we got here, all retries failed
$lastException = end($exceptions) ?: new RuntimeException('Request failed after all retries');

// Enhanced failure reporting
if ($lastException instanceof FetchRequestException && $lastException->getResponse()) {
$statusCode = $lastException->getResponse()->getStatusCode();
throw new RuntimeException(
sprintf(
'Request failed after %d attempts with status code %d: %s',
$attempts + 1,
$statusCode,
$lastException->getMessage()
),
$statusCode,
$lastException
);
}
Comment on lines +366 to +378
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message on line 368 uses $attempts + 1 in the message "Request failed after %d attempts", but $attempts represents the number of retries, not total attempts. This should be either:

  • "Request failed after %d retries" using $attempts
  • "Request failed after %d attempts" using $attempts + 1

However, given that the loop already runs from 0 to $attempts (inclusive), this gives $attempts + 1 total executions. The message construction is misleading about whether it's counting retries or total attempts.

Copilot uses AI. Check for mistakes.

throw $lastException;
}
Comment on lines +279 to +381
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new retryRequestWithEvents method (lines 279-381) is a significant piece of logic that handles retry behavior with event dispatching, but there are no tests covering this method. While the existing retryRequest method may have tests, the new event-dispatching variant introduces additional complexity with ErrorEvent and RetryEvent dispatching that should be tested separately. Consider adding tests to verify:

  1. ErrorEvent is dispatched on the final attempt
  2. ErrorEvent is dispatched for non-retryable errors
  3. RetryEvent is dispatched before each retry
  4. Events contain correct attempt numbers and correlation IDs
  5. The method still correctly handles the retry flow

Copilot uses AI. Check for mistakes.
}
164 changes: 102 additions & 62 deletions src/Fetch/Concerns/PerformsHttpRequests.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

use Fetch\Enum\ContentType;
use Fetch\Enum\Method;
use Fetch\Events\RequestEvent;
use Fetch\Events\ResponseEvent;
use Fetch\Exceptions\RequestException as FetchRequestException;
use Fetch\Http\Response;
use Fetch\Interfaces\Response as ResponseInterface;
Expand Down Expand Up @@ -182,16 +184,33 @@ public function sendRequest(
// Start timing for logging
$startTime = microtime(true);

// Generate a correlation ID for event tracking
$correlationId = method_exists($handler, 'generateCorrelationId')
? $handler->generateCorrelationId()
: bin2hex(random_bytes(16));
Comment on lines +188 to +190
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The correlation ID generation on lines 188-190 uses method_exists($handler, 'generateCorrelationId') check before calling, but generateCorrelationId() is defined in the SupportsHooks trait which is used by ClientHandler. This dynamic check is unnecessary since the trait is already in use and will always be available. Consider removing the method_exists check and directly calling $handler->generateCorrelationId(), or if this code needs to work with handlers that don't use the trait, document why the check is necessary.

Suggested change
$correlationId = method_exists($handler, 'generateCorrelationId')
? $handler->generateCorrelationId()
: bin2hex(random_bytes(16));
$correlationId = $handler->generateCorrelationId();

Copilot uses AI. Check for mistakes.

// Log the request if method exists
if (method_exists($handler, 'logRequest')) {
$handler->logRequest($methodStr, $fullUri, $guzzleOptions);
}

// Dispatch request event
if (method_exists($handler, 'dispatchEvent')) {
$psrRequest = new GuzzleRequest($methodStr, $fullUri, $guzzleOptions['headers'] ?? []);
$handler->dispatchEvent(new RequestEvent(
$psrRequest,
$correlationId,
$startTime,
[],
$guzzleOptions
));
}
Comment on lines +199 to +207
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The PSR-7 Request object is created twice in the same method:

  1. Line 199 for dispatching the RequestEvent
  2. Line 381 (in the original code) and line 426 (in the retry logic) for retry handling

Creating the PSR request once at line 381 and reusing it at line 199 would be more efficient and ensure both the event and the retry logic use the exact same request object. Currently, the request object used in events might have different headers than the one used in retry logic if headers are modified between these points.

Copilot uses AI. Check for mistakes.

// Send the request (async or sync)
if ($handler->isAsync) {
return $handler->executeAsyncRequest($methodStr, $fullUri, $guzzleOptions);
return $handler->executeAsyncRequest($methodStr, $fullUri, $guzzleOptions, $correlationId);
} else {
return $handler->executeSyncRequest($methodStr, $fullUri, $guzzleOptions, $startTime);
return $handler->executeSyncRequest($methodStr, $fullUri, $guzzleOptions, $startTime, $correlationId);
}
}

Expand Down Expand Up @@ -336,14 +355,19 @@ protected function prepareGuzzleOptions(): array
* @param string $uri The full URI
* @param array<string, mixed> $options The Guzzle options
* @param float $startTime The request start time
* @param string|null $correlationId The correlation ID for event tracking
* @return ResponseInterface The response
*/
protected function executeSyncRequest(
string $method,
string $uri,
array $options,
float $startTime,
?string $correlationId = null,
): ResponseInterface {
// Generate correlation ID if not provided
$correlationId = $correlationId ?? bin2hex(random_bytes(16));

// Start profiling if profiler is available
$requestId = null;
if (method_exists($this, 'startProfiling')) {
Expand All @@ -353,79 +377,92 @@ protected function executeSyncRequest(
// Track memory for debugging
$startMemory = memory_get_usage(true);

return $this->retryRequest(function () use ($method, $uri, $options, $startTime, $requestId, $startMemory): ResponseInterface {
try {
// Record request sent event for profiling
if ($requestId !== null && method_exists($this, 'recordProfilingEvent')) {
$this->recordProfilingEvent($requestId, 'request_sent');
}
// Create the PSR request for events
$psrRequest = new GuzzleRequest($method, $uri, $options['headers'] ?? []);

// Send the request to Guzzle
$psrResponse = $this->getHttpClient()->request($method, $uri, $options);
return $this->retryRequestWithEvents(
function () use ($method, $uri, $options, $startTime, $requestId, $startMemory, $psrRequest, $correlationId): ResponseInterface {
try {
// Record request sent event for profiling
if ($requestId !== null && method_exists($this, 'recordProfilingEvent')) {
$this->recordProfilingEvent($requestId, 'request_sent');
}

// Record response received event for profiling
if ($requestId !== null && method_exists($this, 'recordProfilingEvent')) {
$this->recordProfilingEvent($requestId, 'response_start');
}
// Send the request to Guzzle
$psrResponse = $this->getHttpClient()->request($method, $uri, $options);

// Calculate duration
$duration = microtime(true) - $startTime;
// Record response received event for profiling
if ($requestId !== null && method_exists($this, 'recordProfilingEvent')) {
$this->recordProfilingEvent($requestId, 'response_start');
}

// Create our response object
$response = Response::createFromBase($psrResponse);
// Calculate duration
$duration = microtime(true) - $startTime;

// End profiling
if ($requestId !== null && method_exists($this, 'endProfiling')) {
$this->endProfiling($requestId, $response->getStatusCode());
}
// Create our response object
$response = Response::createFromBase($psrResponse);

// Create debug info if debug mode is enabled
if (method_exists($this, 'isDebugEnabled') && $this->isDebugEnabled()) {
$memoryUsage = memory_get_usage(true) - $startMemory;
$timings = [
'total_time' => round($duration * 1000, 3),
'start_time' => $startTime,
'end_time' => microtime(true),
];

if (method_exists($this, 'createDebugInfo')) {
$this->createDebugInfo($method, $uri, $options, $response, $timings, $memoryUsage);
// End profiling
if ($requestId !== null && method_exists($this, 'endProfiling')) {
$this->endProfiling($requestId, $response->getStatusCode());
}
}

// Trigger retry on configured retryable status codes
if (in_array($response->getStatusCode(), $this->getRetryableStatusCodes(), true)) {
$psrRequest = new GuzzleRequest($method, $uri, $options['headers'] ?? []);
// Create debug info if debug mode is enabled
if (method_exists($this, 'isDebugEnabled') && $this->isDebugEnabled()) {
$memoryUsage = memory_get_usage(true) - $startMemory;
$timings = [
'total_time' => round($duration * 1000, 3),
'start_time' => $startTime,
'end_time' => microtime(true),
];

if (method_exists($this, 'createDebugInfo')) {
$this->createDebugInfo($method, $uri, $options, $response, $timings, $memoryUsage);
}
}

throw new FetchRequestException('Retryable status: '.$response->getStatusCode(), $psrRequest, $psrResponse);
}
// Trigger retry on configured retryable status codes
if (in_array($response->getStatusCode(), $this->getRetryableStatusCodes(), true)) {
throw new FetchRequestException('Retryable status: '.$response->getStatusCode(), $psrRequest, $psrResponse);
}

// Log response if method exists
if (method_exists($this, 'logResponse')) {
$this->logResponse($response, $duration);
}
// Log response if method exists
if (method_exists($this, 'logResponse')) {
$this->logResponse($response, $duration);
}

return $response;
} catch (GuzzleException $e) {
// End profiling with error
if ($requestId !== null && method_exists($this, 'endProfiling')) {
$this->endProfiling($requestId, null);
}
// Dispatch response event
if (method_exists($this, 'dispatchEvent')) {
$this->dispatchEvent(new ResponseEvent(
$psrRequest,
$response,
$correlationId,
microtime(true),
$duration
));
}

// Normalize to Fetch RequestException to participate in retry logic
if ($e instanceof GuzzleRequestException) {
$req = $e->getRequest();
$res = $e->getResponse();
return $response;
} catch (GuzzleException $e) {
// End profiling with error
if ($requestId !== null && method_exists($this, 'endProfiling')) {
$this->endProfiling($requestId, null);
}

throw new FetchRequestException(sprintf('Request %s %s failed: %s', $method, $uri, $e->getMessage()), $req, $res, $e);
}
// Normalize to Fetch RequestException to participate in retry logic
if ($e instanceof GuzzleRequestException) {
$req = $e->getRequest();
$res = $e->getResponse();

// Fallback when we don't get a Guzzle RequestException (no request available)
$psrRequest = new GuzzleRequest($method, $uri, $options['headers'] ?? []);
throw new FetchRequestException(sprintf('Request %s %s failed: %s', $method, $uri, $e->getMessage()), $req, $res, $e);
}

throw new FetchRequestException(sprintf('Request %s %s failed: %s', $method, $uri, $e->getMessage()), $psrRequest, null, $e);
}
});
throw new FetchRequestException(sprintf('Request %s %s failed: %s', $method, $uri, $e->getMessage()), $psrRequest, null, $e);
}
},
$psrRequest,
$correlationId
);
}

/**
Expand All @@ -434,20 +471,23 @@ protected function executeSyncRequest(
* @param string $method The HTTP method
* @param string $uri The full URI
* @param array<string, mixed> $options The Guzzle options
* @param string|null $correlationId The correlation ID for event tracking
* @return PromiseInterface A promise that resolves with the response
*/
protected function executeAsyncRequest(
string $method,
string $uri,
array $options,
?string $correlationId = null,
): PromiseInterface {
return async(function () use ($method, $uri, $options): ResponseInterface {
return async(function () use ($method, $uri, $options, $correlationId): ResponseInterface {
$startTime = microtime(true);
$correlationId = $correlationId ?? bin2hex(random_bytes(16));

// Since this is in an async context, we can use try-catch for proper promise rejection
try {
// Execute the synchronous request inside the async function
$response = $this->executeSyncRequest($method, $uri, $options, $startTime);
$response = $this->executeSyncRequest($method, $uri, $options, $startTime, $correlationId);

return $response;
} catch (\Throwable $e) {
Expand Down
Loading
Loading