Skip to content

Avoid redundant curl connections during telemetry HTTP sends - #1541

Open
bmehta001 wants to merge 4 commits into
microsoft:mainfrom
bmehta001:fix/curl-single-transfer-msft
Open

bmehta001 wants to merge 4 commits into
microsoft:mainfrom
bmehta001:fix/curl-single-transfer-msft

Conversation

@bmehta001

@bmehta001 bmehta001 commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Why

A single telemetry HTTP send currently performs CURLOPT_CONNECT_ONLY and then the actual HTTP transfer. libcurl documents that connect-only connections cannot be reused; a loopback regression against curl 8.22.0 observed two TCP accepts for one GET before this change. The readiness poll therefore checks a different connection from the one carrying the request.

What changed

  • With curl 7.80+, use CURLOPT_PREREQFUNCTION to dispatch OnSending after connection establishment and before the HTTP request, within one transfer. Preserve connect/send failure events and abort on cancellation or callback exceptions without unwinding through libcurl.
  • Choose the connect-only fallback using the loaded libcurl version, even when built with newer headers; preserve its state-event timing on runtimes older than 7.80. The one-connection behavior applies only to 7.80+ runtimes.
  • Cover one connection for GET and binary POST, state-event order, cancellation from OnSending, and exceptions raised by an OnSending observer.

OnSending is now delivered from a libcurl callback on curl 7.80+, rather than between two transfers. On curl 7.80+, OnSending runs inside libcurl and receives a null handle to prevent in-transfer option changes; configure curl on the OnConnecting event instead. Older curl retains the legacy event timing and handle.

Additional curl safety fixes

  • Propagate curl_global_init failures as creation/local failures without calling other curl APIs after failed initialization; keep the handle null for safe teardown.
  • Convert response-vector allocation exceptions to a short write (CURLE_WRITE_ERROR) rather than unwinding through libcurl's C callback, when C++ exceptions are enabled. No-exceptions builds also compile.
  • Wait for the loopback server to record an accepted connection when an OnSending observer aborts the transfer immediately.

Validation

  • curl 8.22.0 ASan/UBSan Linux: 18 focused tests x 10 iterations (180 executions), with leak checking and no skips or sanitizer reports.
  • Linux: curl transport compiles with -fno-exceptions against curl 8.22.0 headers.
  • Windows (Visual Studio 2026): 59 targeted HTTP tests passed.
  • Linux (system curl 8.5.0): 32 focused curl tests passed; 2 GET/POST tests also passed with a temporary runtime-version shim reporting 7.79 to exercise the legacy path.
  • git diff --check passed.

This fixes the demonstrated extra connection, not a proven root cause of the reported Curl_wildcard_dtor SIGSEGV. The crash report has no core, faulting instruction, registers, or loaded-module map to establish that cause.

lib/http/HttpClient_Curl.hpp: Use a pre-request hook to preserve event order while issuing one transfer on curl 7.80+.

tests/unittests/HttpClientCurlTests.cpp: Cover connection counts, cancellation, state order, and callback failures.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 695c3a98-72f2-4247-afb3-7d4b0bbc34a5
@bmehta001
bmehta001 requested a review from a team as a code owner September 27, 2026 03:55
lib/http/HttpClient_Curl.hpp: Propagate global-init failures, initialize the handle safely, and abort vector writes rather than unwinding through libcurl.

lib/http/HttpClient_Curl.cpp: Report initialization failures without calling curl_version_info on a failed library.

tests/unittests/HttpClientCurlTests.cpp: Verify initialization and synchronize the abort-before-accept regression.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 695c3a98-72f2-4247-afb3-7d4b0bbc34a5

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Runtime-version fallback and callback reentrancy/state-event issues remain unresolved.

Review effort: Balanced
Findings: 1 High severity

Open (1)
What changed in this PR

Updates the curl transport to avoid redundant connections while preserving telemetry state events and improving failure safety.

Changes:

  • Uses CURLOPT_PREREQFUNCTION for modern libcurl.
  • Handles global initialization and callback allocation failures.
  • Adds connection, cancellation, binary POST, and exception tests.
File Description
lib/​http/​HttpClient_Curl.hpp Implements connection-ready callbacks and safety handling.
lib/​http/​HttpClient_Curl.cpp Propagates global initialization failures.
tests/​unittests/​HttpClientCurlTests.cpp Adds transport regression coverage.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/http/HttpClient_Curl.hpp Outdated
Copilot comment 4114465995: runtime libcurl may be older than build headers. Probe the loaded library after global init and install CURLOPT_PREREQFUNCTION only on 7.80+; route Send() through the existing connect-only path otherwise. Verified at CMakeLists.txt:219-242 and lib/http/HttpClient_Curl.hpp:293-337.

Files changed: lib/http/HttpClient_Curl.hpp, tests/unittests/HttpClientCurlTests.cpp. Check connection counts against the runtime transport path, including GET and binary POST.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 5fa346e8-a5c8-4324-8144-a4a867cb146e

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Existing listeners can now call curl_easy_setopt reentrantly on an active handle from OnSending, causing unsupported behavior.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (1)

Comment thread lib/http/HttpClient_Curl.hpp
Copilot comment 4114543078: sample listeners in examples/cpp/SampleCpp/DebugCallback.cpp:79-86 and examples/cpp/MacProxy/HttpEventListener.cpp:38-45 set options on non-null handles. OnSending on curl 7.80+ now passes null while OnConnecting retains its pre-transfer handle; legacy timing and handle access stay unchanged.

Files changed: lib/http/HttpClient_Curl.hpp, lib/include/public/IHttpClient.hpp, tests/unittests/HttpClientCurlTests.cpp. Document callback handle availability and assert both runtime paths in loopback tests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 5fa346e8-a5c8-4324-8144-a4a867cb146e

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The implementation preserves older-runtime behavior and adds focused coverage for connection reuse, callback ordering, cancellation, and failure paths.

Review effort: Balanced
Findings: None

Resolved since last review (1)

This branch has not been deployed

No deployments
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.

2 participants