open-telemetry / open-telemetry/opentelemetry-cpp
[BUG] An invalid URL creates an unregistered curl session whose operation never completes
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 1.4k
- Forks
- 632
- Avg merge
- 1d 13h
- Merged PRs (30d)
- 75
Description
Describe your environment main at f6e48180, Ubuntu 24.04, gcc 14, libcurl 8.14.1, CMake. Nothing platform specific.
HttpClient::CreateSession() gives back a session that the client does not know about when the URL fails to parse:
std::shared_ptr<...Session> HttpClient::CreateSession(nostd::string_view url) noexcept
{
const auto parsedUrl = common::UrlParser(std::string(url));
if (!parsedUrl.success_)
{
return std::make_shared<Session>(*this); // :330
}
auto session = std::make_shared<Session>(*this, parsedUrl.scheme_, ...);
auto session_id = ++next_session_id_; // :334
session->SetId(session_id);
std::lock_guard<std::mutex> lock_guard{sessions_m_};
sessions_.insert({session_id, session}); // :338
return session;
}
The early return skips both the id and the registration, so the caller holds a session whose session_id_ is still its default 0 (http_client_curl.h:233) and which is in nobody's map. It is otherwise a normal session: CreateRequest() and SendRequest() both work on it.
That would be harmless if the request then failed. It does not. CURLOPT_URL is not parsed when it is set, so Setup() returns CURLE_OK and SendAsync goes on to create the promise and call ScheduleAddSession(0). doAddSessions looks 0 up in sessions_, misses, and continues. Nothing ever runs the operation and nothing ever completes its future.
Steps to reproduce Against unmodified main, using TerminalCountingHandler, which is already in the test file:
TEST_F(BasicCurlHttpTests, InvalidUrl)
{
auto manager = std::make_shared<http_client::curl::HttpCurlClientFactory>()->Create();
auto session = manager->CreateSession("http://127.0.0.1:not-a-port");
auto request = session->CreateRequest();
request->SetUri("get/");
auto handler = std::make_shared<TerminalCountingHandler>();
session->SendRequest(handler);
session->FinishSession();
manager->FinishAllSessions();
}
SendRequest returns and FinishSession never does. Under a 60 second timeout the binary exits 124 having finished zero cases. A core taken at 12 seconds:
#5 std::__future_base::_State_baseV2::wait (...) at /usr/include/c++/14/future:360
#7 HttpOperation::Finish (...) at ext/src/http/client/curl/http_operation_curl.cc:516
#8 Session::FinishSession (...) at ext/src/http/client/curl/http_client_curl.cc:259
I only have that one URL measured, since the loop I wrote never got past it.
What is the expected behavior? A URL the client cannot parse ends the request with a terminal event, ideally CreateFailed carrying the reason, and FinishSession() returns.
What is the actual behavior? FinishSession() blocks forever. For an exporter this means an application that gets its endpoint wrong hangs at shutdown rather than logging a bad endpoint, and the misconfiguration is usually a string from the environment.
Additional context This is the same root cause as #4390, reached a different way: an operation can be given a promise without ever being scheduled, and then nobody is left to complete it. #4390 needs a handler that cancels from an early event; this one needs a typo. One fix covers both, and it does not have to be large: if ScheduleAddSession reports that the session is not registered, SendAsync can complete the operation itself instead of leaving the future open.
Whether an unparseable URL should also get a distinct CreateFailed with the reason, rather than the generic terminal event that fix would produce, is a separate and better question, and it is yours rather than mine. I am happy to write either shape.
Found while working on #4375 and #4390.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with HttpClient::CreateSession in ext/src/http/client/curl/http_client_curl.h, then trace SendAsync, ScheduleAddSession, and doAddSessions to see how the unregistered session is handled. Reproduce the InvalidUrl case with TerminalCountingHandler and verify that the request reaches a terminal event and FinishSession returns instead of blocking indefinitely.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- networking
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100