Skip to content

[BUG] An invalid URL creates an unregistered curl session whose operation never completes #4393

Description

@thc1006

Describe your environment main at f6e4818, 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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    needs-triageIndicates an issue or PR lacks a `triage/foo` label and requires one.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions