Skip to content

Commit 4c6b983

Browse files
committed
[BUG] Wait in slices, and keep the null handle out of curl_multi_info_read
Two corrections to the change before it, both found reviewing it rather than reported. The wait cannot be interrupted. wakeupBackgroundThread reaches the worker through the multi handle, and the whole point of that branch is that there is not one, so the destructor cannot cut the wait short and the previous commit saying teardown was unchanged is wrong. Destroying a client whose worker was inside the wait measured 168 ms, five runs out of five. Taking the same wait in 16 ms slices and rechecking is_shutdown_ leaves the retry rate where it was and brings that to 10 ms, five runs out of five. And curl_multi_info_read was still called with the handle libcurl had refused to give. curl_multi_poll and curl_multi_wait cannot see one, since they sit inside the branch that only runs when perform succeeded. curl_multi_add_handle and curl_multi_remove_handle can, but they are in doAddSessions and doRemoveSessions, which open-telemetry#4395 and open-telemetry#4405 rewrite, so they are named in the description rather than changed here. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
1 parent d40daf7 commit 4c6b983

1 file changed

Lines changed: 14 additions & 2 deletions

File tree

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

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -521,7 +521,17 @@ bool HttpClient::MaybeSpawnBackgroundThread()
521521
"curl_multi_init succeeds");
522522
missing_multi_handle_reported = true;
523523
}
524-
std::this_thread::sleep_for(self->scheduled_delay_milliseconds_);
524+
// In slices, because this thread cannot be woken: wakeupBackgroundThread reaches it
525+
// through the multi handle, and there is not one. A whole delay here would be a
526+
// whole delay added to destroying the client.
527+
constexpr std::chrono::milliseconds kMissingHandleWaitSlice{16};
528+
for (std::chrono::milliseconds waited = std::chrono::milliseconds::zero();
529+
waited < self->scheduled_delay_milliseconds_ &&
530+
!self->is_shutdown_.load(std::memory_order_acquire);
531+
waited += kMissingHandleWaitSlice)
532+
{
533+
std::this_thread::sleep_for(kMissingHandleWaitSlice);
534+
}
525535
}
526536
}
527537
else if (still_running || need_wait_more)
@@ -556,7 +566,9 @@ bool HttpClient::MaybeSpawnBackgroundThread()
556566

557567
do
558568
{
559-
msg = curl_multi_info_read(self->multi_handle_, &queued);
569+
msg = (nullptr == self->multi_handle_)
570+
? nullptr
571+
: curl_multi_info_read(self->multi_handle_, &queued);
560572
if (msg == nullptr)
561573
{
562574
break;

0 commit comments

Comments
 (0)