[net] Add SendPutReq to RCurlConnection for S3 uploads#22376
[net] Add SendPutReq to RCurlConnection for S3 uploads#22376JasMehta08 wants to merge 3 commits into
Conversation
jblomer
left a comment
There was a problem hiding this comment.
Nice! In general looks good with some minor comments.
Please also add a test to curl_env.cxx, similar to the existing one. We should be able with the credentials injected by the CI to write and read a file to S3.
On the commits, please remove the merge commit.
Test Results 20 files 20 suites 3d 4h 48m 6s ⏱️ For more details on these failures, see this check. Results for commit b685af8. ♻️ This comment has been updated with latest results. |
a0424d6 to
dbd63f4
Compare
|
I went through the comments, I have updated the code and added new test to I had a few question,
|
Thanks!
Interesting, I missed that libcurl already provides its own threshold. I think in this case we don't need to do anything about it except perhaps adding to the unit test uploading a chunk that is larger than this threshold. |
jblomer
left a comment
There was a problem hiding this comment.
As for the commits: I think it makes sense to add the ResetHandle() first as a separate commit.
|
@jblomer , I read the curl documentation and for versions above 7.69, the Expect: 100-continue threshold value has been increased to 1MB, but i think that would be ok for us, as the for each individual page per object the Expect: head would add latency, the Expect: head would only be sent for larger objects, probably if we put multiple pages in one object. Let me know if it a lower threshold would be preferred, I can look implement that too. Thanks! |
Looks good. Without additional data, let's trust that libcurl does the sensible thing. |
dbd63f4 to
dfdeee8
Compare
|
@jblomer and @silverweed , I have made the changes, I have split the PR into 3 commits,
I had a doubt in the second commit, I kept Thanks! |
dfdeee8 to
fd0ab12
Compare
|
@jblomer and @silverweed, The CI caught that |
fd0ab12 to
06ac60c
Compare
06ac60c to
b685af8
Compare
This Pull Request:
(Is a part of the GSoC 2026 project
S3 Backend for RNTuple.)The S3 backend needs the ability to upload objects via HTTP PUT. This PR adds a single synchronous PUT method to
RCurlConnection, following the same pattern as the existingSendHeadReq()andSendRangesReq()methods. SigV4 signing is handled automatically by the credentials already configured on the curl handle.This PR:
SendPutReq(const unsigned char *data, std::size_t length)toRCurlConnectionCURLOPT_UPLOADwith a read callback (CURLOPT_READFUNCTION/CURLOPT_READDATA) andCURLOPT_INFILESIZE_LARGEfor Content-LengthTServerSocketpattern from the existing test suite, verifying the PUT method, Content-Length header, and request bodyGTEST_SKIP()whenS3_ACCESS_KEY/S3_SECRET_KEYare not set) that does a PUT, HEAD, and GET round-trip against a real S3 endpointNo connection pooling or retry logic is included in this PR. Those are planned for Phase 4 (Weeks 10-11) as
SendMultiGetReq/SendMultiPutReqand a retry wrapper respectively.Checklist
Local test output
=== SendPutReq Wire-Level Test === Testing against locally built ROOT + RCurl library [Test 1] Basic PUT request PASS: SendPutReq returns success PASS: HTTP method is PUT PASS: Content-Length header matches payload size PASS: body matches payload [Test 2] Empty PUT (zero-length body) PASS: empty SendPutReq returns success PASS: HTTP method is PUT for empty body PASS: body is empty [Test 3] Large PUT (64 KB payload) PASS: large SendPutReq returns success PASS: Content-Length matches 64KB PASS: received body size == 64KB PASS: 64KB body content matches sent payload [Test 4] HEAD after PUT (sticky-option reset) PASS: initial PUT succeeds PASS: first request is PUT PASS: HEAD after PUT succeeds PASS: second request is HEAD (not PUT — sticky options were reset) PASS: HEAD returns correct Content-Length [Test 5] PUT after HEAD (reverse sticky-option check) PASS: first request is HEAD PASS: PUT after HEAD succeeds PASS: second request is PUT (NOBODY was reset) PASS: body arrives correctly after HEAD === Results === Passed: 20 Failed: 0 ALL TESTS PASSEDLocal test output (MinIO integration)