Repository navigation
Make Guzzle coroutine-safe and reduce AOP overhead - #656
Conversation
The formatter removes assignments captured by reference in nested closures, breaking Guzzle wait callbacks. Explicitly disable return_assignment rather than changing valid code to avoid the rewrite. Verified with the failing formatter reproduction, a clean formatting run and the credential-provider regression tests.
SQS, S3 and SES clients can share a custom credential provider across pools. Serialize provider invocation and promise completion using its callable identity so concurrent callers cannot drive each other’s pending credential work or receive another caller’s credentials. Preserve SDK memoization, original pool identity and cancellation cleanup without adding a credential cache. Cover actual pooled manager paths, caller-specific request signing, callable aliases, expiring credential reuse, failure and cancellation. Document safe memoization for providers whose callers share credentials. Verified focused regressions, Queue, Filesystem and Mail suites, full static analysis and formatting.
Pusher and Reverb shared one Guzzle multi-handler across concurrent callers, allowing one coroutine to drive another’s native transfers. Reuse async handlers only within the same client and coroutine, using weak client keys and non-copyable context. Preserve same-coroutine promise fan-out and connection reuse alongside the existing synchronous transport. Keep custom-handler validation and transport options intact. Reject connection caps that require an unsafe cross-coroutine shared handler and document bounded concurrency alternatives. Regression tests reproduce the native failure, preserve overlapping requests, and verify copied-context isolation, option forwarding and connection reuse. Broadcasting tests, static analysis and formatting pass.
Async requests created fresh handlers without the named connection's transport settings. Add an uncached handler factory that shares option derivation with the synchronous path, preserving transport sharing and multiplexing policy while keeping each async handler isolated. Cover sync and async option forwarding, request-preset replacement, and handler reuse. Connection tests and affected consumers pass on Guzzle 7 and 8; static analysis and formatting pass.
Guzzle's global task queue can run one request's callbacks while another request drains it. Shared multi-handlers can also drive foreign transfers. Install coroutine-local stock queues and guard pending promise operations and active transports at their public method boundaries. Guzzle has no global hooks for these boundaries, so use narrowly targeted AOP interception. Retain active native transfers until settlement and propagate cancellation through the owner's queue at exit. Completed results and idle handlers remain reusable. Reject classes loaded before proxy generation or with stale method coverage, and prepare framework proxies before PHPUnit test discovery without activating optional instrumentation. Cover ownership, retries, cancellation, memoized credential recovery, context copying, shared synchronous clients, bootstrap ordering and resource cleanup. Fix whole-class aspect matching so explicit constructor interception does not activate unrelated class-wide aspects. Document ownership and async transport configuration behavior. Validated on Guzzle 8/promises 3 and Guzzle 7/promises 2, including regression mutations. HTTP, DI, testing, Sentry, Telescope, Testbench package-mode and affected AWS/broadcasting checks pass, along with analysis and formatting.
Remove each transfer from the coroutine holder before canceling it. Exit cleanup now makes progress through its own collection instead of depending on the promise aspect to remove the entry indirectly. Normal settlement still releases transport ownership through the existing bookkeeping. The ownership, exit cleanup and retry tests pass on Guzzle 7 and 8.
Add focused Guzzle 7 CI coverage, including streaming transports, and a manual runner for unchanged upstream promise suites. Consolidate yielding callback coverage around real request and database transaction isolation. Exercise Saloon Digest authentication over a loopback connection so its test works with both native cURL negotiation and Guzzle authentication middleware. Validation: full framework suites passed on Guzzle 8 and 7; both upstream promise suites passed with and without the integration. Expanded compatibility checks and the final consolidated tests pass on their supported versions. A stock-queue mutation reproduces the cross-request context failure.
Add a developer harness using production provider registration and generated proxies, with a separate loopback origin. Measure startup independently from promise, HTTP, middleware, credential, broadcasting and cancellation workloads, including process CPU, memory, descriptors and cleanup. Document paired baseline comparisons, named connection reuse, the diagnostic pass-through aspect and measurement limits. Benchmark PHP files pass syntax checks and formatting.
Avoid rebuilding the generic aspect pipeline and reflecting the original closure for every intercepted call. Cache only immutable method chains, while resolving each aspect from the current container and keeping all invocation state local. Preserve priority, rebinding, scoped aspects, nested and concurrent dispatch, short-circuiting, references, and cleanup. Remove the unused AOP pipeline and its direct package dependency. Validated with focused AOP and consumer tests, both Guzzle dependency families, static analysis, formatting, the full 44,038-test suite, and package-mode Testbench. Measured guarded promise-chain CPU drops by about 48% from the initial implementation.
Publish generated proxies through a separate authoritative Composer loader so the application class map remains unchanged. Resolve the application loader through Composer registration metadata and preserve source lookup, regeneration, and validation before publication. Select exact targets directly, compile wildcard patterns once, enumerate the fixed generator directory without Finder, and construct the parser and printer only when code generation is required. These changes remove repeated map scanning and two retained full-map copies across the initial implementation, while preserving proxy fingerprints and visitor behavior. Validated cold and cached startup, PSR-4 sources, overrides, atomic failures, and repeated applications. Both dependency families pass focused coverage; the full suite passes 44,038 tests, package-mode Testbench passes 635 tests, and the package consumer fixture passes 6 tests. Measured retained heap falls by 2.88 MiB and cached bootstrap time by about 57% versus the initial ownership implementation.
Record paired Guzzle 7 and 8 measurements, shared dispatcher improvements, cold and cached bootstrap, retained heap and RSS, cancellation, and repeated-batch cleanup. Identify the native Swoole select delay separately and distinguish refreshed measurements from earlier runs. Include the profile-backed explanation of remaining dispatch, queue, and startup costs. Report autoloader configuration in the reusable harness and release its measured closure before retained-memory snapshots. Keep raw reports and temporary investigation artifacts outside the repository. Full framework, package-mode Testbench, package consumer, focused compatibility, formatting, and static-analysis checks passed.
Translate measured per-request CPU overhead into deployment-wide core estimates at example traffic levels, distinguishing arithmetic projections from throughput measurements. Explain bounded worker metadata and temporary in-flight tracking, and identify the measured outgoing paths separately from serialized credential contention. Use public reproducibility instructions in the benchmark guide and identify the published implementation revision. Benchmark syntax checks and the latest base streaming tests pass; no timing measurements were rerun.
Preserve the technical plan: failure modes, ownership guarantees, bootstrap and cleanup, SDK consumers, supported-version coverage, and benchmark methodology. Link the measured results and retain the reasons for the chosen boundaries without implementation coordination or progress records.
PHPUnit captures output outside the automatic test coroutine, allowing empty-output expectations to pass without observing printed content. Keep the environment bootstrap and response factory tests in the native output-capture context and assert the streamed cancellation output directly. Document the current limitation and track the integration work for PHPUnit issue #7039. Both affected test classes and the focused formatting check pass.
Record upstream PR #7040 alongside issue #7039 so the coroutine test integration follow-up can track the proposed fix.
Freeze the PHP clock when constructing the SQLite, Swoole and worker-array contract limiters. Elapsed I/O and scheduling time must not change consecutive decisions that the shared contract expects to occur at one instant. Keep the existing clock advancement and global cleanup behavior. Validated the affected store tests, the delayed SQLite reproduction, and the full framework suite.
Keep the native alarm for non-yielding work and observe root completion through a private buffered channel. Continue checking remaining children against the same deadline without taking the join slots used by the tested code. Route expiry through the native PHPUnit timeout result and cancel blocked children without throwing into raw callbacks. Cover busy loops, sleeping tests, abandoned children, normal root and child joins, and disabled limits. Preserve coroutine-count assertions relative to their initial infrastructure state. Full framework, Testbench, dogfood, formatting and static-analysis checks pass. A temporary path count verified that after-root polling is rare; no diagnostic code is retained.
Use a separate authoritative loader for source overrides and their generated proxies so application maps are not copied or polluted. Reset unloaded overrides between application boots, accept loaded replacements only from the same canonical source, and record original proxy source paths as generated metadata. Separate replacement filenames preserve ordinary proxies retained by the persistent loader. Generate proxies beneath bootstrap/cache/aop so overlapping releases sharing storage cannot overwrite each other. Update cache clearing, the about report, Testbench cleanup and documentation. Use xxh128 for internal fingerprints and helper names, retaining source-path, content and rule invalidation coverage. Validate repeated boot, cleanup, replacement priority, source regeneration and shared-storage releases. Full framework, Testbench, dogfood, formatting and analysis pass. Preserve the benchmark revision and explicitly identify later unmeasured bootstrap corrections in the report; runtime dispatch and ownership checks are unchanged.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
|
|
@cubic-dev-ai review |
@binaryfire cubic can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 124,545 of the 120,000 allowed lines of code this month. Reviews resume on 10 October 2026 (in 1 day). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
PR Summary by QodoMake Guzzle coroutine-safe and reduce AOP overhead
AI Description
Diagram
High-Level Assessment
Files changed (106)
|
Code Review by Qodo
1. A second autoloader hides app proxies
|
Waiting for memoized credentials drains Guzzle callbacks. A callback starting another SDK request could wait for a provider lock held by the same coroutine and deadlock. Keep cross-coroutine serialization while allowing nested calls by the native owner. Only the outermost frame acquires and releases the lock, and every invocation still waits for its own completed result. Clear the bounded owner map during normal release and test cleanup. Cover nested requests through two framework-created S3 clients using callable aliases, cached credentials, refreshes and subsequent use from another coroutine. The regression hangs with reentry disabled. Full framework, Testbench, package integration, formatting and static analysis checks pass. Clarify that existing credential benchmarks predate this correction.
The early-loaded target test correctly triggered its bootstrap exception in CI, but PHP wrote it to standard output while the assertion read standard error. Set the subprocess error destination explicitly. Remove obsolete manual-interception fallbacks from Sentry and Telescope tests now that the PHPUnit extension generates their Guzzle proxies before discovery. Correct the testing helper guidance without weakening bootstrap validation. Bootstrap, Sentry and Telescope tests pass, along with formatting and static analysis. The full framework and package-mode suites also pass.
Use SplObjectStorage::offsetExists() for both shared-reference lookups instead of contains(), which is deprecated in PHP 8.5. The replacement is available on PHP 8.4 and preserves object membership semantics. Verified on PHP 8.4, including stored null values. Notification transport serialization tests and the full framework suite pass.
Guzzle's process-wide promise queue can run one request's callbacks while another request drains it. In a long-lived coroutine worker, those callbacks can resolve the wrong request or tenant, or use a database transaction owned by another coroutine. Sharing an active cURL multi-handler has a second failure mode: one coroutine can drive another's native transfers and crash the worker.
This PR makes those operations stay with their owning coroutine, fixes the affected SDK consumers, and reduces the AOP cost of enforcing that rule. The same AOP improvements benefit existing instrumentation such as Sentry and Telescope.
Keep Guzzle callbacks and transfers in their owning coroutine
A mutable promise belongs to the native coroutine that creates it. Pending work must be registered, driven, settled and canceled there. Completed results remain reusable across coroutines, and an idle multi-handler can be reused sequentially. A promise that has adopted another pending promise is still unfinished work.
Each coroutine receives a normal Guzzle task queue through the supported
Utils::queue()setter. Pending promise operations and active multi-handlers are checked before another coroutine can run their callbacks or change their state. A conflicting operation fails with an explanation of how to create and finish the work in the same coroutine. Applications retain concurrency through native coroutines andparallel(); they share completed results rather than handing pending operations between requests.Transfer ownership is reserved before request preparation, because reading an upload body can yield. Active transfers remain alive until settlement or cancellation. At coroutine exit, unfinished transfers are canceled and the owner's queue drains their rejection handlers, allowing dependent promises and memoized providers to recover. Queue and transfer state is excluded from copied coroutine context. Outside coroutines, normal Guzzle queue and shutdown behavior is retained.
Registration belongs to the HTTP provider, so protection applies in HTTP requests, queue workers and console commands without requiring an observability package. It covers Guzzle's mutable promises and cURL multi-handler; arbitrary third-party promise implementations and custom transports do not acquire these guarantees automatically.
Why AOP is needed
Guzzle exposes a queue replacement hook, but no global hook for promise construction, pending promise operations, or multi-handler admission and driving. An application client subclass or outer middleware cannot intercept every promise created inside Guzzle or an SDK.
The aspects therefore target a small set of public methods: the mutable promise constructor,
then,wait,cancel,resolveandreject; and the multi-handler's__invoke,tick,executeandclosewhere available. They check ownership around the original methods without copying their implementations or replacing Guzzle's scheduling.Proxy generation runs before provider boot. It rejects vendor classes loaded too early and loaded proxies missing required method interception, rather than silently leaving protection inactive. The error explains lazy client registration. Generated method metadata verifies coverage during bootstrap, without adding that inspection to each invocation.
Correct shared SDK and HTTP consumers
AWS credentials. SQS, S3 and SES clients can share a callable credential provider even when the clients themselves are pooled separately. The provider can retain an in-flight HTTP promise. A shared wrapper serializes provider invocation and its promise completion by callable identity, while each caller executes in its own coroutine and receives its own result or exception. Callbacks drained during that wait may request credentials again in the same coroutine without deadlocking; other coroutines still wait for the outermost call to finish. Existing SDK memoization, pool fingerprints, completed credential reuse and cancellation behavior are preserved. There is no additional credential cache or sharing of one caller's credentials with another.
Pusher and Reverb. The default client previously shared an async multi-handler between concurrent callers. Async handlers are now reused within the same client and coroutine, preserving same-coroutine fan-out and connection reuse. Weak client keys and coroutine-local storage release handlers when they are no longer needed. Synchronous transport reuse, middleware, timeouts,
on_stats, custom handlers and supported transport options remain intact. Connection caps that require sharing one active multi-handler across coroutines are rejected; callers should bound coroutine concurrency or use rate limiting.Named HTTP connections. Async requests previously lost the named connection's handler settings when creating a fresh handler.
Factory::newConnectionHandler()now shares option derivation with the cached synchronous path. Async handlers remain isolated while retaining their registered transport settings. Per-call request presets and explicit handler/client overrides keep their existing meaning.Reduce shared AOP work
The previous dispatcher rebuilt a generic pipeline for every intercepted call and reflected its closure to find the target instance. It now caches an immutable chain per class and method, containing only aspect class names and continuation closures. Each invocation still resolves its aspect from the current container. Generated proxies pass the intercepted instance directly, or null for static calls.
This preserves priority, short-circuiting, original-method bypass, exceptions, references, variadics, scoped aspects, container rebinding, and nested or concurrent calls. No target object, join point, container or aspect instance is retained in the chain. The unused AOP-specific pipeline and its dependency are removed; the general framework pipeline is unchanged.
Bootstrap now selects exact class rules directly, prepares wildcard matching once per rule, and creates the parser and printer only when generation is required. A separate authoritative Composer loader contains only proxy entries, keeping the application's source map unchanged. This avoids retaining copies of the complete application map. Composer discovery continues to identify the application loader, including after test resets. Content fingerprints, PSR-4 lookup, source overrides, proxy regeneration and validation before publication remain intact.
Make class-map overrides safe across application boots
The documented provider
classMap()API previously copied the entire Composer application map when adding replacements. It also rejected the same replacement on a second Testbench boot, while test cleanup left unloaded overrides active for later applications that had not registered them.Replacements now use their own authoritative Composer loader and leave the application map unchanged. Automatic test cleanup unregisters that loader and its generated replacement proxies together. A loaded target can be registered again only from the same canonical source; generated proxies record their original source as class metadata so the same rule applies to them. Different-source replacements still fail. Replacement proxies use separate, fixed filenames so they cannot overwrite ordinary proxies retained between application boots.
Generated proxies now live in
bootstrap/cache/aop, alongside each release's other compiled framework files. Two overlapping releases sharingstoragepreviously wrote the same proxy paths, allowing an older worker to load the newer release's code. The release-local directory prevents that collision without per-release hashes or accumulating files in shared storage. Cache clearing, Testbench cleanup and theaboutreport follow the new location;aboutalso recognizes the generated PHP files correctly.Approaches considered and rejected
The pass-through-aspect variant used below is a measurement control, not a production bypass. All measured outgoing request paths remain supported; the rejected items above are implementation approaches.
AOP performance improvements
These comparisons are against the initial ownership implementation, not against an uninstrumented framework:
The compiled-chain comparison reduced pass-through dispatch from 31.62 to 15.80 µs/op. A separate comparison isolated direct instance passing, reducing guarded chain CPU from 21.72 to 20.48 µs/op. The earlier complete matrix measured 20.50 µs/op; the final bootstrap refresh measured 20.57 with the same dispatch code.
Ownership overhead across outgoing paths
The following compares the unchanged framework with the protected implementation. Values are client CPU microseconds per operation, not batch latency. The loopback origin's CPU is excluded.
† Refreshed paired measurements after the bootstrap corrections. Other rows use the earlier complete matrix with the same dispatch and ownership implementation. Rows from different runs are not a controlled concurrency-scaling comparison.
Async completion through
Utils::unwrap()makes 27 intercepted calls per operation at concurrency one, compared with nine in the promise probe. A separate pass-through comparison separates dispatch from ownership and queue handling; it explains the larger async and cancellation costs. There were no GC runs inside the timed operation loops.The shared credential-provider workload deliberately serializes calls to one provider and includes a 1 ms endpoint delay. At concurrency eight, p95 latency increased from 5.396 to 22.474 ms. This includes waiting for the provider lock; it is not all AOP overhead. Completed memoized credentials remain reusable.
Guzzle 7 shows comparable absolute costs on the focused workloads: approximately 17 µs for promise chains, 43 µs for synchronous HTTP, 90 µs for async concurrency one, 53 µs for async concurrency eight, and 54 µs for cancellation. The source implementation does not need a separate version-specific AOP path.
Memory bounds and capacity planning
At the benchmarked revision, the integration retains approximately 124–188 KiB of additional PHP heap per warmed worker across the measured workloads, with roughly 0.94–1.19 MiB additional RSS. RSS includes shared code pages, so it is not a measure of private memory added to every worker. Allocated PHP memory after exit remained 16 → 16 MiB.
This retained metadata is not added for each request. Temporary ownership state follows outstanding promises and native transfers and is released as work finishes or is canceled. Repeated batches showed the same report-bookkeeping growth on baseline and protected processes, with no additional retained growth, and returned to the same descriptor count. Applications still need bounded outgoing concurrency and queues; the integration does not cap how much unfinished work an application can create.
At the measured 38.03 µs additional CPU per outgoing request for coroutine-parallel HTTP at concurrency 32:
These are arithmetic projections, not throughput measurements at those traffic levels. The tested HTTP and Pusher paths suggest a planning range of approximately 40–110 µs/request, or 0.4–1.1 additional cores at 10,000 outgoing requests/sec. Credential-provider contention is a separate serialized workload. The integration adds predictable CPU work without a request-count-dependent retained-memory cost.
Cached startup compared with the unchanged framework is 69.95 → 78.15 ms. One cold process requiring proxy generation took 132.45 ms; cold startup is reported separately from repeated warm measurements.
Measurement conditions and validation
Measurements precede the subsequent class-map override reset and release-local proxy corrections, and the switch to
xxh128for internal proxy fingerprints and helper names. These add generated source metadata and bootstrap lookup for registered overrides, without changing the per-request dispatcher or ownership checks. Their startup and retained-memory effects have not been remeasured; the resource figures describe the benchmarked revision.The credential-provider measurements also precede the same-coroutine reentry correction. Its active-owner lookup and cleanup have not been remeasured. This correction does not change the HTTP, Pusher or shared AOP dispatch paths.
Measurements used PHP 8.4.25, Swoole 6.2.2, cURL 8.5.0, CLI OPcache enabled, JIT disabled, and an Intel Core i7-7700 with six logical processors exposed. Both checkouts used independently installed identical dependencies and optimized Composer autoloading. The primary matrix used Guzzle 8.2.0/promises 3.0.2; focused comparisons also used Guzzle 7.15.5/promises 2.5.3. Each workload used three alternating process pairs with warmup and three measured samples per process. The report includes ranges, operation counts, latency and resource snapshots.
Async/Pusher wall-time measurements at higher concurrency include the separate native hooked-select readiness stall addressed by Swoole #6290. CPU is reported separately rather than using that stall to make ownership overhead appear negligible. The default AWS Guzzle transport also takes the async transport path for synchronous SDK calls.
Regression coverage exercises real generated proxies, yielding request/tenant and database-transaction isolation, pending and adopted promises, cancellation, owner exit, upload preparation, shared clients, credential memoization, Pusher fan-out, cold/cached bootstrap and cleanup. Representative tests fail with the protections removed. Both supported Guzzle dependency families and their upstream promise suites were validated, together with the full framework suite, affected SDK consumers, Sentry/Telescope, package-mode Testbench, formatting and static analysis. CI adds a focused Guzzle 7 compatibility job alongside the normal dependency resolution.
The shared PHPUnit extension prepares framework proxy targets before test discovery without enabling optional instrumentation and resets test-owned queues safely. The formatter's
return_assignmentrule is also disabled because it removes assignments captured by reference in nested closures, breaking valid Guzzle wait callbacks.Validation also exposed test-harness gaps. Coroutine tests now retain PHPUnit's native busy-loop alarm and enforce the same deadline while the test or its children are suspended, reporting the named test through PHPUnit's normal timeout result. Synchronous output-assertion tests run outside coroutines pending the upstream output-buffer integration. PHP-clock rate-limiter contract tests freeze their clock so I/O delays cannot change back-to-back decisions.
Notification transport snapshots also use
SplObjectStorage::offsetExists()instead of the PHP 8.5-deprecatedcontains(), retaining PHP 8.4 compatibility.The technical design is in
docs/plans/2026-10-09-guzzle-coroutine-ownership.md. Reproducible commands and complete measurements are intests/Benchmarks/GuzzleOwnership/README.mdandresults.md.Note
Make Guzzle coroutine-safe and reduce AOP dispatch overhead
CoroutineOwnershipException(CoroutineOwnership.php, Aspects/PromiseOperationAspect.php).CoroutineTaskQueue,CoroutineState, andTransportStateso each coroutine has its own Guzzle task queue and pending transfers. Coroutine exit cancels unfinished transfers and drains that coroutine's callbacks (CoroutineTaskQueue.php).AspectManagercaches compiled closure chains instead of aspect-name arrays,ProxyDispatcherinvokes cached chains with the intercepted instance attached to the join point (AspectManager.php, ProxyDispatcher.php).SerializedCredentialProviderfor SQS, S3, SES mail, and filesystem clients, so shared providers serialize their work per caller. Adds shared Pusher/Reverb transports for synchronous calls and coroutine-local handlers for asynchronous calls (BroadcastManager.php).storage/framework/aoptobootstrap/cache/aop, switches generated fingerprints and helper names to xxh128, and addsProxySource/ProxyMethodattributes to generated proxies. Adds a CI job for Guzzle 7 compatibility.InvalidArgumentExceptionfor host/total connection-limit options;ProceedingJoinPoint::getInstancenow reports the dispatcher-supplied instance (null for static calls) instead of reflecting the closure; cached AOP entries are closures, so customAspectManagerconsumers must be updated; existing proxy caches understorage/framework/aopare stale and regenerate under the new path.Macroscope summarized 1e7f66f.