perf(s3): share DNS, TLS session and connection cache across requests#169
perf(s3): share DNS, TLS session and connection cache across requests#169loks0n wants to merge 1 commit into
Conversation
Every S3 call previously created and destroyed its own curl handle, paying a fresh DNS lookup, TCP connect and full TLS handshake per operation. Under load (e.g. build storms hammering cluster DNS) this dominated request time: measured 80ms cold vs 6ms warm per request in-cluster, and seconds per call when DNS is degraded. A process-wide curl share handle keeps the DNS cache, TLS session cache and connection pool shared across the per-call easy handles, which stay per-call and therefore coroutine-safe. Verified working under Swoole's native curl hook (SWOOLE_HOOK_ALL). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for contributing! This repository is a read-only mirror; development for this library happens in |
Greptile SummaryThis PR adds a shared curl cache path for S3 requests. The main changes are:
Confidence Score: 4/5The shared S3 connection cache can break overlapping Swoole requests.
src/Storage/Device/S3.php Important Files Changed
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/Storage/Device/S3.php:740
**Shared Connection Cache Races**
When Swoole runs overlapping S3 operations, each coroutine creates its own easy handle but now points it at the same process-wide connection cache. libcurl does not support sharing `CURL_LOCK_DATA_CONNECT` across concurrent transfers, so concurrent `curl_exec()` calls can intermittently fail or reuse connection state incorrectly under the workload this change targets.
### Issue 2 of 2
src/Storage/Device/S3.php:737
**Failed Share Init Crashes**
If `curl_share_init()` returns `false`, this assignment writes a boolean into a `?\CurlShareHandle` static property before the existing S3 error handling can run. That turns a curl setup failure into a PHP `TypeError` for the request instead of a controlled storage error or a fallback to an unshared handle.
Reviews (1): Last reviewed commit: "perf(s3): share DNS, TLS session and con..." | Re-trigger Greptile |
| self::$curlShare = curl_share_init(); | ||
| curl_share_setopt(self::$curlShare, CURLSHOPT_SHARE, CURL_LOCK_DATA_DNS); | ||
| curl_share_setopt(self::$curlShare, CURLSHOPT_SHARE, CURL_LOCK_DATA_SSL_SESSION); | ||
| curl_share_setopt(self::$curlShare, CURLSHOPT_SHARE, CURL_LOCK_DATA_CONNECT); |
There was a problem hiding this comment.
When Swoole runs overlapping S3 operations, each coroutine creates its own easy handle but now points it at the same process-wide connection cache. libcurl does not support sharing CURL_LOCK_DATA_CONNECT across concurrent transfers, so concurrent curl_exec() calls can intermittently fail or reuse connection state incorrectly under the workload this change targets.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Storage/Device/S3.php
Line: 740
Comment:
**Shared Connection Cache Races**
When Swoole runs overlapping S3 operations, each coroutine creates its own easy handle but now points it at the same process-wide connection cache. libcurl does not support sharing `CURL_LOCK_DATA_CONNECT` across concurrent transfers, so concurrent `curl_exec()` calls can intermittently fail or reuse connection state incorrectly under the workload this change targets.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| // Basic setup | ||
| if (self::$curlShare === null) { | ||
| self::$curlShare = curl_share_init(); |
There was a problem hiding this comment.
If curl_share_init() returns false, this assignment writes a boolean into a ?\CurlShareHandle static property before the existing S3 error handling can run. That turns a curl setup failure into a PHP TypeError for the request instead of a controlled storage error or a fallback to an unshared handle.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Storage/Device/S3.php
Line: 737
Comment:
**Failed Share Init Crashes**
If `curl_share_init()` returns `false`, this assignment writes a boolean into a `?\CurlShareHandle` static property before the existing S3 error handling can run. That turns a curl setup failure into a PHP `TypeError` for the request instead of a controlled storage error or a fallback to an unshared handle.
How can I resolve this? If you propose a fix, please make it concise.
What
Attach a process-wide curl share handle (
CURL_LOCK_DATA_DNS+CURL_LOCK_DATA_SSL_SESSION+CURL_LOCK_DATA_CONNECT) to every S3 request. Easy handles remain per-call (coroutine-safe); only the DNS cache, TLS session cache and connection pool are shared.Why
S3::callcreates and destroys a curl handle per operation, so every PUT/HEAD/GET pays DNS + TCP + TLS setup. Profiling Appwrite Cloud's builds worker during a backlog showedcurl_execinS3::write/S3::getInfoas the top CPU/wall sink; cluster DNS degraded to 1.7s–16s per lookup under the concurrent-clone burst, multiplying per-op cost.Measured (in-cluster, DO Spaces fra1, Swoole SWOOLE_HOOK_ALL)
Verified against PHP 8.5.7 / curl 8.19.0 under Swoole's native curl hook.
🤖 Generated with Claude Code