-
-
Notifications
You must be signed in to change notification settings - Fork 25
Add Connection Pooling & HTTP/2 Support #69
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
Add Connection Pooling & HTTP/2 Support #69
Conversation
✅ Deploy Preview for fetch-php canceled.
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Co-authored-by: Thavarshan <10804999+Thavarshan@users.noreply.github.com>
…tion, remove error_log Co-authored-by: Thavarshan <10804999+Thavarshan@users.noreply.github.com>
Co-authored-by: Thavarshan <10804999+Thavarshan@users.noreply.github.com>
|
@copilot please fix all code style issues to make CI pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds connection pooling and HTTP/2 support to improve performance for applications making multiple requests. The implementation introduces a new Fetch\Pool namespace with six classes managing connection lifecycle, DNS caching, and HTTP/2 configuration. The feature is opt-in and integrates with the existing ClientHandler through a new trait.
- Adds connection pooling to eliminate TCP handshake overhead and enable connection reuse
- Implements HTTP/2 support with multiplexing and header compression
- Includes DNS caching with configurable TTL to reduce lookup overhead
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Fetch/Pool/PoolConfiguration.php | Configuration class for connection pool settings with defaults and array conversion |
| src/Fetch/Pool/Http2Configuration.php | HTTP/2 settings including concurrent streams, window size, and curl options |
| src/Fetch/Pool/Connection.php | Individual connection lifecycle management with reusability checks |
| src/Fetch/Pool/HostConnectionPool.php | Per-host connection pool with borrow/return semantics and warmup support |
| src/Fetch/Pool/ConnectionPool.php | Multi-host pool manager with metrics tracking |
| src/Fetch/Pool/DnsCache.php | DNS resolution caching with TTL and automatic pruning |
| src/Fetch/Concerns/ManagesConnectionPool.php | Trait providing pool management methods to ClientHandler |
| src/Fetch/Interfaces/ClientHandler.php | Interface extension with pool management method signatures |
| src/Fetch/Http/ClientHandler.php | Integration of ManagesConnectionPool trait |
| tests/Unit/ManagesConnectionPoolTest.php | Tests for pool management trait integration |
| tests/Unit/ConnectionPoolTest.php | Comprehensive tests for pool classes |
| tests/Unit/ManagesDebugAndProfilingTest.php | Removed unused import |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Fetch/Pool/DnsCache.php
Outdated
| throw new NetworkException( | ||
| "Failed to resolve hostname: {$hostname}", | ||
| new \GuzzleHttp\Psr7\Request('GET', "https://{$hostname}/") |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The NetworkException constructor is being passed a Request object with the hostname in the URL. This may fail if the hostname contains special characters that are invalid in URLs. Consider using a more generic approach or validating/encoding the hostname before constructing the Request, or check if NetworkException actually requires a valid Request object.
| throw new NetworkException( | |
| "Failed to resolve hostname: {$hostname}", | |
| new \GuzzleHttp\Psr7\Request('GET', "https://{$hostname}/") | |
| // Encode hostname for use in URL (IDN and special chars) | |
| $asciiHost = function_exists('idn_to_ascii') ? idn_to_ascii($hostname, 0, INTL_IDNA_VARIANT_UTS46) : $hostname; | |
| $safeHost = rawurlencode($asciiHost); | |
| throw new NetworkException( | |
| "Failed to resolve hostname: {$hostname}", | |
| new \GuzzleHttp\Psr7\Request('GET', "https://{$safeHost}/") |
| // Enable HTTP/2 if available | ||
| 'version' => 2.0, |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTTP/2 is being unconditionally enabled in the HostConnectionPool's HTTP client creation, but there's no check if HTTP/2 is actually configured or should be used. This could force HTTP/2 even when the user hasn't enabled it via the Http2Configuration. Consider checking if HTTP/2 is enabled before setting this option.
| /** | ||
| * The connection pool instance. | ||
| */ | ||
| protected static ?ConnectionPool $connectionPool = null; |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using static properties for connection pool and DNS cache creates a shared state across all ClientHandler instances. This could lead to race conditions in multi-threaded environments (e.g., when using parallel processing or async operations) and makes it difficult to isolate connections per handler. Consider making these instance-level properties or adding proper synchronization mechanisms.
| /** | ||
| * The DNS cache instance. | ||
| */ | ||
| protected static ?DnsCache $dnsCache = null; |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the connection pool, the static DNS cache creates shared state across all ClientHandler instances which could lead to race conditions and unexpected behavior when multiple handlers are used concurrently. Consider making this an instance property instead.
| $existingCurl = $this->options['curl'] ?? []; | ||
| // Use + operator to preserve integer keys (CURL constants) | ||
| // and give priority to existing options over defaults | ||
| $this->options['curl'] = $existingCurl + $curlOptions; |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment states 'give priority to existing options over defaults', but with the + operator, if the same key exists in both arrays, the value from the left operand ($existingCurl) is kept and the right operand ($curlOptions) is ignored. This means HTTP/2 options from $curlOptions won't override existing curl options. If this is intentional, the comment is correct, but if HTTP/2 options should take precedence, use array_merge() or reverse the operands.
| 'curl' => [ | ||
| // Enable connection reuse | ||
| CURLOPT_TCP_KEEPALIVE => 1, | ||
| CURLOPT_TCP_KEEPIDLE => $this->config->getKeepAliveTimeout(), | ||
| CURLOPT_TCP_KEEPINTVL => 10, | ||
| ], |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The comment 'Enable connection reuse' is somewhat misleading. These CURLOPT_TCP_KEEPALIVE options configure TCP keep-alive packets to detect dead connections, not connection reuse per se. Connection reuse is handled at a higher level by the pool. Consider clarifying the comment to 'Configure TCP keep-alive for connection health monitoring' or similar.
| protected function resolveHostname(string $hostname): ?string | ||
| { | ||
| if (self::$dnsCache === null) { | ||
| return null; | ||
| } | ||
|
|
||
| try { | ||
| return self::$dnsCache->resolveFirst($hostname); | ||
| } catch (\Throwable) { | ||
| return null; | ||
| } | ||
| } |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The resolveHostname method silently catches and ignores all exceptions, returning null. Consider adding a PHPDoc comment explaining when null is returned (no DNS cache or DNS resolution failure) to help consumers of this method understand the behavior.
…ering Co-authored-by: Thavarshan <10804999+Thavarshan@users.noreply.github.com>
Fixed in 4c03857. The issues were:
All linting checks now pass ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Record connection latency for metrics. | ||
| * | ||
| * @param string $host The host | ||
| * @param int $port The port | ||
| * @param float $latency Latency in milliseconds | ||
| */ | ||
| public function recordLatency(string $host, int $port, float $latency): void | ||
| { | ||
| $this->metrics['total_latency'] += $latency; | ||
| } |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The recordLatency() method updates metrics but is never called anywhere in the codebase. For the connection pool metrics to be meaningful, this method should be integrated into the actual request/response flow to track latency.
| /** | ||
| * Create a new connection. | ||
| * | ||
| * @return Connection The new connection |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic number 10 used for TCP keep-alive interval without explanation. Consider extracting this to a named constant or configuration parameter for better maintainability.
Example:
public const DEFAULT_TCP_KEEPALIVE_INTERVAL = 10;Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot apply changes based on the comments in this thread |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Thavarshan <10804999+Thavarshan@users.noreply.github.com>
Applied changes from the review thread in commit 3d15f59:
|
PoolConfigurationclass for pool settingsConnectionclass for individual connectionsHostConnectionPoolclass for per-host poolingConnectionPoolclass for managing all poolsHttp2Configurationclass with separate curl/curl_multi optionsDnsCacheclassClientHandlerManagesConnectionPooltraitClientHandlerto use new traitClientHandlerinterface with new methodsConnectionPoolTest.phpwith 23 testsManagesConnectionPoolTest.phpwith 12 testsOriginal prompt
This section details on the original issue you should resolve
<issue_title>Connection Pooling & HTTP/2 Support</issue_title>
<issue_description>## Summary
Add intelligent connection pooling and HTTP/2 support to dramatically improve performance for applications making multiple requests to the same hosts.
Motivation
Connection pooling and HTTP/2 support provide significant performance benefits:
Currently, each Fetch PHP request creates a new connection, missing these optimization opportunities.
Proposed API
Implementation Details
Connection Pool Architecture