From 0c5f90dea5147461df4ff7d32cbb5d42e5011daa Mon Sep 17 00:00:00 2001 From: Marc Alff Date: Sat, 22 Jul 2023 16:51:57 +0200 Subject: [PATCH] [SDK] Valgrind errors on std::atomic variables (#2244) --- .../elasticsearch/es_log_record_exporter.h | 4 ++-- exporters/otlp/src/otlp_http_client.cc | 8 +++----- .../ext/http/client/curl/http_client_curl.h | 6 +++--- .../ext/http/client/curl/http_operation_curl.h | 12 ++++++------ .../ext/zpages/tracez_data_aggregator.h | 2 +- ext/test/http/curl_http_test.cc | 4 ++-- .../sdk/logs/batch_log_record_processor.h | 10 +++++----- .../sdk/logs/simple_log_record_processor.h | 2 +- .../export/periodic_exporting_metric_reader.h | 6 +++--- .../sdk/trace/batch_span_processor.h | 10 +++++----- sdk/src/logs/batch_log_record_processor.cc | 15 ++------------- sdk/src/trace/batch_span_processor.cc | 8 +------- 12 files changed, 34 insertions(+), 53 deletions(-) diff --git a/exporters/elasticsearch/include/opentelemetry/exporters/elasticsearch/es_log_record_exporter.h b/exporters/elasticsearch/include/opentelemetry/exporters/elasticsearch/es_log_record_exporter.h index 06b4c3b727..63e096ff97 100644 --- a/exporters/elasticsearch/include/opentelemetry/exporters/elasticsearch/es_log_record_exporter.h +++ b/exporters/elasticsearch/include/opentelemetry/exporters/elasticsearch/es_log_record_exporter.h @@ -125,8 +125,8 @@ class ElasticsearchLogRecordExporter final : public opentelemetry::sdk::logs::Lo # ifdef ENABLE_ASYNC_EXPORT struct SynchronizationData { - std::atomic session_counter_; - std::atomic finished_session_counter_; + std::atomic session_counter_{0}; + std::atomic finished_session_counter_{0}; std::condition_variable force_flush_cv; std::mutex force_flush_cv_m; std::recursive_mutex force_flush_m; diff --git a/exporters/otlp/src/otlp_http_client.cc b/exporters/otlp/src/otlp_http_client.cc index 44c154f725..a249a842ad 100644 --- a/exporters/otlp/src/otlp_http_client.cc +++ b/exporters/otlp/src/otlp_http_client.cc @@ -64,7 +64,7 @@ namespace { /** - * This class handles the response message from the Elasticsearch request + * This class handles the response message from the HTTP request */ class ResponseHandler : public http_client::EventHandler { @@ -75,9 +75,7 @@ class ResponseHandler : public http_client::EventHandler ResponseHandler(std::function &&callback, bool console_debug = false) : result_callback_{std::move(callback)}, console_debug_{console_debug} - { - stopping_.store(false); - } + {} std::string BuildResponseLogMessage(http_client::Response &response, const std::string &body) noexcept @@ -356,7 +354,7 @@ class ResponseHandler : public http_client::EventHandler const opentelemetry::ext::http::client::Session *session_ = nullptr; // Whether notify has been called - std::atomic stopping_; + std::atomic stopping_{false}; // A string to store the response body std::string body_ = ""; 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 900c5b6e81..876d9fab4f 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 @@ -159,7 +159,7 @@ class Session : public opentelemetry::ext::http::client::Session, std::string scheme = "http", const std::string &host = "", uint16_t port = 80) - : http_client_(http_client), is_session_active_(false) + : http_client_(http_client) { host_ = scheme + "://" + host + ":" + std::to_string(port) + "/"; } @@ -216,7 +216,7 @@ class Session : public opentelemetry::ext::http::client::Session, std::unique_ptr curl_operation_; uint64_t session_id_; HttpClient &http_client_; - std::atomic is_session_active_; + std::atomic is_session_active_{false}; }; class HttpClientSync : public opentelemetry::ext::http::client::HttpClientSync @@ -352,7 +352,7 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient std::mutex multi_handle_m_; CURLM *multi_handle_; - std::atomic next_session_id_; + std::atomic next_session_id_{0}; uint64_t max_sessions_per_connection_; std::mutex sessions_m_; diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h index b711ac8445..48ce11123c 100644 --- a/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h +++ b/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h @@ -273,11 +273,11 @@ class HttpOperation const char *GetCurlErrorMessage(CURLcode code); - std::atomic is_aborted_; // Set to 'true' when async callback is aborted - std::atomic is_finished_; // Set to 'true' when async callback is finished. - std::atomic is_cleaned_; // Set to 'true' when async callback is cleaned. - const bool is_raw_response_; // Do not split response headers from response body - const bool reuse_connection_; // Reuse connection + std::atomic is_aborted_{false}; // Set to 'true' when async callback is aborted + std::atomic is_finished_{false}; // Set to 'true' when async callback is finished. + std::atomic is_cleaned_{false}; // Set to 'true' when async callback is cleaned. + const bool is_raw_response_; // Do not split response headers from response body + const bool reuse_connection_; // Reuse connection const std::chrono::milliseconds http_conn_timeout_; // Timeout for connect. Default: 5000ms char curl_error_message_[CURL_ERROR_SIZE]; @@ -311,7 +311,7 @@ class HttpOperation std::thread::id callback_thread; std::function callback; - std::atomic is_promise_running; + std::atomic is_promise_running{false}; std::promise result_promise; std::future result_future; }; diff --git a/ext/include/opentelemetry/ext/zpages/tracez_data_aggregator.h b/ext/include/opentelemetry/ext/zpages/tracez_data_aggregator.h index 28236b60f5..62c944b704 100644 --- a/ext/include/opentelemetry/ext/zpages/tracez_data_aggregator.h +++ b/ext/include/opentelemetry/ext/zpages/tracez_data_aggregator.h @@ -155,7 +155,7 @@ class TracezDataAggregator /** A boolean that is set to true in the constructor and false in the * destructor to start and end execution of aggregate spans **/ - std::atomic execute_; + std::atomic execute_{false}; /** Thread that executes aggregate spans at regurlar intervals during this object's lifetime**/ diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 1b9539c5f8..78ecd92a6f 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -99,8 +99,8 @@ class BasicCurlHttpTests : public ::testing::Test, public HTTP_SERVER_NS::HttpRe protected: HTTP_SERVER_NS::HttpServer server_; std::string server_address_; - std::atomic is_setup_; - std::atomic is_running_; + std::atomic is_setup_{false}; + std::atomic is_running_{false}; std::vector received_requests_; std::mutex mtx_requests; std::condition_variable cv_got_events; diff --git a/sdk/include/opentelemetry/sdk/logs/batch_log_record_processor.h b/sdk/include/opentelemetry/sdk/logs/batch_log_record_processor.h index 23a1d947aa..d1cd045a58 100644 --- a/sdk/include/opentelemetry/sdk/logs/batch_log_record_processor.h +++ b/sdk/include/opentelemetry/sdk/logs/batch_log_record_processor.h @@ -115,11 +115,11 @@ class BatchLogRecordProcessor : public LogRecordProcessor std::mutex cv_m, force_flush_cv_m, shutdown_m; /* Important boolean flags to handle the workflow of the processor */ - std::atomic is_force_wakeup_background_worker; - std::atomic is_force_flush_pending; - std::atomic is_force_flush_notified; - std::atomic force_flush_timeout_us; - std::atomic is_shutdown; + std::atomic is_force_wakeup_background_worker{false}; + std::atomic is_force_flush_pending{false}; + std::atomic is_force_flush_notified{false}; + std::atomic force_flush_timeout_us{0}; + std::atomic is_shutdown{false}; }; /** diff --git a/sdk/include/opentelemetry/sdk/logs/simple_log_record_processor.h b/sdk/include/opentelemetry/sdk/logs/simple_log_record_processor.h index 8b273c6bb2..de9cecdd84 100644 --- a/sdk/include/opentelemetry/sdk/logs/simple_log_record_processor.h +++ b/sdk/include/opentelemetry/sdk/logs/simple_log_record_processor.h @@ -53,7 +53,7 @@ class SimpleLogRecordProcessor : public LogRecordProcessor // The lock used to ensure the exporter is not called concurrently opentelemetry::common::SpinLockMutex lock_; // The atomic boolean to ensure the ShutDown() function is only called once - std::atomic is_shutdown_; + std::atomic is_shutdown_{false}; }; } // namespace logs } // namespace sdk diff --git a/sdk/include/opentelemetry/sdk/metrics/export/periodic_exporting_metric_reader.h b/sdk/include/opentelemetry/sdk/metrics/export/periodic_exporting_metric_reader.h index 323727c873..b9221193fb 100644 --- a/sdk/include/opentelemetry/sdk/metrics/export/periodic_exporting_metric_reader.h +++ b/sdk/include/opentelemetry/sdk/metrics/export/periodic_exporting_metric_reader.h @@ -50,9 +50,9 @@ class PeriodicExportingMetricReader : public MetricReader std::thread worker_thread_; /* Synchronization primitives */ - std::atomic is_force_flush_pending_; - std::atomic is_force_wakeup_background_worker_; - std::atomic is_force_flush_notified_; + std::atomic is_force_flush_pending_{false}; + std::atomic is_force_wakeup_background_worker_{false}; + std::atomic is_force_flush_notified_{false}; std::condition_variable cv_, force_flush_cv_; std::mutex cv_m_, force_flush_m_; }; diff --git a/sdk/include/opentelemetry/sdk/trace/batch_span_processor.h b/sdk/include/opentelemetry/sdk/trace/batch_span_processor.h index 34d499a2ef..b93631eafa 100644 --- a/sdk/include/opentelemetry/sdk/trace/batch_span_processor.h +++ b/sdk/include/opentelemetry/sdk/trace/batch_span_processor.h @@ -114,11 +114,11 @@ class BatchSpanProcessor : public SpanProcessor std::mutex cv_m, force_flush_cv_m, shutdown_m; /* Important boolean flags to handle the workflow of the processor */ - std::atomic is_force_wakeup_background_worker; - std::atomic is_force_flush_pending; - std::atomic is_force_flush_notified; - std::atomic force_flush_timeout_us; - std::atomic is_shutdown; + std::atomic is_force_wakeup_background_worker{false}; + std::atomic is_force_flush_pending{false}; + std::atomic is_force_flush_notified{false}; + std::atomic force_flush_timeout_us{0}; + std::atomic is_shutdown{false}; }; /** diff --git a/sdk/src/logs/batch_log_record_processor.cc b/sdk/src/logs/batch_log_record_processor.cc index 5487d70e80..c4cede1a53 100644 --- a/sdk/src/logs/batch_log_record_processor.cc +++ b/sdk/src/logs/batch_log_record_processor.cc @@ -30,12 +30,7 @@ BatchLogRecordProcessor::BatchLogRecordProcessor( buffer_(max_queue_size_), synchronization_data_(std::make_shared()), worker_thread_(&BatchLogRecordProcessor::DoBackgroundWork, this) -{ - synchronization_data_->is_force_wakeup_background_worker.store(false); - synchronization_data_->is_force_flush_pending.store(false); - synchronization_data_->is_force_flush_notified.store(false); - synchronization_data_->is_shutdown.store(false); -} +{} BatchLogRecordProcessor::BatchLogRecordProcessor(std::unique_ptr &&exporter, const BatchLogRecordProcessorOptions &options) @@ -46,13 +41,7 @@ BatchLogRecordProcessor::BatchLogRecordProcessor(std::unique_ptr()), worker_thread_(&BatchLogRecordProcessor::DoBackgroundWork, this) -{ - synchronization_data_->is_force_wakeup_background_worker.store(false); - synchronization_data_->is_force_flush_pending.store(false); - synchronization_data_->is_force_flush_notified.store(false); - synchronization_data_->force_flush_timeout_us.store(0); - synchronization_data_->is_shutdown.store(false); -} +{} std::unique_ptr BatchLogRecordProcessor::MakeRecordable() noexcept { diff --git a/sdk/src/trace/batch_span_processor.cc b/sdk/src/trace/batch_span_processor.cc index dd69eaa68f..719a072e64 100644 --- a/sdk/src/trace/batch_span_processor.cc +++ b/sdk/src/trace/batch_span_processor.cc @@ -31,13 +31,7 @@ BatchSpanProcessor::BatchSpanProcessor(std::unique_ptr &&exporter, buffer_(max_queue_size_), synchronization_data_(std::make_shared()), worker_thread_(&BatchSpanProcessor::DoBackgroundWork, this) -{ - synchronization_data_->is_force_wakeup_background_worker.store(false); - synchronization_data_->is_force_flush_pending.store(false); - synchronization_data_->is_force_flush_notified.store(false); - synchronization_data_->force_flush_timeout_us.store(0); - synchronization_data_->is_shutdown.store(false); -} +{} std::unique_ptr BatchSpanProcessor::MakeRecordable() noexcept {