Skip to content

perf(s3): share DNS, TLS session and connection cache across requests - #169

Closed
loks0n wants to merge 1 commit into
mainfrom
perf-s3-curl-share
Closed

perf(s3): share DNS, TLS session and connection cache across requests#169
loks0n wants to merge 1 commit into
mainfrom
perf-s3-curl-share

Conversation

@loks0n

Copy link
Copy Markdown
Contributor

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::call creates 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 showed curl_exec in S3::write/S3::getInfo as 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)

requesttotaldnsconnecttls
cold80ms33ms37ms69ms
warm (shared)6–11ms000

Verified against PHP 8.5.7 / curl 8.19.0 under Swoole's native curl hook.

🤖 Generated with Claude Code

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>
@github-actions

Copy link
Copy Markdown

Thanks for contributing! This repository is a read-only mirror; development for this library happens in packages/storage in the utopia-php monorepo. Please open this pull request there instead.

@greptile-apps

Copy link
Copy Markdown

Greptile Summary

This PR adds a shared curl cache path for S3 requests. The main changes are:

  • A static curl share handle on the S3 device.
  • Lazy initialization of shared DNS, TLS session, and connection caches.
  • Attachment of the share handle to each per-request curl handle.

Confidence Score: 4/5

The shared S3 connection cache can break overlapping Swoole requests.

  • Concurrent S3 calls can enter curl_exec() with separate easy handles that share one connection cache.
  • The curl share initialization path can crash if initialization fails.
  • PHP and curl version requirements cover the new type and constants.

src/Storage/Device/S3.php

Important Files Changed

FilenameOverview
src/Storage/Device/S3.phpAdds a process-wide curl share handle for S3 requests, with concurrency and initialization risks in the changed curl setup path.

Fix All in Claude CodeFix All in Codex

Prompt To Fix All With AI
Fix 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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1Shared 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.

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.

Fix in Claude CodeFix in Codex


// Basic setup
if (self::$curlShare === null) {
self::$curlShare = curl_share_init();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2Failed 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.

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.

Fix in Claude CodeFix in Codex

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@loks0n