Skip to content

Commit 26b7744

Browse files
committed
[BUG] Stop the IO thread spinning when the multi handle cannot be created
Raised by @lalitb in review, who asked whether the case would hang rather than fail if the recovery were broken. It would, and following that down found the reason, which is not in the case. curl_multi_perform does not write still_running when it rejects the handle, and the loop initialises it to one. So a client whose multi handle is null keeps reporting work it does not have: the loop takes the still_running > 0 branch every pass, never reaches the shutdown check below it, and never waits, so the thread spins a core and ~HttpClient blocks in join for good. Gating curl_multi_init to fail persistently, the whole test binary hangs, twice out of twice at sixty seconds and once at two minutes. With still_running cleared on that branch it finishes in 521 ms, three runs out of three, and the case passes rather than hanging, because the reset then cancels the session and the caller is told. A multi handle that fails once and then works is unaffected: the reset creates one, doAddSessions puts the pending session on it and sets still_running again, and the request completes with a response, three runs out of three with the internal log confirming the first handle really was null. The case also cancels before it finishes, so an assertion failure ends the case rather than leaving FinishSession waiting on an operation nothing will complete. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
1 parent 85e9a36 commit 26b7744

3 files changed

Lines changed: 11 additions & 0 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,9 @@ Increment the:
4747
* [BUG] Check what curl_slist_append and curl_multi_init return instead of
4848
treating a failed allocation as success
4949
[#4404](https://github.com/open-telemetry/opentelemetry-cpp/issues/4404)
50+
* [BUG] Stop the curl IO thread spinning, and the client refusing to be
51+
destroyed, when the multi handle cannot be created
52+
[#4404](https://github.com/open-telemetry/opentelemetry-cpp/issues/4404)
5053

5154
* [CONFIGURATION] Add SDK component builder interfaces to the registry
5255
[#4358](https://github.com/open-telemetry/opentelemetry-cpp/issues/4358)

‎ext/src/http/client/curl/http_client_curl.cc‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -482,6 +482,10 @@ bool HttpClient::MaybeSpawnBackgroundThread()
482482
// can not curl_multi_perform it again
483483
if (mc != CURLM_OK)
484484
{
485+
// curl_multi_perform leaves still_running alone when it rejects the handle, and it
486+
// starts at one, so without this the loop keeps reporting work it does not have,
487+
// never reaches the shutdown check below, and the thread cannot be joined.
488+
still_running = 0;
485489
self->resetMultiHandle();
486490
}
487491
else if (still_running || need_wait_more)

‎ext/test/http/curl_http_test.cc‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1109,6 +1109,10 @@ TEST_F(BasicCurlHttpTests, AClientWithoutAMultiHandleStillAnswers)
11091109
<< "the request was accepted and never reached an outcome";
11101110
EXPECT_FALSE(session->IsSessionActive()) << "the session stayed active with nothing running it";
11111111

1112+
// Cancel before finishing. If either assertion above failed then nothing is going to complete
1113+
// this operation, and FinishSession would wait for it for good, so the case would hang rather
1114+
// than fail. Cancelling a session that already answered does nothing.
1115+
session->CancelSession();
11121116
session->FinishSession();
11131117
client->FinishAllSessions();
11141118
}

0 commit comments

Comments
 (0)