Skip to content

[net] Add SendPutReq to RCurlConnection for S3 uploads#22376

Open
JasMehta08 wants to merge 3 commits into
root-project:masterfrom
JasMehta08:curl-send-put-req
Open

[net] Add SendPutReq to RCurlConnection for S3 uploads#22376
JasMehta08 wants to merge 3 commits into
root-project:masterfrom
JasMehta08:curl-send-put-req

Conversation

@JasMehta08
Copy link
Copy Markdown
Contributor

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 existing SendHeadReq() and SendRangesReq() methods. SigV4 signing is handled automatically by the credentials already configured on the curl handle.

This PR:

  • Adds SendPutReq(const unsigned char *data, std::size_t length) to RCurlConnection
  • Uses CURLOPT_UPLOAD with a read callback (CURLOPT_READFUNCTION / CURLOPT_READDATA) and CURLOPT_INFILESIZE_LARGE for Content-Length
  • Adds a wire-level test using the TServerSocket pattern from the existing test suite, verifying the PUT method, Content-Length header, and request body
  • Adds an integration test gated on S3 credentials (GTEST_SKIP() when S3_ACCESS_KEY / S3_SECRET_KEY are not set) that does a PUT, HEAD, and GET round-trip against a real S3 endpoint

No connection pooling or retry logic is included in this PR. Those are planned for Phase 4 (Weeks 10-11) as SendMultiGetReq / SendMultiPutReq and a retry wrapper respectively.

Checklist

  • Tested changes locally
  • Wire-level test passes (TServerSocket captures correct PUT request)
  • Integration test passes against local MinIO
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 PASSED
Local test output (MinIO integration)
=== SendPutReq MinIO Integration Test ===
Endpoint: http://127.0.0.1:9000
Bucket:   test-ntuple

[Test 1] PUT small object, then HEAD to verify
  PASS: PUT small object succeeds
  PASS: HEAD after PUT succeeds
  PASS: HEAD reports correct Content-Length

[Test 2] PUT object, then GET (range read) to verify content
  PASS: PUT content-verify object succeeds
  PASS: GET after PUT succeeds
  PASS: received full payload length
  PASS: GET content matches PUT payload

[Test 3] PUT empty object (zero bytes)
  PASS: PUT empty object succeeds
  PASS: HEAD after empty PUT succeeds
  PASS: empty object has size 0

[Test 4] PUT large object (256 KB) with content verification
  PASS: PUT 256KB object succeeds
  PASS: HEAD after large PUT succeeds
  PASS: HEAD reports 256KB
  PASS: GET 256KB object succeeds
  PASS: received full 256KB
  PASS: 256KB content matches (byte-for-byte)

[Test 5] PUT overwrite (same key, different content)
  PASS: first PUT succeeds
  PASS: overwrite PUT succeeds
  PASS: HEAD reflects overwritten size
  PASS: GET returns overwritten content

[Test 6] Same connection: PUT then HEAD (sticky-option reset)
  PASS: PUT on shared conn succeeds
  PASS: HEAD on same conn after PUT succeeds (sticky reset works)
  PASS: HEAD on same conn reports correct size

=== Results ===
Passed: 23
Failed: 0

ALL TESTS PASSED
(base) jasmehta@Jass-MacBook-Air-874 test_local % mc ls local/test-ntuple/ 
[2026-05-21 14:29:20 IST]     0B STANDARD test-put-empty.bin
[2026-05-21 14:29:20 IST] 256KiB STANDARD test-put-large.bin
[2026-05-21 14:29:20 IST]    24B STANDARD test-put-overwrite.bin
[2026-05-21 14:29:20 IST]    16B STANDARD test-put-same-conn.bin
[2026-05-21 14:29:20 IST]    39B STANDARD test-put-small.bin
[2026-05-21 14:29:20 IST]    26B STANDARD test-put-verify-content.bin

@JasMehta08 JasMehta08 requested a review from dpiparo as a code owner May 21, 2026 13:06
@jblomer jblomer self-assigned this May 21, 2026
@jblomer jblomer requested review from jblomer and removed request for dpiparo May 21, 2026 14:39
@jblomer jblomer assigned JasMehta08 and unassigned jblomer May 22, 2026
Copy link
Copy Markdown
Contributor

@jblomer jblomer left a comment

Choose a reason for hiding this comment

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

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.

Comment thread net/curl/src/RCurlConnection.cxx Outdated
Comment thread net/curl/src/RCurlConnection.cxx Outdated
Comment thread net/curl/src/RCurlConnection.cxx Outdated
Comment thread net/curl/src/RCurlConnection.cxx Outdated
Comment thread net/curl/src/RCurlConnection.cxx
Comment thread net/curl/src/RCurlConnection.cxx Outdated
Comment thread net/curl/src/RCurlConnection.cxx Outdated
Comment thread net/curl/test/curl_connection.cxx Outdated
Comment thread net/curl/test/curl_connection.cxx
Comment thread net/curl/test/curl_connection.cxx
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 24, 2026

