Repository navigation
fix(rest): make HttpClient safe for concurrent requests - #1090
smaheshwar-pltr wants to merge 3 commits into
Conversation
HttpClient sent every request through one cpr::ConnectionPool, and libcurl does not support sharing a connection cache between threads. Overlapping requests, for example from token refresh or the metrics executor, could crash. Give each in-flight request its own pool and reuse idle pools. HttpResponse now copies the status, body and headers out of cpr::Response, whose curl handle must be freed before its pool is reused. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| cpr::Response response_; | ||
| int32_t status_code_; | ||
| std::string body_; | ||
| std::unordered_map<std::string, std::string> headers_; |
There was a problem hiding this comment.
The changes in this class are due to this. The cpr::Response maintains a curl handle so destruction affects the pool. I'm therefore instead storing what's needed explicitly. See below / PR description for more details
| } | ||
| } | ||
|
|
||
| // RestCatalog shares its HttpClient with token refresh and metrics reporting. |
There was a problem hiding this comment.
With libcurl 8.22.0, this fails in 6 of 6 runs with heap corruption (free(): corrupted unsorted chunks or double free or corruption) on main. But with 8.18.0 specifically it doesn't reproduce. That's the version CI uses, so CI doesn't catch it
There was a problem hiding this comment.
I wonder if this is sufficient? If a regression test is preferable I can try to think of other approaches but I don't see anything easy for me to achieve that right now
There was a problem hiding this comment.
Yeah, I can see that the gh actions runner has 8.18.0-1ubuntu2.8 installed[1].
You mean without your fix this fails, right?
[1] https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2604-Readme.md
There was a problem hiding this comment.
Ah realise I wasn't super clear: with 8.22.0 this fails on current main (without this PR's fix) fairly reliable as far as I can tell. With this fix, it passes.
But CI runs 8.18, as you say for which this bug doesn't surface for this test. (Given we're violating libcurl's concurrency precondition, it doesn't provide us with fixed erroneous behaviour across versions for regression testing hence it's a bit annoying)
| } | ||
|
|
||
| private: | ||
| std::unique_ptr<cpr::ConnectionPool> Acquire() { |
There was a problem hiding this comment.
This setup made sense to me, wonder if folks have thoughts on this approach?
| Result<HttpResponse> HttpClient::Head( | ||
| const std::string& path, const std::unordered_map<std::string, std::string>& headers, | ||
| const ErrorHandler& error_handler, auth::AuthSession& session) { | ||
| ICEBERG_ASSIGN_OR_RAISE(auto request_uri, RequestUri(path)); |
There was a problem hiding this comment.
Why remove this line? I think when base_uri is configured, Head() now passes relative paths directly to authentication and libcurl.
There was a problem hiding this comment.
Ah great catch thank you, I messed this up in the rebase. I did think it was a bit odd that nothing failed here, so I've added a regression test in 1f820cc for the HTTP client to catch breaks like this (in this PR). Happy to remove / separate out but figured it'd be fine to go in with this
Head() lost its RequestUri() call in a rebase, so relative paths were sent without the base URI. Restore it, and check that every request method resolves relative paths. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| EXPECT_THAT(client.Get("v1/get", {}, {}, handler, session), | ||
| IsError(ErrorKind::kInvalidArgument)); | ||
| EXPECT_EQ(session.request().url, "https://catalog.example/api/v1/get"); |
There was a problem hiding this comment.
See #1090 (comment) for the motivation behind these new assertions
Closes #1089.
Each request now borrows a
cpr::ConnectionPoolthat no other request is using and returns it afterwards, so connections are still reused.HttpResponsecopies the status, body and headers out ofcpr::Responseinstead of keeping it, because its curl handle stays attached to the pool until it's freed.The new integration test crashes without this change on some libcurl versions, such as 8.22.0, but note that it does not crash on 8.18.0, which CI uses, unfortunately.
AI disclosure: this PR was written by Claude and I.