Reuse the curl handle and grow the response buffer geometrically - #57
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe provider HTTP path now caches and resets a curl handle across requests. Response buffers start with a 32 KiB capacity and grow geometrically with non-throwing allocation. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 9 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Benchmarked before changing anything, as the issue asked. provider_do_curl_request() built a fresh handle for every request, and a fresh handle knows of no connection, so each batch opened its own. Measured against a local stub: 200 requests made 200 TCP connections. With the handle kept for the life of the backend that becomes 200 requests over 1 connection, and 274ms falls to 189ms over loopback, where establishing a connection is about as cheap as it can be. Against a real provider each avoided connection also avoids a TLS handshake, so the saving there is one to two extra round trips per batch to a host that is rarely nearby. curl_easy_reset() clears the options without disturbing the connection pool, which is what lets reuse survive between requests. That is the load-bearing assumption of the whole change, so it was verified rather than taken from the documentation: the connection count above is the evidence. The handle is deliberately never cleaned up, since it lives as long as the backend and an idle keep-alive connection is closed by the server at its own timeout. The response buffer grew to exactly the size wanted on every write callback, which is a repalloc for every chunk curl delivers, each liable to copy everything received so far. It now tracks capacity and doubles, so a response of any size costs a handful of reallocations rather than hundreds. A note for whoever benchmarks this next. A first attempt showed the change making things twenty-six times slower, which was the measuring instrument rather than the code: Python's http.server does not set TCP_NODELAY, so once keep-alive put every request on one socket each one paid the 40ms delayed-ACK stall, 200 of which is the 8.4 seconds observed. Setting disable_nagle_algorithm on the stub removed it entirely. Closes #28
repalloc raises on failure and the callback runs inside curl_easy_perform(), so the longjmp abandoned the handle mid-transfer. With the handle now cached for the life of the backend, every later request reused the wreckage: measured against a stub, a response over MaxAllocSize failed and then every subsequent request in that backend failed with "Failed initialization". The callback now checks the size itself, uses MCXT_ALLOC_NO_OOM, and returns short so curl reports CURLE_WRITE_ERROR through the existing error path. Doubling that overshoots MaxAllocSize retries at the exact size, restoring the ~1GB ceiling that geometric growth had halved.
mason-sharp
force-pushed
the
fix/issue-28-curl-reuse
branch
from
August 12, 2026 02:02
21f3185 to
7b7ce34
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/provider_common.c`:
- Around line 486-492: Update the alloc_failed branch in the response error
handling to use a neutral message such as “could not grow response buffer,”
covering both size validation and allocation failures; do not report these cases
as the response being too large. Preserve the existing curl_easy_perform() error
path for other failures.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f61cf37-d702-40b3-89b5-943c9452483e
📒 Files selected for processing (2)
src/provider_common.csrc/provider_common.h
A single alloc_failed flag reported an allocation refused for want of memory as a response too large to buffer, which points the operator at their provider when the response size may be perfectly ordinary and the server is simply short of memory. Record which of the two happened and say so. Raised by CodeRabbit on #61.
mason-sharp
approved these changes
Aug 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #28.
The issue said "benchmark and, if warranted", so I benchmarked before changing anything, against a local stub HTTP server that counts TCP connections as well as requests.
Measured
One connection per request became one connection for all of them, and wall clock fell 31% over loopback, where establishing a connection is about as cheap as it will ever be. Against a real provider each avoided connection also avoids a TLS handshake, so the saving is one to two further round trips per batch to a host that is rarely nearby. That part is arithmetic from the measured 200-to-1, not a separate claim.
The change
provider_do_curl_request()built a fresh handle per request, and a fresh handle knows of no connection. It now keeps one for the life of the backend.curl_easy_reset()clears the options without disturbing the connection pool, which is the load-bearing assumption of the whole thing, so I verified it rather than taking it from the documentation: the connection count in the table is that verification.The handle is deliberately never cleaned up. It lives as long as the backend, and an idle keep-alive connection is closed by the server at its own timeout, so nothing is held open indefinitely.
Separately,
ResponseBuffergrew to exactly the size wanted on every write callback, meaning arepallocper chunk curl delivers, each liable to copy everything received so far. It now tracks capacity and doubles, so a response of any size costs a handful of reallocations rather than hundreds.A warning for whoever benchmarks this next
My first run showed the change making things twenty-six times slower, at 8.4 seconds against 320 ms. That was the measuring instrument, not the code. Python's
http.serverdoes not setTCP_NODELAY, so once keep-alive put all 200 requests on a single socket, each one paid the 40 ms delayed-ACK stall; 200 × 42 ms is precisely the 8.4 seconds observed. Settingdisable_nagle_algorithmon the stub removed it completely and produced the numbers above.I mention it because the failure is specific to reusing a connection, so anyone re-running this comparison with a naive stub will hit it and could easily conclude the change is a regression. Real HTTP servers set
TCP_NODELAY.Test plan
One caveat on the wall-clock figures: they come from a single run each against a local Python stub, so treat 31% as the shape of the result rather than a precise number. The connection count is the exact and reproducible part.