Test Results

    20 files      20 suites   3d 4h 48m 6s ⏱️
 3 840 tests  3 787 ✅ 0 💤 53 ❌
68 707 runs  68 653 ✅ 1 💤 53 ❌

For more details on these failures, see this check.

Results for commit b685af8.

♻️ This comment has been updated with latest results.

@JasMehta08 JasMehta08 force-pushed the curl-send-put-req branch from a0424d6 to dbd63f4 Compare May 25, 2026 06:31
@JasMehta08
Copy link
Copy Markdown
Contributor Author

@jblomer

I went through the comments, I have updated the code and added new test to curl_env.cxx,

I had a few question,

  • Should we also reset CURLOPT_HTTPHEADER inside ResetHandle()? Currently only SendPutReq sets and clears it, but adding it to ResetHandle() would be safer if future methods add custom headers.
  • The threshold, Curl's own internal threshold is 1024 bytes. should we match curl's default of 1 KB?

Comment thread net/curl/src/RCurlConnection.cxx
Comment thread net/curl/src/RCurlConnection.cxx
Comment thread net/curl/src/RCurlConnection.cxx Outdated
Comment thread net/curl/src/RCurlConnection.cxx Outdated
Comment thread net/curl/src/RCurlConnection.cxx Outdated
Comment thread net/curl/test/curl_connection.cxx
@jblomer
Copy link
Copy Markdown
Contributor

jblomer commented May 26, 2026

I went through the comments, I have updated the code and added new test to curl_env.cxx,

Thanks!

  • Should we also reset CURLOPT_HTTPHEADER inside ResetHandle()? Currently only SendPutReq sets and clears it, but adding it to ResetHandle() would be safer if future methods add custom headers.
  • The threshold, Curl's own internal threshold is 1024 bytes. should we match curl's default of 1 KB?

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.

Copy link
Copy Markdown
Contributor

@jblomer jblomer left a comment

Choose a reason for hiding this comment

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

As for the commits: I think it makes sense to add the ResetHandle() first as a separate commit.

@JasMehta08
Copy link
Copy Markdown
Contributor Author

@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!

@jblomer
Copy link
Copy Markdown
Contributor

jblomer commented May 26, 2026

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.

Looks good. Without additional data, let's trust that libcurl does the sensible thing.

Comment thread net/curl/src/RCurlConnection.cxx
Comment thread net/curl/src/RCurlConnection.cxx
Comment thread net/curl/src/RCurlConnection.cxx Outdated
@JasMehta08
Copy link
Copy Markdown
Contributor Author

@jblomer and @silverweed ,

I have made the changes, I have split the PR into 3 commits,

  • In the first one, converted ResetHandle() into a private method, to centralize the cleanup. It is called at top of each method: SendHeadReq, SendRangesReq, and SendPutReq. Also added the documentation for it.

  • In the second one, Created SetCurlOption helper that wraps curl_easy_setopt+ R__ASSERT, replacing all the repetitive two-line pairs across the file. Also added documentation for it.

  • In the last one,

    • Replaced CURL_READFUNC_ABORT return path with R__ASSERT(requested < CURL_READFUNC_ABORT) and R__ASSERT(requested <= remaining). Changed ToLower to return by value.
    • Added doc comments on RUploadState, CallbackPutRead, CallbackPutSeek, and ResetHandle.
    • Added PutLargeExpect100 test (2 MB payload) to exercise the Expect: 100-continue handshake.
    • Added inline comment explaining that curl only uses SEEK_SET.

I had a doubt in the second commit, I kept R__ASSERT rather than RResult for the SetCurlOption helper, since these calls use valid options and values by construction and can only fail due to programming bugs, not runtime conditions. The two curl_easy_setopt calls in SetCredentials that can genuinely fail at runtime still use throw RException. This is would be preferred right? Happy to switch to RResult if that is preferred.

Thanks!

@JasMehta08 JasMehta08 force-pushed the curl-send-put-req branch from dfdeee8 to fd0ab12 Compare May 27, 2026 17:24
@JasMehta08
Copy link
Copy Markdown
Contributor Author

@jblomer and @silverweed,

The CI caught that R__ASSERT(requested <= remaining)fails,
curl can request more bytes than remain in the upload buffer even when CURLOPT_INFILESIZE_LARGE is set. Curl's internal buffer size is independent of the upload size, so the callback needs to handle returning fewer bytes than requested. Switching to std::min(requested, remaining) might be the solution.

Comment thread net/curl/src/RCurlConnection.cxx Outdated
Comment thread net/curl/src/RCurlConnection.cxx
@JasMehta08 JasMehta08 force-pushed the curl-send-put-req branch from fd0ab12 to 06ac60c Compare May 28, 2026 11:05
Copy link
Copy Markdown
Contributor

@jblomer jblomer left a comment

Choose a reason for hiding this comment

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

Very nice! LGTM.

Comment thread net/curl/src/RCurlConnection.cxx
@JasMehta08 JasMehta08 force-pushed the curl-send-put-req branch from 06ac60c to b685af8 Compare May 29, 2026 06:06
Sign up for free to 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.

3 participants