From a5b68170260d0cf906142f3cbbd0566efa3d5c92 Mon Sep 17 00:00:00 2001 From: toraxie Date: Tue, 10 Dec 2024 19:58:36 +0800 Subject: [PATCH 01/13] update --- .../ext/http/client/curl/http_client_curl.h | 18 ++---- ext/src/http/client/curl/http_client_curl.cc | 62 ++++++++++++++++--- 2 files changed, 61 insertions(+), 19 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h index 97386890f2..e0898d3e47 100644 --- a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h +++ b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h @@ -328,19 +328,9 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient void ScheduleAbortSession(uint64_t session_id); void ScheduleRemoveSession(uint64_t session_id, HttpCurlEasyResource &&resource); - void WaitBackgroundThreadExit() - { - std::unique_ptr background_thread; - { - std::lock_guard lock_guard{background_thread_m_}; - background_thread.swap(background_thread_); - } + void SetBackgroundWaitFor(std::chrono::milliseconds ms); - if (background_thread && background_thread->joinable()) - { - background_thread->join(); - } - } + void WaitBackgroundThreadExit(); private: void wakeupBackgroundThread(); @@ -366,6 +356,10 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient std::unique_ptr background_thread_; std::chrono::milliseconds scheduled_delay_milliseconds_; + std::condition_variable background_thread_waiter_cv_; + std::mutex background_thread_waiter_lock_; + std::chrono::milliseconds background_thread_wait_for_; + nostd::shared_ptr curl_global_initializer_; }; diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 6827b9f9c7..e8d2b417cf 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -189,6 +189,7 @@ HttpClient::HttpClient() next_session_id_{0}, max_sessions_per_connection_{8}, scheduled_delay_milliseconds_{std::chrono::milliseconds(256)}, + background_thread_wait_for_{std::chrono::minutes{1}}, curl_global_initializer_(HttpCurlGlobalInitializer::GetInstance()) {} @@ -196,6 +197,7 @@ HttpClient::~HttpClient() { while (true) { + background_thread_wait_for_ = std::chrono::milliseconds{0}; std::unique_ptr background_thread; { std::lock_guard lock_guard{background_thread_m_}; @@ -211,6 +213,7 @@ HttpClient::~HttpClient() } if (background_thread->joinable()) { + background_thread_waiter_cv_.notify_all(); background_thread->join(); } } @@ -236,6 +239,7 @@ std::shared_ptr HttpClient::CreateSes std::lock_guard lock_guard{sessions_m_}; sessions_.insert({session_id, session}); + background_thread_waiter_cv_.notify_all(); // FIXME: Session may leak if it do not call SendRequest return session; } @@ -416,6 +420,23 @@ void HttpClient::MaybeSpawnBackgroundThread() still_running = 1; } + if (still_running > 0) + { + continue; + } + + // If there is no pending jobs, Exit flush thread if there is not data to flush more + // than one minute. + if (self->background_thread_wait_for_ != std::chrono::milliseconds{0}) + { + std::unique_lock lk{self->background_thread_waiter_lock_}; + if (self->background_thread_waiter_cv_.wait_for( + lk, self->background_thread_wait_for_) != std::cv_status::timeout) + { + continue; + } + } + if (still_running == 0) { std::lock_guard lock_guard{self->background_thread_m_}; @@ -440,16 +461,18 @@ void HttpClient::MaybeSpawnBackgroundThread() still_running = 1; } + if (still_running > 0) + { + continue; + } + // If there is no pending jobs, we can stop the background thread. - if (still_running == 0) + if (self->background_thread_) { - if (self->background_thread_) - { - self->background_thread_->detach(); - self->background_thread_.reset(); - } - break; + self->background_thread_->detach(); + self->background_thread_.reset(); } + break; } } }, @@ -502,6 +525,30 @@ void HttpClient::ScheduleRemoveSession(uint64_t session_id, HttpCurlEasyResource wakeupBackgroundThread(); } +void HttpClient::SetBackgroundWaitFor(std::chrono::milliseconds ms) +{ + std::lock_guard lock_guard{background_thread_m_}; + background_thread_wait_for_ = ms; +} + +void HttpClient::WaitBackgroundThreadExit() +{ + std::unique_ptr background_thread; + { + std::lock_guard lock_guard{background_thread_m_}; + background_thread.swap(background_thread_); + } + + if (background_thread && background_thread->joinable()) + { + auto wait_for = background_thread_wait_for_; + background_thread_wait_for_ = std::chrono::milliseconds{0}; + background_thread_waiter_cv_.notify_all(); + background_thread->join(); + background_thread_wait_for_ = wait_for; + } +} + void HttpClient::wakeupBackgroundThread() { // Before libcurl 7.68.0, we can only wait for timeout and do the rest jobs @@ -513,6 +560,7 @@ void HttpClient::wakeupBackgroundThread() curl_multi_wakeup(multi_handle_); } #endif + background_thread_waiter_cv_.notify_all(); } bool HttpClient::doAddSessions() From 6260e42906f8db65571deb5a29ae782477f95f29 Mon Sep 17 00:00:00 2001 From: toraxie Date: Tue, 10 Dec 2024 22:49:47 +0800 Subject: [PATCH 02/13] update --- .../ext/http/client/curl/http_client_curl.h | 3 +- ext/src/http/client/curl/http_client_curl.cc | 58 +++++++++---------- 2 files changed, 27 insertions(+), 34 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h index e0898d3e47..af41672647 100644 --- a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h +++ b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h @@ -356,9 +356,8 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient std::unique_ptr background_thread_; std::chrono::milliseconds scheduled_delay_milliseconds_; - std::condition_variable background_thread_waiter_cv_; - std::mutex background_thread_waiter_lock_; std::chrono::milliseconds background_thread_wait_for_; + std::atomic is_shutdown; nostd::shared_ptr curl_global_initializer_; }; diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index e8d2b417cf..01e0578e38 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -191,13 +191,15 @@ HttpClient::HttpClient() scheduled_delay_milliseconds_{std::chrono::milliseconds(256)}, background_thread_wait_for_{std::chrono::minutes{1}}, curl_global_initializer_(HttpCurlGlobalInitializer::GetInstance()) -{} +{ + is_shutdown.store(false); +} HttpClient::~HttpClient() { + is_shutdown.store(true, std::memory_order_release); while (true) { - background_thread_wait_for_ = std::chrono::milliseconds{0}; std::unique_ptr background_thread; { std::lock_guard lock_guard{background_thread_m_}; @@ -213,7 +215,6 @@ HttpClient::~HttpClient() } if (background_thread->joinable()) { - background_thread_waiter_cv_.notify_all(); background_thread->join(); } } @@ -239,7 +240,6 @@ std::shared_ptr HttpClient::CreateSes std::lock_guard lock_guard{sessions_m_}; sessions_.insert({session_id, session}); - background_thread_waiter_cv_.notify_all(); // FIXME: Session may leak if it do not call SendRequest return session; } @@ -350,18 +350,28 @@ void HttpClient::MaybeSpawnBackgroundThread() background_thread_.reset(new std::thread( [](HttpClient *self) { int still_running = 1; + std::chrono::system_clock::time_point last_free_job_timepoint = + std::chrono::system_clock::now(); while (true) { CURLMsg *msg; int queued; CURLMcode mc = curl_multi_perform(self->multi_handle_, &still_running); + + std::chrono::system_clock::time_point now = std::chrono::system_clock::now(); + + auto wait_for = self->background_thread_wait_for_; + if (self->is_shutdown.load(std::memory_order_acquire)) + { + wait_for = std::chrono::milliseconds{0}; + } // According to https://curl.se/libcurl/c/curl_multi_perform.html, when mc is not OK, we // can not curl_multi_perform it again if (mc != CURLM_OK) { self->resetMultiHandle(); } - else if (still_running) + else if (still_running || now - last_free_job_timepoint < wait_for) { // curl_multi_poll is added from libcurl 7.66.0, before 7.68.0, we can only wait util // timeout to do the rest jobs @@ -422,22 +432,11 @@ void HttpClient::MaybeSpawnBackgroundThread() if (still_running > 0) { + last_free_job_timepoint = now; continue; } - // If there is no pending jobs, Exit flush thread if there is not data to flush more - // than one minute. - if (self->background_thread_wait_for_ != std::chrono::milliseconds{0}) - { - std::unique_lock lk{self->background_thread_waiter_lock_}; - if (self->background_thread_waiter_cv_.wait_for( - lk, self->background_thread_wait_for_) != std::cv_status::timeout) - { - continue; - } - } - - if (still_running == 0) + if (still_running == 0 && now - last_free_job_timepoint > wait_for) { std::lock_guard lock_guard{self->background_thread_m_}; // Double check, make sure no more pending sessions after locking background thread @@ -461,18 +460,16 @@ void HttpClient::MaybeSpawnBackgroundThread() still_running = 1; } - if (still_running > 0) - { - continue; - } - // If there is no pending jobs, we can stop the background thread. - if (self->background_thread_) + if (still_running == 0) { - self->background_thread_->detach(); - self->background_thread_.reset(); + if (self->background_thread_) + { + self->background_thread_->detach(); + self->background_thread_.reset(); + } + break; } - break; } } }, @@ -533,6 +530,7 @@ void HttpClient::SetBackgroundWaitFor(std::chrono::milliseconds ms) void HttpClient::WaitBackgroundThreadExit() { + is_shutdown.store(true, std::memory_order_release); std::unique_ptr background_thread; { std::lock_guard lock_guard{background_thread_m_}; @@ -541,12 +539,9 @@ void HttpClient::WaitBackgroundThreadExit() if (background_thread && background_thread->joinable()) { - auto wait_for = background_thread_wait_for_; - background_thread_wait_for_ = std::chrono::milliseconds{0}; - background_thread_waiter_cv_.notify_all(); background_thread->join(); - background_thread_wait_for_ = wait_for; } + is_shutdown.store(false, std::memory_order_release); } void HttpClient::wakeupBackgroundThread() @@ -560,7 +555,6 @@ void HttpClient::wakeupBackgroundThread() curl_multi_wakeup(multi_handle_); } #endif - background_thread_waiter_cv_.notify_all(); } bool HttpClient::doAddSessions() From 37fab9b8b50990255a20dbc0a93c22baf8e147e6 Mon Sep 17 00:00:00 2001 From: toraxie Date: Tue, 10 Dec 2024 22:51:21 +0800 Subject: [PATCH 03/13] update --- ext/src/http/client/curl/http_client_curl.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 01e0578e38..1a57204003 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -524,7 +524,6 @@ void HttpClient::ScheduleRemoveSession(uint64_t session_id, HttpCurlEasyResource void HttpClient::SetBackgroundWaitFor(std::chrono::milliseconds ms) { - std::lock_guard lock_guard{background_thread_m_}; background_thread_wait_for_ = ms; } From a9bc7df19980f649314bc79d318a8c4662c54572 Mon Sep 17 00:00:00 2001 From: toraxie Date: Wed, 11 Dec 2024 19:31:09 +0800 Subject: [PATCH 04/13] update --- ext/src/http/client/curl/http_client_curl.cc | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 1a57204003..bbc732a12a 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -358,13 +358,16 @@ void HttpClient::MaybeSpawnBackgroundThread() int queued; CURLMcode mc = curl_multi_perform(self->multi_handle_, &still_running); - std::chrono::system_clock::time_point now = std::chrono::system_clock::now(); - - auto wait_for = self->background_thread_wait_for_; + std::chrono::milliseconds wait_for; +#if LIBCURL_VERSION_NUM >= 0x074200 + // only avaliable with curl_multi_poll + wait_for = self->background_thread_wait_for_; +#endif if (self->is_shutdown.load(std::memory_order_acquire)) { wait_for = std::chrono::milliseconds{0}; } + // According to https://curl.se/libcurl/c/curl_multi_perform.html, when mc is not OK, we // can not curl_multi_perform it again if (mc != CURLM_OK) From 367ce5a7440fa8be50f08bab977b138910733daa Mon Sep 17 00:00:00 2001 From: toraxie Date: Wed, 11 Dec 2024 23:55:44 +0800 Subject: [PATCH 05/13] update --- ext/src/http/client/curl/http_client_curl.cc | 33 +++++++++++++------- ext/test/http/curl_http_test.cc | 18 +++++++++++ 2 files changed, 39 insertions(+), 12 deletions(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index bbc732a12a..c493ad8b73 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -352,29 +352,20 @@ void HttpClient::MaybeSpawnBackgroundThread() int still_running = 1; std::chrono::system_clock::time_point last_free_job_timepoint = std::chrono::system_clock::now(); + bool need_wait_more = false; while (true) { CURLMsg *msg; int queued; CURLMcode mc = curl_multi_perform(self->multi_handle_, &still_running); - std::chrono::milliseconds wait_for; -#if LIBCURL_VERSION_NUM >= 0x074200 - // only avaliable with curl_multi_poll - wait_for = self->background_thread_wait_for_; -#endif - if (self->is_shutdown.load(std::memory_order_acquire)) - { - wait_for = std::chrono::milliseconds{0}; - } - // According to https://curl.se/libcurl/c/curl_multi_perform.html, when mc is not OK, we // can not curl_multi_perform it again if (mc != CURLM_OK) { self->resetMultiHandle(); } - else if (still_running || now - last_free_job_timepoint < wait_for) + else if (still_running || need_wait_more) { // curl_multi_poll is added from libcurl 7.66.0, before 7.68.0, we can only wait util // timeout to do the rest jobs @@ -433,13 +424,31 @@ void HttpClient::MaybeSpawnBackgroundThread() still_running = 1; } + std::chrono::system_clock::time_point now = std::chrono::system_clock::now(); if (still_running > 0) { last_free_job_timepoint = now; + need_wait_more = false; + continue; + } + + std::chrono::milliseconds wait_for; +#if LIBCURL_VERSION_NUM >= 0x074200 + // only avaliable with curl_multi_poll + wait_for = self->background_thread_wait_for_; +#endif + if (self->is_shutdown.load(std::memory_order_acquire)) + { + wait_for = std::chrono::milliseconds{0}; + } + + if (now - last_free_job_timepoint < wait_for) + { + need_wait_more = true; continue; } - if (still_running == 0 && now - last_free_job_timepoint > wait_for) + if (still_running == 0) { std::lock_guard lock_guard{self->background_thread_m_}; // Double check, make sure no more pending sessions after locking background thread diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 137c9b7f9b..9b5a6df403 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -526,3 +526,21 @@ TEST_F(BasicCurlHttpTests, FinishInAsyncCallback) } } } + +TEST_F(BasicCurlHttpTests, ElegantQuitQuick) +{ + auto http_client = http_client::HttpClientFactory::Create(); + std::dynamic_pointer_cast(http_client)->MaybeSpawnBackgroundThread(); + auto beg = std::chrono::system_clock::now(); + auto session = http_client->CreateSession("http://127.0.0.1:19000/get/"); + auto request = session->CreateRequest(); + request->SetUri("get/"); + auto handler = std::make_shared(); + session->SendRequest(handler); + http_client->FinishAllSessions(); + http_client.reset(); + // when use background_thread_wait_for_ should have no side effort on elegant quit + ASSERT_TRUE(std::chrono::system_clock::now() - beg < std::chrono::milliseconds{5}); + ASSERT_TRUE(handler->is_called_); + ASSERT_TRUE(handler->got_response_); +} From b448f6c87eb01f8e5283f160c62c9c3234fa346b Mon Sep 17 00:00:00 2001 From: toraxie Date: Thu, 12 Dec 2024 14:21:53 +0800 Subject: [PATCH 06/13] update --- .../ext/http/client/curl/http_client_curl.h | 2 +- ext/src/http/client/curl/http_client_curl.cc | 18 ++++++++++-------- ext/test/http/curl_http_test.cc | 9 ++++++--- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h index af41672647..8ff836f28a 100644 --- a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h +++ b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h @@ -357,7 +357,7 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient std::chrono::milliseconds scheduled_delay_milliseconds_; std::chrono::milliseconds background_thread_wait_for_; - std::atomic is_shutdown; + std::atomic is_shutdown_; nostd::shared_ptr curl_global_initializer_; }; diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index c493ad8b73..fef1f895dd 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -192,12 +192,12 @@ HttpClient::HttpClient() background_thread_wait_for_{std::chrono::minutes{1}}, curl_global_initializer_(HttpCurlGlobalInitializer::GetInstance()) { - is_shutdown.store(false); + is_shutdown_.store(false); } HttpClient::~HttpClient() { - is_shutdown.store(true, std::memory_order_release); + is_shutdown_.store(true, std::memory_order_release); while (true) { std::unique_ptr background_thread; @@ -432,14 +432,16 @@ void HttpClient::MaybeSpawnBackgroundThread() continue; } - std::chrono::milliseconds wait_for; + std::chrono::milliseconds wait_for = std::chrono::milliseconds::zero(); + ; #if LIBCURL_VERSION_NUM >= 0x074200 - // only avaliable with curl_multi_poll + // only avaliable with curl_multi_poll, because curl_multi_wait would cause CPU busy, + // curl_multi_wait+sleep could not wakeup quickly wait_for = self->background_thread_wait_for_; #endif - if (self->is_shutdown.load(std::memory_order_acquire)) + if (self->is_shutdown_.load(std::memory_order_acquire)) { - wait_for = std::chrono::milliseconds{0}; + wait_for = std::chrono::milliseconds::zero(); } if (now - last_free_job_timepoint < wait_for) @@ -541,7 +543,7 @@ void HttpClient::SetBackgroundWaitFor(std::chrono::milliseconds ms) void HttpClient::WaitBackgroundThreadExit() { - is_shutdown.store(true, std::memory_order_release); + is_shutdown_.store(true, std::memory_order_release); std::unique_ptr background_thread; { std::lock_guard lock_guard{background_thread_m_}; @@ -552,7 +554,7 @@ void HttpClient::WaitBackgroundThreadExit() { background_thread->join(); } - is_shutdown.store(false, std::memory_order_release); + is_shutdown_.store(false, std::memory_order_release); } void HttpClient::wakeupBackgroundThread() diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 9b5a6df403..c58d7852d8 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -530,8 +530,9 @@ TEST_F(BasicCurlHttpTests, FinishInAsyncCallback) TEST_F(BasicCurlHttpTests, ElegantQuitQuick) { auto http_client = http_client::HttpClientFactory::Create(); - std::dynamic_pointer_cast(http_client)->MaybeSpawnBackgroundThread(); - auto beg = std::chrono::system_clock::now(); + std::static_pointer_cast(http_client)->MaybeSpawnBackgroundThread(); + auto beg = std::chrono::system_clock::now(); + // start background first, then test it could wakeup auto session = http_client->CreateSession("http://127.0.0.1:19000/get/"); auto request = session->CreateRequest(); request->SetUri("get/"); @@ -540,7 +541,9 @@ TEST_F(BasicCurlHttpTests, ElegantQuitQuick) http_client->FinishAllSessions(); http_client.reset(); // when use background_thread_wait_for_ should have no side effort on elegant quit - ASSERT_TRUE(std::chrono::system_clock::now() - beg < std::chrono::milliseconds{5}); + // because ci machine may slow, so we assert it cost should less than + // scheduled_delay_milliseconds_ + ASSERT_TRUE(std::chrono::system_clock::now() - beg < std::chrono::milliseconds{20}); ASSERT_TRUE(handler->is_called_); ASSERT_TRUE(handler->got_response_); } From feab23b5fe69f32ce2e96799ef16f9b023993297 Mon Sep 17 00:00:00 2001 From: toraxie Date: Thu, 12 Dec 2024 14:25:23 +0800 Subject: [PATCH 07/13] update --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 22176df917..e1ee4af753 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,9 @@ Increment the: ## [Unreleased] +* [SDK] Do not frequently create and destroy http client threads + [#3198](https://github.com/open-telemetry/opentelemetry-cpp/pull/3198) + ## [1.18 2024-11-25] * [EXPORTER] Fix crash in ElasticsearchLogRecordExporter From 992f9392d8e146ecf7f727d63fbec6f8cce0f38f Mon Sep 17 00:00:00 2001 From: toraxie Date: Thu, 12 Dec 2024 14:41:12 +0800 Subject: [PATCH 08/13] update unittest --- .../ext/http/client/curl/http_client_curl.h | 3 ++- ext/src/http/client/curl/http_client_curl.cc | 5 ++-- ext/test/http/curl_http_test.cc | 25 +++++++++++++++++++ 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h index 8ff836f28a..f48adcbfc7 100644 --- a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h +++ b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h @@ -322,7 +322,8 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient inline CURLM *GetMultiHandle() noexcept { return multi_handle_; } - void MaybeSpawnBackgroundThread(); + // return true if create background thread, false is already exist background thread + bool MaybeSpawnBackgroundThread(); void ScheduleAddSession(uint64_t session_id); void ScheduleAbortSession(uint64_t session_id); diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index fef1f895dd..829344aba4 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -339,12 +339,12 @@ void HttpClient::CleanupSession(uint64_t session_id) } } -void HttpClient::MaybeSpawnBackgroundThread() +bool HttpClient::MaybeSpawnBackgroundThread() { std::lock_guard lock_guard{background_thread_m_}; if (background_thread_) { - return; + return false; } background_thread_.reset(new std::thread( @@ -488,6 +488,7 @@ void HttpClient::MaybeSpawnBackgroundThread() } }, this)); + return true; } void HttpClient::ScheduleAddSession(uint64_t session_id) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index c58d7852d8..3e843ed4fe 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -1,6 +1,7 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 +#include #include #include #include @@ -11,6 +12,7 @@ #include #include #include +#include #include #include @@ -547,3 +549,26 @@ TEST_F(BasicCurlHttpTests, ElegantQuitQuick) ASSERT_TRUE(handler->is_called_); ASSERT_TRUE(handler->got_response_); } + +TEST_F(BasicCurlHttpTests, BackgroundThreadWaitMore) +{ + { + curl::HttpClient http_client; + http_client.MaybeSpawnBackgroundThread(); + std::this_thread::sleep_for(std::chrono::milliseconds{10}); +#if LIBCURL_VERSION_NUM >= 0x074200 + ASSERT_FALSE(http_client.MaybeSpawnBackgroundThread()); +#else + // low version curl do not support delay quit, so old background would quit + ASSERT_TRUE(http_client.MaybeSpawnBackgroundThread()); +#endif + } + { + curl::HttpClient http_client; + http_client.SetBackgroundWaitFor(std::chrono::milliseconds::zero()); + http_client.MaybeSpawnBackgroundThread(); + std::this_thread::sleep_for(std::chrono::milliseconds{10}); + // we can disable delay quit by set wait for 0 + ASSERT_TRUE(http_client.MaybeSpawnBackgroundThread()); + } +} From 9820b8ea09e47ca9c6e13d7bbc06d2df892aa6e0 Mon Sep 17 00:00:00 2001 From: toraxie Date: Thu, 12 Dec 2024 16:42:40 +0800 Subject: [PATCH 09/13] fix unittest --- ext/src/http/client/curl/http_client_curl.cc | 8 ++++---- ext/test/http/curl_http_test.cc | 5 ++++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 829344aba4..80b498a66c 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -433,10 +433,10 @@ bool HttpClient::MaybeSpawnBackgroundThread() } std::chrono::milliseconds wait_for = std::chrono::milliseconds::zero(); - ; -#if LIBCURL_VERSION_NUM >= 0x074200 - // only avaliable with curl_multi_poll, because curl_multi_wait would cause CPU busy, - // curl_multi_wait+sleep could not wakeup quickly + +#if LIBCURL_VERSION_NUM >= 0x074400 + // only available with curl_multi_poll+curl_multi_wakeup, because curl_multi_wait would + // cause CPU busy, curl_multi_wait+sleep could not wakeup quickly wait_for = self->background_thread_wait_for_; #endif if (self->is_shutdown_.load(std::memory_order_acquire)) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 3e843ed4fe..52e4409320 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -545,7 +545,10 @@ TEST_F(BasicCurlHttpTests, ElegantQuitQuick) // when use background_thread_wait_for_ should have no side effort on elegant quit // because ci machine may slow, so we assert it cost should less than // scheduled_delay_milliseconds_ - ASSERT_TRUE(std::chrono::system_clock::now() - beg < std::chrono::milliseconds{20}); + auto cost = std::chrono::system_clock::now() - beg; + ASSERT_TRUE(cost < std::chrono::milliseconds{10}) + << "cost ms: " << std::chrono::duration_cast(cost).count() + << " libcurl version: 0x" << std::hex << LIBCURL_VERSION_NUM; ASSERT_TRUE(handler->is_called_); ASSERT_TRUE(handler->got_response_); } From 66356c52e979ec7db3fa34df653100dee4d36e11 Mon Sep 17 00:00:00 2001 From: toraxie Date: Thu, 12 Dec 2024 19:43:59 +0800 Subject: [PATCH 10/13] wake background thread before quit --- ext/src/http/client/curl/http_client_curl.cc | 1 + ext/test/http/curl_http_test.cc | 9 +++++---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 80b498a66c..4a0e1a80d0 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -215,6 +215,7 @@ HttpClient::~HttpClient() } if (background_thread->joinable()) { + wakeupBackgroundThread(); // if delay quit, wake up first background_thread->join(); } } diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 52e4409320..982b448f2f 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -533,20 +533,21 @@ TEST_F(BasicCurlHttpTests, ElegantQuitQuick) { auto http_client = http_client::HttpClientFactory::Create(); std::static_pointer_cast(http_client)->MaybeSpawnBackgroundThread(); - auto beg = std::chrono::system_clock::now(); // start background first, then test it could wakeup auto session = http_client->CreateSession("http://127.0.0.1:19000/get/"); auto request = session->CreateRequest(); request->SetUri("get/"); auto handler = std::make_shared(); session->SendRequest(handler); + std::this_thread::sleep_for(std::chrono::milliseconds{10}); // let it enter poll state + auto beg = std::chrono::system_clock::now(); http_client->FinishAllSessions(); http_client.reset(); // when use background_thread_wait_for_ should have no side effort on elegant quit - // because ci machine may slow, so we assert it cost should less than - // scheduled_delay_milliseconds_ + // it shoule less than scheduled_delay_milliseconds_ + // because ci machine may slow, some take 10ms, so we assert it less than 20ms auto cost = std::chrono::system_clock::now() - beg; - ASSERT_TRUE(cost < std::chrono::milliseconds{10}) + ASSERT_TRUE(cost < std::chrono::milliseconds{20}) << "cost ms: " << std::chrono::duration_cast(cost).count() << " libcurl version: 0x" << std::hex << LIBCURL_VERSION_NUM; ASSERT_TRUE(handler->is_called_); From 98e926b830602662a42fa3eb87cab6a3050ac7ba Mon Sep 17 00:00:00 2001 From: Marc Alff Date: Tue, 17 Dec 2024 20:14:54 +0100 Subject: [PATCH 11/13] Apply suggestions from code review --- .../opentelemetry/ext/http/client/curl/http_client_curl.h | 2 +- ext/test/http/curl_http_test.cc | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h index f48adcbfc7..bef15997fc 100644 --- a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h +++ b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h @@ -358,7 +358,7 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient std::chrono::milliseconds scheduled_delay_milliseconds_; std::chrono::milliseconds background_thread_wait_for_; - std::atomic is_shutdown_; + std::atomic is_shutdown_{false}; nostd::shared_ptr curl_global_initializer_; }; diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 6fb126e8b8..497c5bb529 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -548,9 +548,9 @@ TEST_F(BasicCurlHttpTests, ElegantQuitQuick) auto beg = std::chrono::system_clock::now(); http_client->FinishAllSessions(); http_client.reset(); - // when use background_thread_wait_for_ should have no side effort on elegant quit - // it shoule less than scheduled_delay_milliseconds_ - // because ci machine may slow, some take 10ms, so we assert it less than 20ms + // when background_thread_wait_for_ is used, it should have no side effect on elegant quit + // wait should be less than scheduled_delay_milliseconds_ + // Due to load on CI hosts (some take 10ms), we assert it is less than 20ms auto cost = std::chrono::system_clock::now() - beg; ASSERT_TRUE(cost < std::chrono::milliseconds{20}) << "cost ms: " << std::chrono::duration_cast(cost).count() From 2d3a2479222f4733a2d94e5d68f1124f97f2cd22 Mon Sep 17 00:00:00 2001 From: Marc Alff Date: Tue, 17 Dec 2024 20:17:05 +0100 Subject: [PATCH 12/13] Update ext/src/http/client/curl/http_client_curl.cc --- ext/src/http/client/curl/http_client_curl.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 4a0e1a80d0..f53da0a469 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -191,9 +191,7 @@ HttpClient::HttpClient() scheduled_delay_milliseconds_{std::chrono::milliseconds(256)}, background_thread_wait_for_{std::chrono::minutes{1}}, curl_global_initializer_(HttpCurlGlobalInitializer::GetInstance()) -{ - is_shutdown_.store(false); -} +{} HttpClient::~HttpClient() { From 51553aee7c1ef4f8b757924fd513051acfdb8d50 Mon Sep 17 00:00:00 2001 From: toraxie Date: Wed, 18 Dec 2024 16:28:28 +0800 Subject: [PATCH 13/13] fix code by suggestion --- ext/src/http/client/curl/http_client_curl.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index f53da0a469..e6292f54f2 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -552,6 +552,7 @@ void HttpClient::WaitBackgroundThreadExit() if (background_thread && background_thread->joinable()) { + wakeupBackgroundThread(); background_thread->join(); } is_shutdown_.store(false, std::memory_order_release);