Skip to content

[BUG] Check what curl_global_init returns (#4434) - #4531

Open
Dyfintie wants to merge 1 commit into
open-telemetry:mainfrom
Dyfintie:fix-curl-global-init-4434
Open

[BUG] Check what curl_global_init returns (#4434)#4531
Dyfintie wants to merge 1 commit into
open-telemetry:mainfrom
Dyfintie:fix-curl-global-init-4434

Conversation

@Dyfintie

@Dyfintie Dyfintie commented Sep 7, 2026

Copy link
Copy Markdown

Fixes #4434.

## Changes
- [x] Bug fix (non-breaking change which fixes an issue)

This PR addresses the unhandled return value of `curl_global_init(CURL_GLOBAL_ALL)` in `HttpCurlGlobalInitializer`:
- Stores the return `CURLcode` and tracks an `is_initialized_` boolean status.
- Logs an error with `OTEL_INTERNAL_LOG_ERROR` if `curl_global_init` fails.
- Skips calling `curl_global_cleanup()` in the destructor if initialization failed.
- Avoids creating `multi_handle_` in `HttpClient` if global init failed.
- Immediately signals `SessionState::CreateFailed` on `HttpClient::SendRequest()`, `HttpClientSync::Get()`, and `HttpClientSync::Post()`.
- Adds unit tests in `curl_http_test.cc` via `HttpClientTestPeer` covering both asynchronous and synchronous client behaviors on

initialization failure.

## Testing
- Added unit tests:
  - `BasicCurlHttpTests.GlobalInitFailureHttpClient`
  - `BasicCurlHttpTests.GlobalInitFailureHttpClientSync`
- Ran full test suite locally: all 28 tests in `curl_http_test` passed.
- Formatted with `clang-format`.

For significant contributions please make sure you have completed the following items:

* [ ] `CHANGELOG.md` updated for non-trivial changes
* [x] Unit tests have been added
* [x] Changes in public API reviewed

@Dyfintie
Dyfintie requested a review from a team as a code owner September 7, 2026 17:41
@thc1006

thc1006 commented Sep 8, 2026

Copy link
Copy Markdown
Member

Thanks for this, and for asking on the issue first. Two things I checked that I think are right, and one question.

The guard in Session::SendRequest is in the right place, and it is worth saying why, because it is easy to lose in a later edit. It is the first statement in the function, so it returns before http_client_.MaybeSpawnBackgroundThread(). That matters: the background loop calls curl_multi_perform, curl_multi_poll, curl_multi_wait and curl_multi_info_read on multi_handle_ with no null check, so the thread never starting is what keeps those safe rather than any check inside them.

Moving the curl_global_initializer_ declaration up in the header is also right, and necessary rather than cosmetic. Members initialise in declaration order and not in the order of the initialiser list, so without the header change the new multi_handle_ initialiser would read a member that had not been constructed yet.

The question. On main there are four more uses of multi_handle_ that are not null checked: curl_multi_add_handle around line 752, curl_multi_remove_handle around 819 and 863, and the curl_multi_cleanup and curl_multi_init pair around 913. Reading the code I could not reach any of them from a failed global init, because each sits behind either the background thread or a session that was already scheduled, and this PR stops both. curl_multi_wakeup is already guarded on main. Since this PR is what makes a null multi_handle_ possible in the first place, it seems worth confirming that rather than inferring it, and a case that constructs the client with a failed initializer and then calls CancelAllSessions() and FinishAllSessions() would cover the destructor path.

One overlap to be aware of rather than to act on. #4406 makes the client survive without a multi handle from the other direction, where curl_multi_init() itself fails, and it measured what the current code does in that state: 3.00 seconds of CPU in a 3 second window and 1,168,126 error lines from the IO thread. Same end state, different trigger. It is a draft and parked, so please do not wait for it. I will rebase onto whichever of us lands first.

Small thing: "a private constructor to to simulate" in the header has a doubled "to".

Ensure HttpCurlGlobalInitializer checks the return value of
  curl_global_init. If initialization fails, log an error, avoid calling curl_global_cleanup, and cause HttpClient and HttpClientSync to report
  SessionState::CreateFailed.

    Fixes open-telemetry#4434

Signed-off-by: Varun <v08pandey@gmail.com>
@Dyfintie
Dyfintie force-pushed the fix-curl-global-init-4434 branch from d2805e3 to 2c5efd2 Compare September 8, 2026 21:50
@Dyfintie

Dyfintie commented Sep 8, 2026

Copy link
Copy Markdown
Author

Thank you so much for the thorough review and insights :)
I have updated the PR with the following changes:

  1. Guard rationale comment: Added a comment in
    Session::SendRequest documenting that returning early before
    MaybeSpawnBackgroundThread() prevents the background IO loop
    from starting with an uninitialized or null multi_handle_.

  2. Explicit test coverage: Extended BasicCurlHttpTests.
    GlobalInitFailureHttpClient to explicitly call client.
    CancelAllSessions() and client.FinishAllSessions(), ensuring the
    cancellation and destructor paths execute cleanly when
    multi_handle_ is null.

  3. Comment typo fix: Fixed the doubled "to" in the header
    comment.:)

  4. Also fixed implemented the linter fixes.

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.

[BUG] The curl client does not check what curl_global_init returns

2 participants