From d33a185c03c7bc050777e3fb6cd9a90b744f58ec Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Tue, 23 Feb 2021 16:42:55 +0000 Subject: [PATCH 01/41] initial Signed-off-by: chaoqin-li1123 --- source/common/common/thread.h | 4 ++++ source/common/config/delta_subscription_state.cc | 2 +- source/common/config/filesystem_subscription_impl.cc | 2 +- source/common/config/grpc_mux_impl.cc | 2 +- source/common/config/http_subscription_impl.cc | 4 ++-- source/common/runtime/runtime_impl.cc | 4 ++-- source/common/secret/sds_api.cc | 2 +- source/common/upstream/cds_api_impl.cc | 2 +- source/exe/main_common.cc | 4 ++-- .../grpc_credentials/file_based_metadata/config.cc | 2 +- .../injected_resource/injected_resource_monitor.cc | 2 +- source/server/admin/runtime_handler.cc | 2 +- source/server/config_validation/server.cc | 4 ++-- source/server/lds_api.cc | 2 +- source/server/server.cc | 8 ++++---- 15 files changed, 25 insertions(+), 21 deletions(-) diff --git a/source/common/common/thread.h b/source/common/common/thread.h index 819e5902b746..0083de4e710a 100644 --- a/source/common/common/thread.h +++ b/source/common/common/thread.h @@ -190,5 +190,9 @@ struct MainThread { std::thread::id main_thread_id_{std::this_thread::get_id()}; }; +#define envoy_try \ + assert(Thread::MainThread::isMainThread()); \ + try + } // namespace Thread } // namespace Envoy diff --git a/source/common/config/delta_subscription_state.cc b/source/common/config/delta_subscription_state.cc index 8821427c0c37..b60d602271f1 100644 --- a/source/common/config/delta_subscription_state.cc +++ b/source/common/config/delta_subscription_state.cc @@ -67,7 +67,7 @@ UpdateAck DeltaSubscriptionState::handleResponse( // We *always* copy the response's nonce into the next request, even if we're going to make that // request a NACK by setting error_detail. UpdateAck ack(message.nonce(), type_url_); - try { + envoy_try { handleGoodResponse(message); } catch (const EnvoyException& e) { handleBadResponse(e, ack); diff --git a/source/common/config/filesystem_subscription_impl.cc b/source/common/config/filesystem_subscription_impl.cc index 537fa8044260..4c401334ef49 100644 --- a/source/common/config/filesystem_subscription_impl.cc +++ b/source/common/config/filesystem_subscription_impl.cc @@ -61,7 +61,7 @@ void FilesystemSubscriptionImpl::refresh() { ENVOY_LOG(debug, "Filesystem config refresh for {}", path_); stats_.update_attempt_.inc(); ProtobufTypes::MessagePtr config_update; - try { + envoy_try { const std::string version = refreshInternal(&config_update); stats_.update_time_.set(DateUtil::nowToMilliseconds(api_.timeSource())); stats_.version_.set(HashUtil::xxHash64(version)); diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index ffd1e31319ef..47a22a086af5 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -186,7 +186,7 @@ void GrpcMuxImpl::onDiscoveryResponse( // the delta state. The proper fix for this is to converge these implementations, // see https://github.com/envoyproxy/envoy/issues/11477. same_type_resume = pause(type_url); - try { + envoy_try { // To avoid O(n^2) explosion (e.g. when we have 1000s of EDS watches), we // build a map here from resource name to resource and then walk watches_. // We have to walk all watches (and need an efficient map as a result) to diff --git a/source/common/config/http_subscription_impl.cc b/source/common/config/http_subscription_impl.cc index 054b4cc49946..de92a41ff72e 100644 --- a/source/common/config/http_subscription_impl.cc +++ b/source/common/config/http_subscription_impl.cc @@ -82,13 +82,13 @@ void HttpSubscriptionImpl::createRequest(Http::RequestMessage& request) { void HttpSubscriptionImpl::parseResponse(const Http::ResponseMessage& response) { disableInitFetchTimeoutTimer(); envoy::service::discovery::v3::DiscoveryResponse message; - try { + envoy_try { MessageUtil::loadFromJson(response.bodyAsString(), message, validation_visitor_); } catch (const EnvoyException& e) { handleFailure(Config::ConfigUpdateFailureReason::UpdateRejected, &e); return; } - try { + envoy_try { const auto decoded_resources = DecodedResourcesWrapper(resource_decoder_, message.resources(), message.version_info()); callbacks_.onConfigUpdate(decoded_resources.refvec_, message.version_info()); diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index eacc4b58ffa2..d9a57a9a2134 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -238,7 +238,7 @@ bool SnapshotImpl::parseEntryDoubleValue(Entry& entry) { void SnapshotImpl::parseEntryFractionalPercentValue(Entry& entry) { envoy::type::v3::FractionalPercent converted_fractional_percent; - try { + envoy_try { MessageUtil::loadFromYamlAndValidate(entry.raw_string_value_, converted_fractional_percent, ProtobufMessage::getStrictValidationVisitor()); } catch (const ProtoValidationException& ex) { @@ -569,7 +569,7 @@ SnapshotImplPtr LoaderImpl::createNewSnapshot() { path += "/" + service_cluster_; } if (api_.fileSystem().directoryExists(path)) { - try { + envoy_try { layers.emplace_back(std::make_unique(layer.name(), path, api_)); ++disk_layers; } catch (EnvoyException& e) { diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 83a660b40186..7caa5cb1489e 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -48,7 +48,7 @@ void SdsApi::resolveDataSource(const FileContentMap& files, void SdsApi::onWatchUpdate() { // Filesystem reads and update callbacks can fail if the key material is missing or bad. We're not // under an onConfigUpdate() context, so we need to catch these cases explicitly here. - try { + envoy_try { // Obtain a stable set of files. If a rotation happens while we're reading, // then we need to try again. uint64_t prev_hash = 0; diff --git a/source/common/upstream/cds_api_impl.cc b/source/common/upstream/cds_api_impl.cc index a2c95c8c5ca7..5c149330c28c 100644 --- a/source/common/upstream/cds_api_impl.cc +++ b/source/common/upstream/cds_api_impl.cc @@ -77,7 +77,7 @@ void CdsApiImpl::onConfigUpdate(const std::vector& a bool any_applied = false; for (const auto& resource : added_resources) { envoy::config::cluster::v3::Cluster cluster; - try { + envoy_try { cluster = dynamic_cast(resource.get().resource()); if (!cluster_names.insert(cluster.name()).second) { // NOTE: at this point, the first of these duplicates has already been successfully applied. diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc index 3ac535e19281..e140b53f448a 100644 --- a/source/exe/main_common.cc +++ b/source/exe/main_common.cc @@ -116,7 +116,7 @@ void MainCommonBase::configureHotRestarter(Random::RandomGenerator& random_gener // HotRestartImpl is going to multiply this value by 10, so leave head room. base_id = static_cast(random_generator.random()) & 0x0FFFFFFF; - try { + envoy_try { restarter = std::make_unique(base_id, 0, options_.socketPath(), options_.socketMode()); } catch (Server::HotRestartDomainSocketInUseException& ex) { @@ -213,7 +213,7 @@ int MainCommon::main(int argc, char** argv, PostServerHook hook) { // Initialize the server's main context under a try/catch loop and simply return EXIT_FAILURE // as needed. Whatever code in the initialization path that fails is expected to log an error // message so the user can diagnose. - try { + envoy_try { main_common = std::make_unique(argc, argv); Envoy::Server::Instance* server = main_common->server(); if (server != nullptr && hook != nullptr) { diff --git a/source/extensions/grpc_credentials/file_based_metadata/config.cc b/source/extensions/grpc_credentials/file_based_metadata/config.cc index fa918ff24380..49c31e90216b 100644 --- a/source/extensions/grpc_credentials/file_based_metadata/config.cc +++ b/source/extensions/grpc_credentials/file_based_metadata/config.cc @@ -69,7 +69,7 @@ FileBasedMetadataAuthenticator::GetMetadata(grpc::string_ref, grpc::string_ref, if (!config_.header_key().empty()) { header_key = config_.header_key(); } - try { + envoy_try { std::string header_value = Envoy::Config::DataSource::read(config_.secret_data(), true, api_); metadata->insert(std::make_pair(header_key, header_prefix + header_value)); } catch (const EnvoyException& e) { diff --git a/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc b/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc index 354d5970b43f..3d758065b37f 100644 --- a/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc +++ b/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc @@ -26,7 +26,7 @@ void InjectedResourceMonitor::onFileChanged() { file_changed_ = true; } void InjectedResourceMonitor::updateResourceUsage(Server::ResourceMonitor::Callbacks& callbacks) { if (file_changed_) { file_changed_ = false; - try { + envoy_try { const std::string contents = api_.fileSystem().fileReadToEnd(filename_); double pressure; if (absl::SimpleAtod(contents, &pressure)) { diff --git a/source/server/admin/runtime_handler.cc b/source/server/admin/runtime_handler.cc index 9c5e4c74b752..cf7c8cca8032 100644 --- a/source/server/admin/runtime_handler.cc +++ b/source/server/admin/runtime_handler.cc @@ -99,7 +99,7 @@ Http::Code RuntimeHandler::handlerRuntimeModify(absl::string_view url, Http::Res } absl::node_hash_map overrides; overrides.insert(params.begin(), params.end()); - try { + envoy_try { server_.runtime().mergeValues(overrides); } catch (const EnvoyException& e) { response.add(e.what()); diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index 30f125685e02..6c40cf9562b7 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -24,7 +24,7 @@ bool validateConfig(const Options& options, Thread::MutexBasicLockable access_log_lock; Stats::IsolatedStoreImpl stats_store; - try { + envoy_try { Event::RealTimeSystem time_system; ValidationInstance server(options, time_system, local_address, stats_store, access_log_lock, component_factory, thread_factory, file_system); @@ -53,7 +53,7 @@ ValidationInstance::ValidationInstance( mutex_tracer_(nullptr), grpc_context_(stats_store_.symbolTable()), http_context_(stats_store_.symbolTable()), router_context_(stats_store_.symbolTable()), time_system_(time_system), server_contexts_(*this) { - try { + envoy_try { initialize(options, local_address, component_factory); } catch (const EnvoyException& e) { ENVOY_LOG(critical, "error initializing configuration '{}': {}", options.configPath(), diff --git a/source/server/lds_api.cc b/source/server/lds_api.cc index 4daa03077b13..cae39ccd7efe 100644 --- a/source/server/lds_api.cc +++ b/source/server/lds_api.cc @@ -66,7 +66,7 @@ void LdsApiImpl::onConfigUpdate(const std::vector& a std::string message; for (const auto& resource : added_resources) { envoy::config::listener::v3::Listener listener; - try { + envoy_try { listener = dynamic_cast(resource.get().resource()); if (!listener_names.insert(listener.name()).second) { diff --git a/source/server/server.cc b/source/server/server.cc index 61e4c366f83f..bb9008a2adfe 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -90,9 +90,9 @@ InstanceImpl::InstanceImpl( grpc_context_(store.symbolTable()), http_context_(store.symbolTable()), router_context_(store.symbolTable()), process_context_(std::move(process_context)), hooks_(hooks), server_contexts_(*this) { - try { + envoy_try { if (!options.logPath().empty()) { - try { + envoy_try { file_logger_ = std::make_unique( options.logPath(), access_log_manager_, Logger::Registry::getSink()); } catch (const EnvoyException& e) { @@ -602,7 +602,7 @@ void InstanceImpl::onClusterManagerPrimaryInitializationComplete() { void InstanceImpl::onRuntimeReady() { // Begin initializing secondary clusters after RTDS configuration has been applied. // Initializing can throw exceptions, so catch these. - try { + envoy_try { clusterManager().initializeSecondaryClusters(bootstrap_); } catch (const EnvoyException& e) { ENVOY_LOG(warn, "Skipping initialization of secondary cluster: {}", e.what()); @@ -613,7 +613,7 @@ void InstanceImpl::onRuntimeReady() { const auto& hds_config = bootstrap_.hds_config(); async_client_manager_ = std::make_unique( *config_.clusterManager(), thread_local_, time_source_, *api_, grpc_context_.statNames()); - try { + envoy_try { hds_delegate_ = std::make_unique( stats_store_, Config::Utility::factoryForGrpcApiConfigSource(*async_client_manager_, hds_config, From 6ec2e5ff7da531989460a20d5b083ecaeaf5c198 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Tue, 23 Feb 2021 17:00:06 +0000 Subject: [PATCH 02/41] fix format Signed-off-by: chaoqin-li1123 --- .../common/config/delta_subscription_state.cc | 5 ++--- .../config/filesystem_subscription_impl.cc | 6 ++++-- source/common/config/grpc_mux_impl.cc | 3 ++- .../common/config/http_subscription_impl.cc | 8 ++++---- source/common/runtime/runtime_impl.cc | 9 ++++++--- source/common/secret/sds_api.cc | 3 ++- source/common/upstream/cds_api_impl.cc | 3 ++- source/exe/main_common.cc | 12 +++++++---- .../file_based_metadata/config.cc | 3 ++- .../injected_resource_monitor.cc | 3 ++- source/server/admin/runtime_handler.cc | 5 ++--- source/server/config_validation/server.cc | 8 ++++---- source/server/lds_api.cc | 3 ++- source/server/server.cc | 20 +++++++++++-------- 14 files changed, 54 insertions(+), 37 deletions(-) diff --git a/source/common/config/delta_subscription_state.cc b/source/common/config/delta_subscription_state.cc index b60d602271f1..376ca7a46489 100644 --- a/source/common/config/delta_subscription_state.cc +++ b/source/common/config/delta_subscription_state.cc @@ -67,9 +67,8 @@ UpdateAck DeltaSubscriptionState::handleResponse( // We *always* copy the response's nonce into the next request, even if we're going to make that // request a NACK by setting error_detail. UpdateAck ack(message.nonce(), type_url_); - envoy_try { - handleGoodResponse(message); - } catch (const EnvoyException& e) { + envoy_try { handleGoodResponse(message); } + catch (const EnvoyException& e) { handleBadResponse(e, ack); } return ack; diff --git a/source/common/config/filesystem_subscription_impl.cc b/source/common/config/filesystem_subscription_impl.cc index 4c401334ef49..fc3c1bc3efe1 100644 --- a/source/common/config/filesystem_subscription_impl.cc +++ b/source/common/config/filesystem_subscription_impl.cc @@ -69,9 +69,11 @@ void FilesystemSubscriptionImpl::refresh() { stats_.update_success_.inc(); ENVOY_LOG(debug, "Filesystem config update accepted for {}: {}", path_, config_update->DebugString()); - } catch (const ProtobufMessage::UnknownProtoFieldException& e) { + } + catch (const ProtobufMessage::UnknownProtoFieldException& e) { configRejected(e, config_update == nullptr ? "" : config_update->DebugString()); - } catch (const EnvoyException& e) { + } + catch (const EnvoyException& e) { if (config_update != nullptr) { configRejected(e, config_update->DebugString()); } else { diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 47a22a086af5..082565e6fd1a 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -251,7 +251,8 @@ void GrpcMuxImpl::onDiscoveryResponse( // would do that tracking here. apiStateFor(type_url).request_.set_version_info(message->version_info()); Memory::Utils::tryShrinkHeap(); - } catch (const EnvoyException& e) { + } + catch (const EnvoyException& e) { for (auto watch : apiStateFor(type_url).watches_) { watch->callbacks_.onConfigUpdateFailed( Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, &e); diff --git a/source/common/config/http_subscription_impl.cc b/source/common/config/http_subscription_impl.cc index de92a41ff72e..8608b55cae2f 100644 --- a/source/common/config/http_subscription_impl.cc +++ b/source/common/config/http_subscription_impl.cc @@ -82,9 +82,8 @@ void HttpSubscriptionImpl::createRequest(Http::RequestMessage& request) { void HttpSubscriptionImpl::parseResponse(const Http::ResponseMessage& response) { disableInitFetchTimeoutTimer(); envoy::service::discovery::v3::DiscoveryResponse message; - envoy_try { - MessageUtil::loadFromJson(response.bodyAsString(), message, validation_visitor_); - } catch (const EnvoyException& e) { + envoy_try { MessageUtil::loadFromJson(response.bodyAsString(), message, validation_visitor_); } + catch (const EnvoyException& e) { handleFailure(Config::ConfigUpdateFailureReason::UpdateRejected, &e); return; } @@ -97,7 +96,8 @@ void HttpSubscriptionImpl::parseResponse(const Http::ResponseMessage& response) stats_.version_.set(HashUtil::xxHash64(request_.version_info())); stats_.version_text_.set(request_.version_info()); stats_.update_success_.inc(); - } catch (const EnvoyException& e) { + } + catch (const EnvoyException& e) { handleFailure(Config::ConfigUpdateFailureReason::UpdateRejected, &e); } } diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index d9a57a9a2134..98006fa36d9b 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -241,10 +241,12 @@ void SnapshotImpl::parseEntryFractionalPercentValue(Entry& entry) { envoy_try { MessageUtil::loadFromYamlAndValidate(entry.raw_string_value_, converted_fractional_percent, ProtobufMessage::getStrictValidationVisitor()); - } catch (const ProtoValidationException& ex) { + } + catch (const ProtoValidationException& ex) { ENVOY_LOG(error, "unable to validate fraction percent runtime proto: {}", ex.what()); return; - } catch (const EnvoyException& ex) { + } + catch (const EnvoyException& ex) { // An EnvoyException is thrown when we try to parse a bogus string as a protobuf. This is fine, // since there was no expectation that the raw string was a valid proto. return; @@ -572,7 +574,8 @@ SnapshotImplPtr LoaderImpl::createNewSnapshot() { envoy_try { layers.emplace_back(std::make_unique(layer.name(), path, api_)); ++disk_layers; - } catch (EnvoyException& e) { + } + catch (EnvoyException& e) { // TODO(htuch): Consider latching here, rather than ignoring the // layer. This would be consistent with filesystem RTDS. ++error_layers; diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 7caa5cb1489e..30debb4eaf93 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -72,7 +72,8 @@ void SdsApi::onWatchUpdate() { update_callback_manager_.runCallbacks(); files_hash_ = new_hash; } - } catch (const EnvoyException& e) { + } + catch (const EnvoyException& e) { ENVOY_LOG_MISC(warn, fmt::format("Failed to reload certificates: {}", e.what())); sds_api_stats_.key_rotation_failed_.inc(); } diff --git a/source/common/upstream/cds_api_impl.cc b/source/common/upstream/cds_api_impl.cc index 5c149330c28c..e26aea3240e7 100644 --- a/source/common/upstream/cds_api_impl.cc +++ b/source/common/upstream/cds_api_impl.cc @@ -89,7 +89,8 @@ void CdsApiImpl::onConfigUpdate(const std::vector& a } else { ENVOY_LOG(debug, "cds: add/update cluster '{}' skipped", cluster.name()); } - } catch (const EnvoyException& e) { + } + catch (const EnvoyException& e) { exception_msgs.push_back(fmt::format("{}: {}", cluster.name(), e.what())); } } diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc index e140b53f448a..3b483741d403 100644 --- a/source/exe/main_common.cc +++ b/source/exe/main_common.cc @@ -119,7 +119,8 @@ void MainCommonBase::configureHotRestarter(Random::RandomGenerator& random_gener envoy_try { restarter = std::make_unique(base_id, 0, options_.socketPath(), options_.socketMode()); - } catch (Server::HotRestartDomainSocketInUseException& ex) { + } + catch (Server::HotRestartDomainSocketInUseException& ex) { // No luck, try again. ENVOY_LOG_MISC(debug, "dynamic base id: {}", ex.what()); } @@ -219,12 +220,15 @@ int MainCommon::main(int argc, char** argv, PostServerHook hook) { if (server != nullptr && hook != nullptr) { hook(*server); } - } catch (const Envoy::NoServingException& e) { + } + catch (const Envoy::NoServingException& e) { return EXIT_SUCCESS; - } catch (const Envoy::MalformedArgvException& e) { + } + catch (const Envoy::MalformedArgvException& e) { std::cerr << e.what() << std::endl; return EXIT_FAILURE; - } catch (const Envoy::EnvoyException& e) { + } + catch (const Envoy::EnvoyException& e) { std::cerr << e.what() << std::endl; return EXIT_FAILURE; } diff --git a/source/extensions/grpc_credentials/file_based_metadata/config.cc b/source/extensions/grpc_credentials/file_based_metadata/config.cc index 49c31e90216b..e736cc67c923 100644 --- a/source/extensions/grpc_credentials/file_based_metadata/config.cc +++ b/source/extensions/grpc_credentials/file_based_metadata/config.cc @@ -72,7 +72,8 @@ FileBasedMetadataAuthenticator::GetMetadata(grpc::string_ref, grpc::string_ref, envoy_try { std::string header_value = Envoy::Config::DataSource::read(config_.secret_data(), true, api_); metadata->insert(std::make_pair(header_key, header_prefix + header_value)); - } catch (const EnvoyException& e) { + } + catch (const EnvoyException& e) { return grpc::Status(grpc::StatusCode::NOT_FOUND, e.what()); } return grpc::Status::OK; diff --git a/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc b/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc index 3d758065b37f..7bb06f2d5b42 100644 --- a/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc +++ b/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc @@ -38,7 +38,8 @@ void InjectedResourceMonitor::updateResourceUsage(Server::ResourceMonitor::Callb } else { throw EnvoyException("failed to parse injected resource pressure"); } - } catch (const EnvoyException& error) { + } + catch (const EnvoyException& error) { error_ = error; pressure_.reset(); } diff --git a/source/server/admin/runtime_handler.cc b/source/server/admin/runtime_handler.cc index cf7c8cca8032..2e8b62730e8b 100644 --- a/source/server/admin/runtime_handler.cc +++ b/source/server/admin/runtime_handler.cc @@ -99,9 +99,8 @@ Http::Code RuntimeHandler::handlerRuntimeModify(absl::string_view url, Http::Res } absl::node_hash_map overrides; overrides.insert(params.begin(), params.end()); - envoy_try { - server_.runtime().mergeValues(overrides); - } catch (const EnvoyException& e) { + envoy_try { server_.runtime().mergeValues(overrides); } + catch (const EnvoyException& e) { response.add(e.what()); return Http::Code::ServiceUnavailable; } diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index 6c40cf9562b7..30dce8e48d15 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -31,7 +31,8 @@ bool validateConfig(const Options& options, std::cout << "configuration '" << options.configPath() << "' OK" << std::endl; server.shutdown(); return true; - } catch (const EnvoyException& e) { + } + catch (const EnvoyException& e) { return false; } } @@ -53,9 +54,8 @@ ValidationInstance::ValidationInstance( mutex_tracer_(nullptr), grpc_context_(stats_store_.symbolTable()), http_context_(stats_store_.symbolTable()), router_context_(stats_store_.symbolTable()), time_system_(time_system), server_contexts_(*this) { - envoy_try { - initialize(options, local_address, component_factory); - } catch (const EnvoyException& e) { + envoy_try { initialize(options, local_address, component_factory); } + catch (const EnvoyException& e) { ENVOY_LOG(critical, "error initializing configuration '{}': {}", options.configPath(), e.what()); shutdown(); diff --git a/source/server/lds_api.cc b/source/server/lds_api.cc index cae39ccd7efe..6ae69b88f606 100644 --- a/source/server/lds_api.cc +++ b/source/server/lds_api.cc @@ -80,7 +80,8 @@ void LdsApiImpl::onConfigUpdate(const std::vector& a } else { ENVOY_LOG(debug, "lds: add/update listener '{}' skipped", listener.name()); } - } catch (const EnvoyException& e) { + } + catch (const EnvoyException& e) { failure_state.push_back(std::make_unique()); auto& state = failure_state.back(); state->set_details(e.what()); diff --git a/source/server/server.cc b/source/server/server.cc index bb9008a2adfe..e70af8526a62 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -95,7 +95,8 @@ InstanceImpl::InstanceImpl( envoy_try { file_logger_ = std::make_unique( options.logPath(), access_log_manager_, Logger::Registry::getSink()); - } catch (const EnvoyException& e) { + } + catch (const EnvoyException& e) { throw EnvoyException( fmt::format("Failed to open log-file '{}'. e.what(): {}", options.logPath(), e.what())); } @@ -104,16 +105,19 @@ InstanceImpl::InstanceImpl( restarter_.initialize(*dispatcher_, *this); drain_manager_ = component_factory.createDrainManager(*this); initialize(options, std::move(local_address), component_factory, hooks); - } catch (const EnvoyException& e) { + } + catch (const EnvoyException& e) { ENVOY_LOG(critical, "error initializing configuration '{}': {}", options.configPath(), e.what()); terminate(); throw; - } catch (const std::exception& e) { + } + catch (const std::exception& e) { ENVOY_LOG(critical, "error initializing due to unexpected exception: {}", e.what()); terminate(); throw; - } catch (...) { + } + catch (...) { ENVOY_LOG(critical, "error initializing due to unknown exception"); terminate(); throw; @@ -602,9 +606,8 @@ void InstanceImpl::onClusterManagerPrimaryInitializationComplete() { void InstanceImpl::onRuntimeReady() { // Begin initializing secondary clusters after RTDS configuration has been applied. // Initializing can throw exceptions, so catch these. - envoy_try { - clusterManager().initializeSecondaryClusters(bootstrap_); - } catch (const EnvoyException& e) { + envoy_try { clusterManager().initializeSecondaryClusters(bootstrap_); } + catch (const EnvoyException& e) { ENVOY_LOG(warn, "Skipping initialization of secondary cluster: {}", e.what()); shutdown(); } @@ -624,7 +627,8 @@ void InstanceImpl::onRuntimeReady() { access_log_manager_, *config_.clusterManager(), *local_info_, *admin_, *singleton_manager_, thread_local_, messageValidationContext().dynamicValidationVisitor(), *api_, options_); - } catch (const EnvoyException& e) { + } + catch (const EnvoyException& e) { ENVOY_LOG(warn, "Skipping initialization of HDS cluster: {}", e.what()); shutdown(); } From 626b80000dbbe0bde07ac24010326aaf02f6a95e Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Tue, 23 Feb 2021 22:58:52 +0000 Subject: [PATCH 03/41] remove some raw try and fix format Signed-off-by: chaoqin-li1123 --- source/common/formatter/substitution_formatter.cc | 5 ++--- source/common/http/rest_api_fetcher.cc | 5 ++--- source/common/network/apple_dns_impl.cc | 5 +++-- source/common/upstream/health_discovery_service.cc | 5 ++--- source/common/upstream/upstream_impl.cc | 5 ++--- source/server/listener_manager_impl.cc | 5 ++--- source/server/options_impl.cc | 13 +++++++------ 7 files changed, 20 insertions(+), 23 deletions(-) diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc index c264c23aec83..c57c46ab5734 100644 --- a/source/common/formatter/substitution_formatter.cc +++ b/source/common/formatter/substitution_formatter.cc @@ -1287,9 +1287,8 @@ ProtobufWkt::Value FilterStateFormatter::formatValue(const Http::RequestHeaderMa } ProtobufWkt::Value val; - try { - MessageUtil::jsonConvertValue(*proto, val); - } catch (EnvoyException& ex) { + envoy_try { MessageUtil::jsonConvertValue(*proto, val); } + catch (EnvoyException& ex) { return unspecifiedValue(); } return val; diff --git a/source/common/http/rest_api_fetcher.cc b/source/common/http/rest_api_fetcher.cc index e65b518d4543..981370988231 100644 --- a/source/common/http/rest_api_fetcher.cc +++ b/source/common/http/rest_api_fetcher.cc @@ -39,9 +39,8 @@ void RestApiFetcher::onSuccess(const Http::AsyncClient::Request& request, return; } - try { - parseResponse(*response); - } catch (EnvoyException& e) { + envoy_try { parseResponse(*response); } + catch (EnvoyException& e) { onFetchFailure(Config::ConfigUpdateFailureReason::UpdateRejected, &e); } diff --git a/source/common/network/apple_dns_impl.cc b/source/common/network/apple_dns_impl.cc index f8994cb48e8c..75465c4ad05e 100644 --- a/source/common/network/apple_dns_impl.cc +++ b/source/common/network/apple_dns_impl.cc @@ -148,7 +148,7 @@ ActiveDnsQuery* AppleDnsResolverImpl::resolve(const std::string& dns_name, ENVOY_LOG(debug, "DNS resolver resolve={}", dns_name); Address::InstanceConstSharedPtr address{}; - try { + envoy_try { // When an IP address is submitted to c-ares in DnsResolverImpl, c-ares synchronously returns // the IP without submitting a DNS query. Because Envoy has come to rely on this behavior, this // resolver implements a similar resolution path to avoid making improper DNS queries for @@ -156,7 +156,8 @@ ActiveDnsQuery* AppleDnsResolverImpl::resolve(const std::string& dns_name, address = Utility::parseInternetAddress(dns_name); ENVOY_LOG(debug, "DNS resolver resolved ({}) to ({}) without issuing call to Apple API", dns_name, address->asString()); - } catch (const EnvoyException& e) { + } + catch (const EnvoyException& e) { // Resolution via Apple APIs ENVOY_LOG(trace, "DNS resolver local resolution failed with: {}", e.what()); diff --git a/source/common/upstream/health_discovery_service.cc b/source/common/upstream/health_discovery_service.cc index 79535af0c066..b9887cfdc000 100644 --- a/source/common/upstream/health_discovery_service.cc +++ b/source/common/upstream/health_discovery_service.cc @@ -293,9 +293,8 @@ void HdsDelegate::onReceiveMessage( } // Validate message fields - try { - MessageUtil::validate(*message, validation_visitor_); - } catch (const ProtoValidationException& ex) { + envoy_try { MessageUtil::validate(*message, validation_visitor_); } + catch (const ProtoValidationException& ex) { // Increment error count stats_.errors_.inc(); ENVOY_LOG(warn, "Unable to validate health check specifier: {}", ex.what()); diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 411f49ae7f2b..2013c16659cc 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -1131,9 +1131,8 @@ void ClusterImplBase::reloadHealthyHostsHelper(const HostSharedPtr&) { const Network::Address::InstanceConstSharedPtr ClusterImplBase::resolveProtoAddress(const envoy::config::core::v3::Address& address) { - try { - return Network::Address::resolveProtoAddress(address); - } catch (EnvoyException& e) { + envoy_try { return Network::Address::resolveProtoAddress(address); } + catch (EnvoyException& e) { if (info_->type() == envoy::config::cluster::v3::Cluster::STATIC || info_->type() == envoy::config::cluster::v3::Cluster::EDS) { throw EnvoyException(fmt::format("{}. Consider setting resolver_name or setting cluster type " diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 285601a7e7cd..af597bc4dba4 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -360,9 +360,8 @@ bool ListenerManagerImpl::addOrUpdateListener(const envoy::config::listener::v3: } auto it = error_state_tracker_.find(name); - try { - return addOrUpdateListenerInternal(config, version_info, added_via_api, name); - } catch (const EnvoyException& e) { + envoy_try { return addOrUpdateListenerInternal(config, version_info, added_via_api, name); } + catch (const EnvoyException& e) { if (it == error_state_tracker_.end()) { it = error_state_tracker_.emplace(name, std::make_unique()).first; } diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index d4623d5a4141..cff6625d9869 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -158,18 +158,19 @@ OptionsImpl::OptionsImpl(std::vector args, "600", "string", cmd); cmd.setExceptionHandling(false); - try { + envoy_try { cmd.parse(args); count_ = cmd.getArgList().size(); - } catch (TCLAP::ArgException& e) { - try { - cmd.getOutput()->failure(cmd, e); - } catch (const TCLAP::ExitException&) { + } + catch (TCLAP::ArgException& e) { + envoy_try { cmd.getOutput()->failure(cmd, e); } + catch (const TCLAP::ExitException&) { // failure() has already written an informative message to stderr, so all that's left to do // is throw our own exception with the original message. throw MalformedArgvException(e.what()); } - } catch (const TCLAP::ExitException& e) { + } + catch (const TCLAP::ExitException& e) { // parse() throws an ExitException with status 0 after printing the output for --help and // --version. throw NoServingException(); From b053baf6c7a349f997f6bad10f40da2a1d6fb29d Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 24 Feb 2021 04:14:47 +0000 Subject: [PATCH 04/41] access log Signed-off-by: chaoqin-li1123 --- .../access_log/access_log_manager_impl.cc | 32 +++++++++++-------- .../access_log/access_log_manager_impl.h | 2 +- .../common/upstream/original_dst_cluster.cc | 15 +++++---- 3 files changed, 28 insertions(+), 21 deletions(-) diff --git a/source/common/access_log/access_log_manager_impl.cc b/source/common/access_log/access_log_manager_impl.cc index 0cd104ef0f49..258736df18a9 100644 --- a/source/common/access_log/access_log_manager_impl.cc +++ b/source/common/access_log/access_log_manager_impl.cc @@ -59,12 +59,16 @@ Filesystem::FlagSet AccessLogFileImpl::defaultFlags() { return default_flags; } -void AccessLogFileImpl::open() { +bool AccessLogFileImpl::open() { const Api::IoCallBoolResult result = file_->open(defaultFlags()); if (!result.rc_) { - throw EnvoyException( - fmt::format("unable to open file '{}': {}", file_->path(), result.err_->getErrorDetails())); + /* + ENVOY_LOG(debug, + "unable to open file '{}': {}", file_->path(), result.err_->getErrorDetails()); + */ + return false; } + return true; } void AccessLogFileImpl::reopen() { reopen_file_ = true; } @@ -147,19 +151,21 @@ void AccessLogFileImpl::flushThreadFunc() { // if we failed to open file before, then simply ignore if (file_->isOpen()) { - try { - if (reopen_file_) { - reopen_file_ = false; - const Api::IoCallBoolResult result = file_->close(); - ASSERT(result.rc_, fmt::format("unable to close file '{}': {}", file_->path(), - result.err_->getErrorDetails())); - open(); - } - + bool open_success = true; + if (reopen_file_) { + reopen_file_ = false; + const Api::IoCallBoolResult result = file_->close(); + ASSERT(result.rc_, fmt::format("unable to close file '{}': {}", file_->path(), + result.err_->getErrorDetails())); + open_success = open(); + } + if (open_success) { doWrite(about_to_write_buffer_); - } catch (const EnvoyException&) { + } + else { stats_.reopen_failed_.inc(); } + } } } diff --git a/source/common/access_log/access_log_manager_impl.h b/source/common/access_log/access_log_manager_impl.h index 1727a0bbf053..58dcb159fc68 100644 --- a/source/common/access_log/access_log_manager_impl.h +++ b/source/common/access_log/access_log_manager_impl.h @@ -84,7 +84,7 @@ class AccessLogFileImpl : public AccessLogFile { private: void doWrite(Buffer::Instance& buffer); void flushThreadFunc(); - void open(); + bool open(); void createFlushStructures(); // return default flags set which used by open diff --git a/source/common/upstream/original_dst_cluster.cc b/source/common/upstream/original_dst_cluster.cc index f869a22d039c..67dce1199fc0 100644 --- a/source/common/upstream/original_dst_cluster.cc +++ b/source/common/upstream/original_dst_cluster.cc @@ -89,16 +89,17 @@ OriginalDstCluster::LoadBalancer::requestOverrideHost(LoadBalancerContext* conte override_header = downstream_headers->get(Http::Headers::get().EnvoyOriginalDstHost); } if (!override_header.empty()) { - try { // This is an implicitly untrusted header, so per the API documentation only the first // value is used. const std::string request_override_host(override_header[0]->value().getStringView()); - request_host = Network::Utility::parseInternetAddressAndPort(request_override_host, false); - ENVOY_LOG(debug, "Using request override host {}.", request_override_host); - } catch (const Envoy::EnvoyException& e) { - ENVOY_LOG(debug, "original_dst_load_balancer: invalid override header value. {}", e.what()); - parent_->info()->stats().original_dst_host_invalid_.inc(); - } + request_host = Network::Utility::parseInternetAddressAndPortNoThrow(request_override_host, false); + if (request_host) { + ENVOY_LOG(debug, "Using request override host {}.", request_override_host); + } + else { + ENVOY_LOG(debug, "original_dst_load_balancer: invalid override header value. {}", request_override_host); + parent_->info()->stats().original_dst_host_invalid_.inc(); + } } return request_host; } From 5d90db0ef163baf3d2d6fa26e632ba2b8df35451 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 24 Feb 2021 06:11:02 +0000 Subject: [PATCH 05/41] fix test Signed-off-by: chaoqin-li1123 --- .../access_log/access_log_manager_impl.cc | 32 ++++++++----------- .../access_log/access_log_manager_impl.h | 2 +- 2 files changed, 14 insertions(+), 20 deletions(-) diff --git a/source/common/access_log/access_log_manager_impl.cc b/source/common/access_log/access_log_manager_impl.cc index 258736df18a9..0cd104ef0f49 100644 --- a/source/common/access_log/access_log_manager_impl.cc +++ b/source/common/access_log/access_log_manager_impl.cc @@ -59,16 +59,12 @@ Filesystem::FlagSet AccessLogFileImpl::defaultFlags() { return default_flags; } -bool AccessLogFileImpl::open() { +void AccessLogFileImpl::open() { const Api::IoCallBoolResult result = file_->open(defaultFlags()); if (!result.rc_) { - /* - ENVOY_LOG(debug, - "unable to open file '{}': {}", file_->path(), result.err_->getErrorDetails()); - */ - return false; + throw EnvoyException( + fmt::format("unable to open file '{}': {}", file_->path(), result.err_->getErrorDetails())); } - return true; } void AccessLogFileImpl::reopen() { reopen_file_ = true; } @@ -151,21 +147,19 @@ void AccessLogFileImpl::flushThreadFunc() { // if we failed to open file before, then simply ignore if (file_->isOpen()) { - bool open_success = true; - if (reopen_file_) { - reopen_file_ = false; - const Api::IoCallBoolResult result = file_->close(); - ASSERT(result.rc_, fmt::format("unable to close file '{}': {}", file_->path(), - result.err_->getErrorDetails())); - open_success = open(); - } - if (open_success) { + try { + if (reopen_file_) { + reopen_file_ = false; + const Api::IoCallBoolResult result = file_->close(); + ASSERT(result.rc_, fmt::format("unable to close file '{}': {}", file_->path(), + result.err_->getErrorDetails())); + open(); + } + doWrite(about_to_write_buffer_); - } - else { + } catch (const EnvoyException&) { stats_.reopen_failed_.inc(); } - } } } diff --git a/source/common/access_log/access_log_manager_impl.h b/source/common/access_log/access_log_manager_impl.h index 58dcb159fc68..1727a0bbf053 100644 --- a/source/common/access_log/access_log_manager_impl.h +++ b/source/common/access_log/access_log_manager_impl.h @@ -84,7 +84,7 @@ class AccessLogFileImpl : public AccessLogFile { private: void doWrite(Buffer::Instance& buffer); void flushThreadFunc(); - bool open(); + void open(); void createFlushStructures(); // return default flags set which used by open From f4e4c33c7a33e5bf48b33ec63cbac30a729a1bbe Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 26 Feb 2021 21:35:59 +0000 Subject: [PATCH 06/41] new macros Signed-off-by: chaoqin-li1123 --- source/common/common/thread.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/source/common/common/thread.h b/source/common/common/thread.h index 0083de4e710a..359842ccbc4f 100644 --- a/source/common/common/thread.h +++ b/source/common/common/thread.h @@ -194,5 +194,11 @@ struct MainThread { assert(Thread::MainThread::isMainThread()); \ try +#define TRY \ +try { \ +assert(Thread::MainThread::isMainThread()); + +#define END_TRY } + } // namespace Thread } // namespace Envoy From 5be7631cfdc7b433a054a2a924223d07f66bd1b1 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Fri, 26 Feb 2021 22:08:19 +0000 Subject: [PATCH 07/41] USE new macros Signed-off-by: chaoqin-li1123 --- source/common/config/delta_subscription_state.cc | 2 +- .../common/config/filesystem_subscription_impl.cc | 4 ++-- source/common/config/grpc_mux_impl.cc | 4 ++-- source/common/config/http_subscription_impl.cc | 6 +++--- source/common/formatter/substitution_formatter.cc | 2 +- source/common/http/rest_api_fetcher.cc | 2 +- source/common/network/apple_dns_impl.cc | 4 ++-- source/common/runtime/runtime_impl.cc | 8 ++++---- source/common/secret/sds_api.cc | 4 ++-- source/common/upstream/cds_api_impl.cc | 4 ++-- source/common/upstream/health_discovery_service.cc | 2 +- source/common/upstream/upstream_impl.cc | 2 +- source/exe/main_common.cc | 8 ++++---- .../grpc_credentials/file_based_metadata/config.cc | 4 ++-- .../injected_resource/injected_resource_monitor.cc | 4 ++-- source/server/admin/runtime_handler.cc | 2 +- source/server/config_validation/server.cc | 6 +++--- source/server/lds_api.cc | 4 ++-- source/server/listener_manager_impl.cc | 2 +- source/server/options_impl.cc | 6 +++--- source/server/server.cc | 14 +++++++------- 21 files changed, 47 insertions(+), 47 deletions(-) diff --git a/source/common/config/delta_subscription_state.cc b/source/common/config/delta_subscription_state.cc index 376ca7a46489..67b35ad1f262 100644 --- a/source/common/config/delta_subscription_state.cc +++ b/source/common/config/delta_subscription_state.cc @@ -67,7 +67,7 @@ UpdateAck DeltaSubscriptionState::handleResponse( // We *always* copy the response's nonce into the next request, even if we're going to make that // request a NACK by setting error_detail. UpdateAck ack(message.nonce(), type_url_); - envoy_try { handleGoodResponse(message); } + TRY { handleGoodResponse(message); } END_TRY catch (const EnvoyException& e) { handleBadResponse(e, ack); } diff --git a/source/common/config/filesystem_subscription_impl.cc b/source/common/config/filesystem_subscription_impl.cc index fc3c1bc3efe1..b0a36adabbe8 100644 --- a/source/common/config/filesystem_subscription_impl.cc +++ b/source/common/config/filesystem_subscription_impl.cc @@ -61,7 +61,7 @@ void FilesystemSubscriptionImpl::refresh() { ENVOY_LOG(debug, "Filesystem config refresh for {}", path_); stats_.update_attempt_.inc(); ProtobufTypes::MessagePtr config_update; - envoy_try { + TRY { const std::string version = refreshInternal(&config_update); stats_.update_time_.set(DateUtil::nowToMilliseconds(api_.timeSource())); stats_.version_.set(HashUtil::xxHash64(version)); @@ -69,7 +69,7 @@ void FilesystemSubscriptionImpl::refresh() { stats_.update_success_.inc(); ENVOY_LOG(debug, "Filesystem config update accepted for {}: {}", path_, config_update->DebugString()); - } + } END_TRY catch (const ProtobufMessage::UnknownProtoFieldException& e) { configRejected(e, config_update == nullptr ? "" : config_update->DebugString()); } diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 082565e6fd1a..946fad9f8950 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -186,7 +186,7 @@ void GrpcMuxImpl::onDiscoveryResponse( // the delta state. The proper fix for this is to converge these implementations, // see https://github.com/envoyproxy/envoy/issues/11477. same_type_resume = pause(type_url); - envoy_try { + TRY { // To avoid O(n^2) explosion (e.g. when we have 1000s of EDS watches), we // build a map here from resource name to resource and then walk watches_. // We have to walk all watches (and need an efficient map as a result) to @@ -251,7 +251,7 @@ void GrpcMuxImpl::onDiscoveryResponse( // would do that tracking here. apiStateFor(type_url).request_.set_version_info(message->version_info()); Memory::Utils::tryShrinkHeap(); - } + } END_TRY catch (const EnvoyException& e) { for (auto watch : apiStateFor(type_url).watches_) { watch->callbacks_.onConfigUpdateFailed( diff --git a/source/common/config/http_subscription_impl.cc b/source/common/config/http_subscription_impl.cc index 8608b55cae2f..ec6f24f8e244 100644 --- a/source/common/config/http_subscription_impl.cc +++ b/source/common/config/http_subscription_impl.cc @@ -82,12 +82,12 @@ void HttpSubscriptionImpl::createRequest(Http::RequestMessage& request) { void HttpSubscriptionImpl::parseResponse(const Http::ResponseMessage& response) { disableInitFetchTimeoutTimer(); envoy::service::discovery::v3::DiscoveryResponse message; - envoy_try { MessageUtil::loadFromJson(response.bodyAsString(), message, validation_visitor_); } + TRY { MessageUtil::loadFromJson(response.bodyAsString(), message, validation_visitor_); } END_TRY catch (const EnvoyException& e) { handleFailure(Config::ConfigUpdateFailureReason::UpdateRejected, &e); return; } - envoy_try { + TRY { const auto decoded_resources = DecodedResourcesWrapper(resource_decoder_, message.resources(), message.version_info()); callbacks_.onConfigUpdate(decoded_resources.refvec_, message.version_info()); @@ -96,7 +96,7 @@ void HttpSubscriptionImpl::parseResponse(const Http::ResponseMessage& response) stats_.version_.set(HashUtil::xxHash64(request_.version_info())); stats_.version_text_.set(request_.version_info()); stats_.update_success_.inc(); - } + } END_TRY catch (const EnvoyException& e) { handleFailure(Config::ConfigUpdateFailureReason::UpdateRejected, &e); } diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc index c57c46ab5734..12b034d76646 100644 --- a/source/common/formatter/substitution_formatter.cc +++ b/source/common/formatter/substitution_formatter.cc @@ -1287,7 +1287,7 @@ ProtobufWkt::Value FilterStateFormatter::formatValue(const Http::RequestHeaderMa } ProtobufWkt::Value val; - envoy_try { MessageUtil::jsonConvertValue(*proto, val); } + TRY { MessageUtil::jsonConvertValue(*proto, val); } END_TRY catch (EnvoyException& ex) { return unspecifiedValue(); } diff --git a/source/common/http/rest_api_fetcher.cc b/source/common/http/rest_api_fetcher.cc index 981370988231..4965edc76e85 100644 --- a/source/common/http/rest_api_fetcher.cc +++ b/source/common/http/rest_api_fetcher.cc @@ -39,7 +39,7 @@ void RestApiFetcher::onSuccess(const Http::AsyncClient::Request& request, return; } - envoy_try { parseResponse(*response); } + TRY { parseResponse(*response); } END_TRY catch (EnvoyException& e) { onFetchFailure(Config::ConfigUpdateFailureReason::UpdateRejected, &e); } diff --git a/source/common/network/apple_dns_impl.cc b/source/common/network/apple_dns_impl.cc index 75465c4ad05e..e4d9d4c45640 100644 --- a/source/common/network/apple_dns_impl.cc +++ b/source/common/network/apple_dns_impl.cc @@ -148,7 +148,7 @@ ActiveDnsQuery* AppleDnsResolverImpl::resolve(const std::string& dns_name, ENVOY_LOG(debug, "DNS resolver resolve={}", dns_name); Address::InstanceConstSharedPtr address{}; - envoy_try { + TRY { // When an IP address is submitted to c-ares in DnsResolverImpl, c-ares synchronously returns // the IP without submitting a DNS query. Because Envoy has come to rely on this behavior, this // resolver implements a similar resolution path to avoid making improper DNS queries for @@ -156,7 +156,7 @@ ActiveDnsQuery* AppleDnsResolverImpl::resolve(const std::string& dns_name, address = Utility::parseInternetAddress(dns_name); ENVOY_LOG(debug, "DNS resolver resolved ({}) to ({}) without issuing call to Apple API", dns_name, address->asString()); - } + } END_TRY catch (const EnvoyException& e) { // Resolution via Apple APIs ENVOY_LOG(trace, "DNS resolver local resolution failed with: {}", e.what()); diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index 98006fa36d9b..34d25fdbee11 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -238,10 +238,10 @@ bool SnapshotImpl::parseEntryDoubleValue(Entry& entry) { void SnapshotImpl::parseEntryFractionalPercentValue(Entry& entry) { envoy::type::v3::FractionalPercent converted_fractional_percent; - envoy_try { + TRY { MessageUtil::loadFromYamlAndValidate(entry.raw_string_value_, converted_fractional_percent, ProtobufMessage::getStrictValidationVisitor()); - } + } END_TRY catch (const ProtoValidationException& ex) { ENVOY_LOG(error, "unable to validate fraction percent runtime proto: {}", ex.what()); return; @@ -571,10 +571,10 @@ SnapshotImplPtr LoaderImpl::createNewSnapshot() { path += "/" + service_cluster_; } if (api_.fileSystem().directoryExists(path)) { - envoy_try { + TRY { layers.emplace_back(std::make_unique(layer.name(), path, api_)); ++disk_layers; - } + } END_TRY catch (EnvoyException& e) { // TODO(htuch): Consider latching here, rather than ignoring the // layer. This would be consistent with filesystem RTDS. diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 30debb4eaf93..680902ea89a9 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -48,7 +48,7 @@ void SdsApi::resolveDataSource(const FileContentMap& files, void SdsApi::onWatchUpdate() { // Filesystem reads and update callbacks can fail if the key material is missing or bad. We're not // under an onConfigUpdate() context, so we need to catch these cases explicitly here. - envoy_try { + TRY { // Obtain a stable set of files. If a rotation happens while we're reading, // then we need to try again. uint64_t prev_hash = 0; @@ -72,7 +72,7 @@ void SdsApi::onWatchUpdate() { update_callback_manager_.runCallbacks(); files_hash_ = new_hash; } - } + } END_TRY catch (const EnvoyException& e) { ENVOY_LOG_MISC(warn, fmt::format("Failed to reload certificates: {}", e.what())); sds_api_stats_.key_rotation_failed_.inc(); diff --git a/source/common/upstream/cds_api_impl.cc b/source/common/upstream/cds_api_impl.cc index e26aea3240e7..1e7f7759c48f 100644 --- a/source/common/upstream/cds_api_impl.cc +++ b/source/common/upstream/cds_api_impl.cc @@ -77,7 +77,7 @@ void CdsApiImpl::onConfigUpdate(const std::vector& a bool any_applied = false; for (const auto& resource : added_resources) { envoy::config::cluster::v3::Cluster cluster; - envoy_try { + TRY { cluster = dynamic_cast(resource.get().resource()); if (!cluster_names.insert(cluster.name()).second) { // NOTE: at this point, the first of these duplicates has already been successfully applied. @@ -89,7 +89,7 @@ void CdsApiImpl::onConfigUpdate(const std::vector& a } else { ENVOY_LOG(debug, "cds: add/update cluster '{}' skipped", cluster.name()); } - } + } END_TRY catch (const EnvoyException& e) { exception_msgs.push_back(fmt::format("{}: {}", cluster.name(), e.what())); } diff --git a/source/common/upstream/health_discovery_service.cc b/source/common/upstream/health_discovery_service.cc index b9887cfdc000..d1daabcb261f 100644 --- a/source/common/upstream/health_discovery_service.cc +++ b/source/common/upstream/health_discovery_service.cc @@ -293,7 +293,7 @@ void HdsDelegate::onReceiveMessage( } // Validate message fields - envoy_try { MessageUtil::validate(*message, validation_visitor_); } + TRY { MessageUtil::validate(*message, validation_visitor_); } END_TRY catch (const ProtoValidationException& ex) { // Increment error count stats_.errors_.inc(); diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 2013c16659cc..ddd43733a36a 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -1131,7 +1131,7 @@ void ClusterImplBase::reloadHealthyHostsHelper(const HostSharedPtr&) { const Network::Address::InstanceConstSharedPtr ClusterImplBase::resolveProtoAddress(const envoy::config::core::v3::Address& address) { - envoy_try { return Network::Address::resolveProtoAddress(address); } + TRY { return Network::Address::resolveProtoAddress(address); } END_TRY catch (EnvoyException& e) { if (info_->type() == envoy::config::cluster::v3::Cluster::STATIC || info_->type() == envoy::config::cluster::v3::Cluster::EDS) { diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc index 3b483741d403..b653e733f509 100644 --- a/source/exe/main_common.cc +++ b/source/exe/main_common.cc @@ -116,10 +116,10 @@ void MainCommonBase::configureHotRestarter(Random::RandomGenerator& random_gener // HotRestartImpl is going to multiply this value by 10, so leave head room. base_id = static_cast(random_generator.random()) & 0x0FFFFFFF; - envoy_try { + TRY { restarter = std::make_unique(base_id, 0, options_.socketPath(), options_.socketMode()); - } + } END_TRY catch (Server::HotRestartDomainSocketInUseException& ex) { // No luck, try again. ENVOY_LOG_MISC(debug, "dynamic base id: {}", ex.what()); @@ -214,13 +214,13 @@ int MainCommon::main(int argc, char** argv, PostServerHook hook) { // Initialize the server's main context under a try/catch loop and simply return EXIT_FAILURE // as needed. Whatever code in the initialization path that fails is expected to log an error // message so the user can diagnose. - envoy_try { + TRY { main_common = std::make_unique(argc, argv); Envoy::Server::Instance* server = main_common->server(); if (server != nullptr && hook != nullptr) { hook(*server); } - } + } END_TRY catch (const Envoy::NoServingException& e) { return EXIT_SUCCESS; } diff --git a/source/extensions/grpc_credentials/file_based_metadata/config.cc b/source/extensions/grpc_credentials/file_based_metadata/config.cc index e736cc67c923..8783adc55e5b 100644 --- a/source/extensions/grpc_credentials/file_based_metadata/config.cc +++ b/source/extensions/grpc_credentials/file_based_metadata/config.cc @@ -69,10 +69,10 @@ FileBasedMetadataAuthenticator::GetMetadata(grpc::string_ref, grpc::string_ref, if (!config_.header_key().empty()) { header_key = config_.header_key(); } - envoy_try { + TRY { std::string header_value = Envoy::Config::DataSource::read(config_.secret_data(), true, api_); metadata->insert(std::make_pair(header_key, header_prefix + header_value)); - } + } END_TRY catch (const EnvoyException& e) { return grpc::Status(grpc::StatusCode::NOT_FOUND, e.what()); } diff --git a/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc b/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc index 7bb06f2d5b42..e852e0487a7d 100644 --- a/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc +++ b/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc @@ -26,7 +26,7 @@ void InjectedResourceMonitor::onFileChanged() { file_changed_ = true; } void InjectedResourceMonitor::updateResourceUsage(Server::ResourceMonitor::Callbacks& callbacks) { if (file_changed_) { file_changed_ = false; - envoy_try { + TRY { const std::string contents = api_.fileSystem().fileReadToEnd(filename_); double pressure; if (absl::SimpleAtod(contents, &pressure)) { @@ -38,7 +38,7 @@ void InjectedResourceMonitor::updateResourceUsage(Server::ResourceMonitor::Callb } else { throw EnvoyException("failed to parse injected resource pressure"); } - } + } END_TRY catch (const EnvoyException& error) { error_ = error; pressure_.reset(); diff --git a/source/server/admin/runtime_handler.cc b/source/server/admin/runtime_handler.cc index 2e8b62730e8b..037176c9fcca 100644 --- a/source/server/admin/runtime_handler.cc +++ b/source/server/admin/runtime_handler.cc @@ -99,7 +99,7 @@ Http::Code RuntimeHandler::handlerRuntimeModify(absl::string_view url, Http::Res } absl::node_hash_map overrides; overrides.insert(params.begin(), params.end()); - envoy_try { server_.runtime().mergeValues(overrides); } + TRY { server_.runtime().mergeValues(overrides); } END_TRY catch (const EnvoyException& e) { response.add(e.what()); return Http::Code::ServiceUnavailable; diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index 30dce8e48d15..85ad5d6ee72e 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -24,14 +24,14 @@ bool validateConfig(const Options& options, Thread::MutexBasicLockable access_log_lock; Stats::IsolatedStoreImpl stats_store; - envoy_try { + TRY { Event::RealTimeSystem time_system; ValidationInstance server(options, time_system, local_address, stats_store, access_log_lock, component_factory, thread_factory, file_system); std::cout << "configuration '" << options.configPath() << "' OK" << std::endl; server.shutdown(); return true; - } + } END_TRY catch (const EnvoyException& e) { return false; } @@ -54,7 +54,7 @@ ValidationInstance::ValidationInstance( mutex_tracer_(nullptr), grpc_context_(stats_store_.symbolTable()), http_context_(stats_store_.symbolTable()), router_context_(stats_store_.symbolTable()), time_system_(time_system), server_contexts_(*this) { - envoy_try { initialize(options, local_address, component_factory); } + TRY { initialize(options, local_address, component_factory); } END_TRY catch (const EnvoyException& e) { ENVOY_LOG(critical, "error initializing configuration '{}': {}", options.configPath(), e.what()); diff --git a/source/server/lds_api.cc b/source/server/lds_api.cc index 6ae69b88f606..a185542ac677 100644 --- a/source/server/lds_api.cc +++ b/source/server/lds_api.cc @@ -66,7 +66,7 @@ void LdsApiImpl::onConfigUpdate(const std::vector& a std::string message; for (const auto& resource : added_resources) { envoy::config::listener::v3::Listener listener; - envoy_try { + TRY { listener = dynamic_cast(resource.get().resource()); if (!listener_names.insert(listener.name()).second) { @@ -80,7 +80,7 @@ void LdsApiImpl::onConfigUpdate(const std::vector& a } else { ENVOY_LOG(debug, "lds: add/update listener '{}' skipped", listener.name()); } - } + } END_TRY catch (const EnvoyException& e) { failure_state.push_back(std::make_unique()); auto& state = failure_state.back(); diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index af597bc4dba4..e73f40e396d0 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -360,7 +360,7 @@ bool ListenerManagerImpl::addOrUpdateListener(const envoy::config::listener::v3: } auto it = error_state_tracker_.find(name); - envoy_try { return addOrUpdateListenerInternal(config, version_info, added_via_api, name); } + TRY { return addOrUpdateListenerInternal(config, version_info, added_via_api, name); } END_TRY catch (const EnvoyException& e) { if (it == error_state_tracker_.end()) { it = error_state_tracker_.emplace(name, std::make_unique()).first; diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index cff6625d9869..e28bdd3d30c0 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -158,12 +158,12 @@ OptionsImpl::OptionsImpl(std::vector args, "600", "string", cmd); cmd.setExceptionHandling(false); - envoy_try { + TRY { cmd.parse(args); count_ = cmd.getArgList().size(); - } + } END_TRY catch (TCLAP::ArgException& e) { - envoy_try { cmd.getOutput()->failure(cmd, e); } + TRY { cmd.getOutput()->failure(cmd, e); } END_TRY catch (const TCLAP::ExitException&) { // failure() has already written an informative message to stderr, so all that's left to do // is throw our own exception with the original message. diff --git a/source/server/server.cc b/source/server/server.cc index e70af8526a62..b32ea159d9e9 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -90,12 +90,12 @@ InstanceImpl::InstanceImpl( grpc_context_(store.symbolTable()), http_context_(store.symbolTable()), router_context_(store.symbolTable()), process_context_(std::move(process_context)), hooks_(hooks), server_contexts_(*this) { - envoy_try { + TRY { if (!options.logPath().empty()) { - envoy_try { + TRY { file_logger_ = std::make_unique( options.logPath(), access_log_manager_, Logger::Registry::getSink()); - } + } END_TRY catch (const EnvoyException& e) { throw EnvoyException( fmt::format("Failed to open log-file '{}'. e.what(): {}", options.logPath(), e.what())); @@ -105,7 +105,7 @@ InstanceImpl::InstanceImpl( restarter_.initialize(*dispatcher_, *this); drain_manager_ = component_factory.createDrainManager(*this); initialize(options, std::move(local_address), component_factory, hooks); - } + } END_TRY catch (const EnvoyException& e) { ENVOY_LOG(critical, "error initializing configuration '{}': {}", options.configPath(), e.what()); @@ -606,7 +606,7 @@ void InstanceImpl::onClusterManagerPrimaryInitializationComplete() { void InstanceImpl::onRuntimeReady() { // Begin initializing secondary clusters after RTDS configuration has been applied. // Initializing can throw exceptions, so catch these. - envoy_try { clusterManager().initializeSecondaryClusters(bootstrap_); } + TRY { clusterManager().initializeSecondaryClusters(bootstrap_); } END_TRY catch (const EnvoyException& e) { ENVOY_LOG(warn, "Skipping initialization of secondary cluster: {}", e.what()); shutdown(); @@ -616,7 +616,7 @@ void InstanceImpl::onRuntimeReady() { const auto& hds_config = bootstrap_.hds_config(); async_client_manager_ = std::make_unique( *config_.clusterManager(), thread_local_, time_source_, *api_, grpc_context_.statNames()); - envoy_try { + TRY { hds_delegate_ = std::make_unique( stats_store_, Config::Utility::factoryForGrpcApiConfigSource(*async_client_manager_, hds_config, @@ -627,7 +627,7 @@ void InstanceImpl::onRuntimeReady() { access_log_manager_, *config_.clusterManager(), *local_info_, *admin_, *singleton_manager_, thread_local_, messageValidationContext().dynamicValidationVisitor(), *api_, options_); - } + } END_TRY catch (const EnvoyException& e) { ENVOY_LOG(warn, "Skipping initialization of HDS cluster: {}", e.what()); shutdown(); From 36d33b9cdedb51ca9c6a68bdcd4572bfe171d226 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Sun, 28 Feb 2021 01:29:30 +0000 Subject: [PATCH 08/41] fix format Signed-off-by: chaoqin-li1123 --- source/common/common/thread.h | 10 +++----- .../common/config/delta_subscription_state.cc | 3 ++- .../config/filesystem_subscription_impl.cc | 3 ++- source/common/config/grpc_mux_impl.cc | 3 ++- .../common/config/http_subscription_impl.cc | 6 +++-- .../formatter/substitution_formatter.cc | 3 ++- source/common/http/rest_api_fetcher.cc | 3 ++- source/common/network/apple_dns_impl.cc | 3 ++- source/common/runtime/runtime_impl.cc | 6 +++-- source/common/secret/sds_api.cc | 3 ++- source/common/upstream/cds_api_impl.cc | 3 ++- .../upstream/health_discovery_service.cc | 3 ++- .../common/upstream/original_dst_cluster.cc | 23 ++++++++++--------- source/common/upstream/upstream_impl.cc | 3 ++- source/exe/main_common.cc | 6 +++-- .../file_based_metadata/config.cc | 3 ++- .../injected_resource_monitor.cc | 3 ++- source/server/admin/runtime_handler.cc | 3 ++- source/server/config_validation/server.cc | 6 +++-- source/server/lds_api.cc | 3 ++- source/server/listener_manager_impl.cc | 3 ++- source/server/options_impl.cc | 6 +++-- source/server/server.cc | 12 ++++++---- 23 files changed, 73 insertions(+), 47 deletions(-) diff --git a/source/common/common/thread.h b/source/common/common/thread.h index 359842ccbc4f..0584c5c8d934 100644 --- a/source/common/common/thread.h +++ b/source/common/common/thread.h @@ -190,13 +190,9 @@ struct MainThread { std::thread::id main_thread_id_{std::this_thread::get_id()}; }; -#define envoy_try \ - assert(Thread::MainThread::isMainThread()); \ - try - -#define TRY \ -try { \ -assert(Thread::MainThread::isMainThread()); +#define TRY \ + try { \ + assert(Thread::MainThread::isMainThread()); #define END_TRY } diff --git a/source/common/config/delta_subscription_state.cc b/source/common/config/delta_subscription_state.cc index 67b35ad1f262..f1fe7d3626f4 100644 --- a/source/common/config/delta_subscription_state.cc +++ b/source/common/config/delta_subscription_state.cc @@ -67,7 +67,8 @@ UpdateAck DeltaSubscriptionState::handleResponse( // We *always* copy the response's nonce into the next request, even if we're going to make that // request a NACK by setting error_detail. UpdateAck ack(message.nonce(), type_url_); - TRY { handleGoodResponse(message); } END_TRY + TRY { handleGoodResponse(message); } + END_TRY catch (const EnvoyException& e) { handleBadResponse(e, ack); } diff --git a/source/common/config/filesystem_subscription_impl.cc b/source/common/config/filesystem_subscription_impl.cc index b0a36adabbe8..b86f12bdc601 100644 --- a/source/common/config/filesystem_subscription_impl.cc +++ b/source/common/config/filesystem_subscription_impl.cc @@ -69,7 +69,8 @@ void FilesystemSubscriptionImpl::refresh() { stats_.update_success_.inc(); ENVOY_LOG(debug, "Filesystem config update accepted for {}: {}", path_, config_update->DebugString()); - } END_TRY + } + END_TRY catch (const ProtobufMessage::UnknownProtoFieldException& e) { configRejected(e, config_update == nullptr ? "" : config_update->DebugString()); } diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 946fad9f8950..7dd5134d4c6b 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -251,7 +251,8 @@ void GrpcMuxImpl::onDiscoveryResponse( // would do that tracking here. apiStateFor(type_url).request_.set_version_info(message->version_info()); Memory::Utils::tryShrinkHeap(); - } END_TRY + } + END_TRY catch (const EnvoyException& e) { for (auto watch : apiStateFor(type_url).watches_) { watch->callbacks_.onConfigUpdateFailed( diff --git a/source/common/config/http_subscription_impl.cc b/source/common/config/http_subscription_impl.cc index ec6f24f8e244..5264fa58f458 100644 --- a/source/common/config/http_subscription_impl.cc +++ b/source/common/config/http_subscription_impl.cc @@ -82,7 +82,8 @@ void HttpSubscriptionImpl::createRequest(Http::RequestMessage& request) { void HttpSubscriptionImpl::parseResponse(const Http::ResponseMessage& response) { disableInitFetchTimeoutTimer(); envoy::service::discovery::v3::DiscoveryResponse message; - TRY { MessageUtil::loadFromJson(response.bodyAsString(), message, validation_visitor_); } END_TRY + TRY { MessageUtil::loadFromJson(response.bodyAsString(), message, validation_visitor_); } + END_TRY catch (const EnvoyException& e) { handleFailure(Config::ConfigUpdateFailureReason::UpdateRejected, &e); return; @@ -96,7 +97,8 @@ void HttpSubscriptionImpl::parseResponse(const Http::ResponseMessage& response) stats_.version_.set(HashUtil::xxHash64(request_.version_info())); stats_.version_text_.set(request_.version_info()); stats_.update_success_.inc(); - } END_TRY + } + END_TRY catch (const EnvoyException& e) { handleFailure(Config::ConfigUpdateFailureReason::UpdateRejected, &e); } diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc index 12b034d76646..62d46f2d3c27 100644 --- a/source/common/formatter/substitution_formatter.cc +++ b/source/common/formatter/substitution_formatter.cc @@ -1287,7 +1287,8 @@ ProtobufWkt::Value FilterStateFormatter::formatValue(const Http::RequestHeaderMa } ProtobufWkt::Value val; - TRY { MessageUtil::jsonConvertValue(*proto, val); } END_TRY + TRY { MessageUtil::jsonConvertValue(*proto, val); } + END_TRY catch (EnvoyException& ex) { return unspecifiedValue(); } diff --git a/source/common/http/rest_api_fetcher.cc b/source/common/http/rest_api_fetcher.cc index 4965edc76e85..e1d41b54b048 100644 --- a/source/common/http/rest_api_fetcher.cc +++ b/source/common/http/rest_api_fetcher.cc @@ -39,7 +39,8 @@ void RestApiFetcher::onSuccess(const Http::AsyncClient::Request& request, return; } - TRY { parseResponse(*response); } END_TRY + TRY { parseResponse(*response); } + END_TRY catch (EnvoyException& e) { onFetchFailure(Config::ConfigUpdateFailureReason::UpdateRejected, &e); } diff --git a/source/common/network/apple_dns_impl.cc b/source/common/network/apple_dns_impl.cc index e4d9d4c45640..328582b69a0d 100644 --- a/source/common/network/apple_dns_impl.cc +++ b/source/common/network/apple_dns_impl.cc @@ -156,7 +156,8 @@ ActiveDnsQuery* AppleDnsResolverImpl::resolve(const std::string& dns_name, address = Utility::parseInternetAddress(dns_name); ENVOY_LOG(debug, "DNS resolver resolved ({}) to ({}) without issuing call to Apple API", dns_name, address->asString()); - } END_TRY + } + END_TRY catch (const EnvoyException& e) { // Resolution via Apple APIs ENVOY_LOG(trace, "DNS resolver local resolution failed with: {}", e.what()); diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index 34d25fdbee11..f6da5f0938d8 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -241,7 +241,8 @@ void SnapshotImpl::parseEntryFractionalPercentValue(Entry& entry) { TRY { MessageUtil::loadFromYamlAndValidate(entry.raw_string_value_, converted_fractional_percent, ProtobufMessage::getStrictValidationVisitor()); - } END_TRY + } + END_TRY catch (const ProtoValidationException& ex) { ENVOY_LOG(error, "unable to validate fraction percent runtime proto: {}", ex.what()); return; @@ -574,7 +575,8 @@ SnapshotImplPtr LoaderImpl::createNewSnapshot() { TRY { layers.emplace_back(std::make_unique(layer.name(), path, api_)); ++disk_layers; - } END_TRY + } + END_TRY catch (EnvoyException& e) { // TODO(htuch): Consider latching here, rather than ignoring the // layer. This would be consistent with filesystem RTDS. diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 680902ea89a9..8f98a0d68770 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -72,7 +72,8 @@ void SdsApi::onWatchUpdate() { update_callback_manager_.runCallbacks(); files_hash_ = new_hash; } - } END_TRY + } + END_TRY catch (const EnvoyException& e) { ENVOY_LOG_MISC(warn, fmt::format("Failed to reload certificates: {}", e.what())); sds_api_stats_.key_rotation_failed_.inc(); diff --git a/source/common/upstream/cds_api_impl.cc b/source/common/upstream/cds_api_impl.cc index 1e7f7759c48f..56934e7e784e 100644 --- a/source/common/upstream/cds_api_impl.cc +++ b/source/common/upstream/cds_api_impl.cc @@ -89,7 +89,8 @@ void CdsApiImpl::onConfigUpdate(const std::vector& a } else { ENVOY_LOG(debug, "cds: add/update cluster '{}' skipped", cluster.name()); } - } END_TRY + } + END_TRY catch (const EnvoyException& e) { exception_msgs.push_back(fmt::format("{}: {}", cluster.name(), e.what())); } diff --git a/source/common/upstream/health_discovery_service.cc b/source/common/upstream/health_discovery_service.cc index d1daabcb261f..3a6e7fc21b14 100644 --- a/source/common/upstream/health_discovery_service.cc +++ b/source/common/upstream/health_discovery_service.cc @@ -293,7 +293,8 @@ void HdsDelegate::onReceiveMessage( } // Validate message fields - TRY { MessageUtil::validate(*message, validation_visitor_); } END_TRY + TRY { MessageUtil::validate(*message, validation_visitor_); } + END_TRY catch (const ProtoValidationException& ex) { // Increment error count stats_.errors_.inc(); diff --git a/source/common/upstream/original_dst_cluster.cc b/source/common/upstream/original_dst_cluster.cc index 67dce1199fc0..a3c1f8cbfabc 100644 --- a/source/common/upstream/original_dst_cluster.cc +++ b/source/common/upstream/original_dst_cluster.cc @@ -89,17 +89,18 @@ OriginalDstCluster::LoadBalancer::requestOverrideHost(LoadBalancerContext* conte override_header = downstream_headers->get(Http::Headers::get().EnvoyOriginalDstHost); } if (!override_header.empty()) { - // This is an implicitly untrusted header, so per the API documentation only the first - // value is used. - const std::string request_override_host(override_header[0]->value().getStringView()); - request_host = Network::Utility::parseInternetAddressAndPortNoThrow(request_override_host, false); - if (request_host) { - ENVOY_LOG(debug, "Using request override host {}.", request_override_host); - } - else { - ENVOY_LOG(debug, "original_dst_load_balancer: invalid override header value. {}", request_override_host); - parent_->info()->stats().original_dst_host_invalid_.inc(); - } + // This is an implicitly untrusted header, so per the API documentation only the first + // value is used. + const std::string request_override_host(override_header[0]->value().getStringView()); + request_host = + Network::Utility::parseInternetAddressAndPortNoThrow(request_override_host, false); + if (request_host) { + ENVOY_LOG(debug, "Using request override host {}.", request_override_host); + } else { + ENVOY_LOG(debug, "original_dst_load_balancer: invalid override header value. {}", + request_override_host); + parent_->info()->stats().original_dst_host_invalid_.inc(); + } } return request_host; } diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index ddd43733a36a..daa4b11b71f3 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -1131,7 +1131,8 @@ void ClusterImplBase::reloadHealthyHostsHelper(const HostSharedPtr&) { const Network::Address::InstanceConstSharedPtr ClusterImplBase::resolveProtoAddress(const envoy::config::core::v3::Address& address) { - TRY { return Network::Address::resolveProtoAddress(address); } END_TRY + TRY { return Network::Address::resolveProtoAddress(address); } + END_TRY catch (EnvoyException& e) { if (info_->type() == envoy::config::cluster::v3::Cluster::STATIC || info_->type() == envoy::config::cluster::v3::Cluster::EDS) { diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc index b653e733f509..fc2999a62b1a 100644 --- a/source/exe/main_common.cc +++ b/source/exe/main_common.cc @@ -119,7 +119,8 @@ void MainCommonBase::configureHotRestarter(Random::RandomGenerator& random_gener TRY { restarter = std::make_unique(base_id, 0, options_.socketPath(), options_.socketMode()); - } END_TRY + } + END_TRY catch (Server::HotRestartDomainSocketInUseException& ex) { // No luck, try again. ENVOY_LOG_MISC(debug, "dynamic base id: {}", ex.what()); @@ -220,7 +221,8 @@ int MainCommon::main(int argc, char** argv, PostServerHook hook) { if (server != nullptr && hook != nullptr) { hook(*server); } - } END_TRY + } + END_TRY catch (const Envoy::NoServingException& e) { return EXIT_SUCCESS; } diff --git a/source/extensions/grpc_credentials/file_based_metadata/config.cc b/source/extensions/grpc_credentials/file_based_metadata/config.cc index 8783adc55e5b..da4809ec9b98 100644 --- a/source/extensions/grpc_credentials/file_based_metadata/config.cc +++ b/source/extensions/grpc_credentials/file_based_metadata/config.cc @@ -72,7 +72,8 @@ FileBasedMetadataAuthenticator::GetMetadata(grpc::string_ref, grpc::string_ref, TRY { std::string header_value = Envoy::Config::DataSource::read(config_.secret_data(), true, api_); metadata->insert(std::make_pair(header_key, header_prefix + header_value)); - } END_TRY + } + END_TRY catch (const EnvoyException& e) { return grpc::Status(grpc::StatusCode::NOT_FOUND, e.what()); } diff --git a/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc b/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc index e852e0487a7d..84fe3b1aef97 100644 --- a/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc +++ b/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc @@ -38,7 +38,8 @@ void InjectedResourceMonitor::updateResourceUsage(Server::ResourceMonitor::Callb } else { throw EnvoyException("failed to parse injected resource pressure"); } - } END_TRY + } + END_TRY catch (const EnvoyException& error) { error_ = error; pressure_.reset(); diff --git a/source/server/admin/runtime_handler.cc b/source/server/admin/runtime_handler.cc index 037176c9fcca..809f38e0d378 100644 --- a/source/server/admin/runtime_handler.cc +++ b/source/server/admin/runtime_handler.cc @@ -99,7 +99,8 @@ Http::Code RuntimeHandler::handlerRuntimeModify(absl::string_view url, Http::Res } absl::node_hash_map overrides; overrides.insert(params.begin(), params.end()); - TRY { server_.runtime().mergeValues(overrides); } END_TRY + TRY { server_.runtime().mergeValues(overrides); } + END_TRY catch (const EnvoyException& e) { response.add(e.what()); return Http::Code::ServiceUnavailable; diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index 85ad5d6ee72e..f06796ab7b26 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -31,7 +31,8 @@ bool validateConfig(const Options& options, std::cout << "configuration '" << options.configPath() << "' OK" << std::endl; server.shutdown(); return true; - } END_TRY + } + END_TRY catch (const EnvoyException& e) { return false; } @@ -54,7 +55,8 @@ ValidationInstance::ValidationInstance( mutex_tracer_(nullptr), grpc_context_(stats_store_.symbolTable()), http_context_(stats_store_.symbolTable()), router_context_(stats_store_.symbolTable()), time_system_(time_system), server_contexts_(*this) { - TRY { initialize(options, local_address, component_factory); } END_TRY + TRY { initialize(options, local_address, component_factory); } + END_TRY catch (const EnvoyException& e) { ENVOY_LOG(critical, "error initializing configuration '{}': {}", options.configPath(), e.what()); diff --git a/source/server/lds_api.cc b/source/server/lds_api.cc index a185542ac677..56c9ad0593f8 100644 --- a/source/server/lds_api.cc +++ b/source/server/lds_api.cc @@ -80,7 +80,8 @@ void LdsApiImpl::onConfigUpdate(const std::vector& a } else { ENVOY_LOG(debug, "lds: add/update listener '{}' skipped", listener.name()); } - } END_TRY + } + END_TRY catch (const EnvoyException& e) { failure_state.push_back(std::make_unique()); auto& state = failure_state.back(); diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index e73f40e396d0..2ee8bda44c51 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -360,7 +360,8 @@ bool ListenerManagerImpl::addOrUpdateListener(const envoy::config::listener::v3: } auto it = error_state_tracker_.find(name); - TRY { return addOrUpdateListenerInternal(config, version_info, added_via_api, name); } END_TRY + TRY { return addOrUpdateListenerInternal(config, version_info, added_via_api, name); } + END_TRY catch (const EnvoyException& e) { if (it == error_state_tracker_.end()) { it = error_state_tracker_.emplace(name, std::make_unique()).first; diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index e28bdd3d30c0..e9a8f638d215 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -161,9 +161,11 @@ OptionsImpl::OptionsImpl(std::vector args, TRY { cmd.parse(args); count_ = cmd.getArgList().size(); - } END_TRY + } + END_TRY catch (TCLAP::ArgException& e) { - TRY { cmd.getOutput()->failure(cmd, e); } END_TRY + TRY { cmd.getOutput()->failure(cmd, e); } + END_TRY catch (const TCLAP::ExitException&) { // failure() has already written an informative message to stderr, so all that's left to do // is throw our own exception with the original message. diff --git a/source/server/server.cc b/source/server/server.cc index b32ea159d9e9..07517b7f079f 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -95,7 +95,8 @@ InstanceImpl::InstanceImpl( TRY { file_logger_ = std::make_unique( options.logPath(), access_log_manager_, Logger::Registry::getSink()); - } END_TRY + } + END_TRY catch (const EnvoyException& e) { throw EnvoyException( fmt::format("Failed to open log-file '{}'. e.what(): {}", options.logPath(), e.what())); @@ -105,7 +106,8 @@ InstanceImpl::InstanceImpl( restarter_.initialize(*dispatcher_, *this); drain_manager_ = component_factory.createDrainManager(*this); initialize(options, std::move(local_address), component_factory, hooks); - } END_TRY + } + END_TRY catch (const EnvoyException& e) { ENVOY_LOG(critical, "error initializing configuration '{}': {}", options.configPath(), e.what()); @@ -606,7 +608,8 @@ void InstanceImpl::onClusterManagerPrimaryInitializationComplete() { void InstanceImpl::onRuntimeReady() { // Begin initializing secondary clusters after RTDS configuration has been applied. // Initializing can throw exceptions, so catch these. - TRY { clusterManager().initializeSecondaryClusters(bootstrap_); } END_TRY + TRY { clusterManager().initializeSecondaryClusters(bootstrap_); } + END_TRY catch (const EnvoyException& e) { ENVOY_LOG(warn, "Skipping initialization of secondary cluster: {}", e.what()); shutdown(); @@ -627,7 +630,8 @@ void InstanceImpl::onRuntimeReady() { access_log_manager_, *config_.clusterManager(), *local_info_, *admin_, *singleton_manager_, thread_local_, messageValidationContext().dynamicValidationVisitor(), *api_, options_); - } END_TRY + } + END_TRY catch (const EnvoyException& e) { ENVOY_LOG(warn, "Skipping initialization of HDS cluster: {}", e.what()); shutdown(); From 2c62c2f2c1d0e69b702ad00fbe5a0a9874f59f4d Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Mon, 1 Mar 2021 22:08:56 +0000 Subject: [PATCH 09/41] ASSERT Signed-off-by: chaoqin-li1123 --- source/common/common/thread.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/common/thread.h b/source/common/common/thread.h index 0584c5c8d934..116cddf84c7b 100644 --- a/source/common/common/thread.h +++ b/source/common/common/thread.h @@ -192,7 +192,7 @@ struct MainThread { #define TRY \ try { \ - assert(Thread::MainThread::isMainThread()); + ASSERT(Thread::MainThread::isMainThread()); #define END_TRY } From e5aae2276c02a03ab08cd2aceafb52b7d4291b7c Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Mon, 1 Mar 2021 22:38:38 +0000 Subject: [PATCH 10/41] change macros name Signed-off-by: chaoqin-li1123 --- source/common/common/thread.h | 2 +- source/common/config/delta_subscription_state.cc | 2 +- source/common/config/filesystem_subscription_impl.cc | 2 +- source/common/config/grpc_mux_impl.cc | 2 +- source/common/config/http_subscription_impl.cc | 4 ++-- source/common/formatter/substitution_formatter.cc | 2 +- source/common/http/rest_api_fetcher.cc | 2 +- source/common/network/apple_dns_impl.cc | 2 +- source/common/runtime/runtime_impl.cc | 4 ++-- source/common/secret/sds_api.cc | 2 +- source/common/upstream/cds_api_impl.cc | 2 +- source/common/upstream/health_discovery_service.cc | 2 +- source/common/upstream/upstream_impl.cc | 2 +- source/exe/main_common.cc | 4 ++-- .../grpc_credentials/file_based_metadata/config.cc | 2 +- .../injected_resource/injected_resource_monitor.cc | 2 +- source/server/admin/runtime_handler.cc | 2 +- source/server/config_validation/server.cc | 4 ++-- source/server/lds_api.cc | 2 +- source/server/listener_manager_impl.cc | 2 +- source/server/options_impl.cc | 4 ++-- source/server/server.cc | 8 ++++---- 22 files changed, 30 insertions(+), 30 deletions(-) diff --git a/source/common/common/thread.h b/source/common/common/thread.h index 116cddf84c7b..5326bac1881d 100644 --- a/source/common/common/thread.h +++ b/source/common/common/thread.h @@ -190,7 +190,7 @@ struct MainThread { std::thread::id main_thread_id_{std::this_thread::get_id()}; }; -#define TRY \ +#define TRY_ASSERT_MAIN_THREAD \ try { \ ASSERT(Thread::MainThread::isMainThread()); diff --git a/source/common/config/delta_subscription_state.cc b/source/common/config/delta_subscription_state.cc index f1fe7d3626f4..ea9bc092e92f 100644 --- a/source/common/config/delta_subscription_state.cc +++ b/source/common/config/delta_subscription_state.cc @@ -67,7 +67,7 @@ UpdateAck DeltaSubscriptionState::handleResponse( // We *always* copy the response's nonce into the next request, even if we're going to make that // request a NACK by setting error_detail. UpdateAck ack(message.nonce(), type_url_); - TRY { handleGoodResponse(message); } + TRY_ASSERT_MAIN_THREAD { handleGoodResponse(message); } END_TRY catch (const EnvoyException& e) { handleBadResponse(e, ack); diff --git a/source/common/config/filesystem_subscription_impl.cc b/source/common/config/filesystem_subscription_impl.cc index b86f12bdc601..5e27d1e8d937 100644 --- a/source/common/config/filesystem_subscription_impl.cc +++ b/source/common/config/filesystem_subscription_impl.cc @@ -61,7 +61,7 @@ void FilesystemSubscriptionImpl::refresh() { ENVOY_LOG(debug, "Filesystem config refresh for {}", path_); stats_.update_attempt_.inc(); ProtobufTypes::MessagePtr config_update; - TRY { + TRY_ASSERT_MAIN_THREAD { const std::string version = refreshInternal(&config_update); stats_.update_time_.set(DateUtil::nowToMilliseconds(api_.timeSource())); stats_.version_.set(HashUtil::xxHash64(version)); diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 7dd5134d4c6b..e2792f57a763 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -186,7 +186,7 @@ void GrpcMuxImpl::onDiscoveryResponse( // the delta state. The proper fix for this is to converge these implementations, // see https://github.com/envoyproxy/envoy/issues/11477. same_type_resume = pause(type_url); - TRY { + TRY_ASSERT_MAIN_THREAD { // To avoid O(n^2) explosion (e.g. when we have 1000s of EDS watches), we // build a map here from resource name to resource and then walk watches_. // We have to walk all watches (and need an efficient map as a result) to diff --git a/source/common/config/http_subscription_impl.cc b/source/common/config/http_subscription_impl.cc index 5264fa58f458..e8b969f5f629 100644 --- a/source/common/config/http_subscription_impl.cc +++ b/source/common/config/http_subscription_impl.cc @@ -82,13 +82,13 @@ void HttpSubscriptionImpl::createRequest(Http::RequestMessage& request) { void HttpSubscriptionImpl::parseResponse(const Http::ResponseMessage& response) { disableInitFetchTimeoutTimer(); envoy::service::discovery::v3::DiscoveryResponse message; - TRY { MessageUtil::loadFromJson(response.bodyAsString(), message, validation_visitor_); } + TRY_ASSERT_MAIN_THREAD { MessageUtil::loadFromJson(response.bodyAsString(), message, validation_visitor_); } END_TRY catch (const EnvoyException& e) { handleFailure(Config::ConfigUpdateFailureReason::UpdateRejected, &e); return; } - TRY { + TRY_ASSERT_MAIN_THREAD { const auto decoded_resources = DecodedResourcesWrapper(resource_decoder_, message.resources(), message.version_info()); callbacks_.onConfigUpdate(decoded_resources.refvec_, message.version_info()); diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc index 62d46f2d3c27..2c8d4b177f87 100644 --- a/source/common/formatter/substitution_formatter.cc +++ b/source/common/formatter/substitution_formatter.cc @@ -1287,7 +1287,7 @@ ProtobufWkt::Value FilterStateFormatter::formatValue(const Http::RequestHeaderMa } ProtobufWkt::Value val; - TRY { MessageUtil::jsonConvertValue(*proto, val); } + TRY_ASSERT_MAIN_THREAD { MessageUtil::jsonConvertValue(*proto, val); } END_TRY catch (EnvoyException& ex) { return unspecifiedValue(); diff --git a/source/common/http/rest_api_fetcher.cc b/source/common/http/rest_api_fetcher.cc index e1d41b54b048..af154ef89caf 100644 --- a/source/common/http/rest_api_fetcher.cc +++ b/source/common/http/rest_api_fetcher.cc @@ -39,7 +39,7 @@ void RestApiFetcher::onSuccess(const Http::AsyncClient::Request& request, return; } - TRY { parseResponse(*response); } + TRY_ASSERT_MAIN_THREAD { parseResponse(*response); } END_TRY catch (EnvoyException& e) { onFetchFailure(Config::ConfigUpdateFailureReason::UpdateRejected, &e); diff --git a/source/common/network/apple_dns_impl.cc b/source/common/network/apple_dns_impl.cc index 328582b69a0d..8e8afff94fcf 100644 --- a/source/common/network/apple_dns_impl.cc +++ b/source/common/network/apple_dns_impl.cc @@ -148,7 +148,7 @@ ActiveDnsQuery* AppleDnsResolverImpl::resolve(const std::string& dns_name, ENVOY_LOG(debug, "DNS resolver resolve={}", dns_name); Address::InstanceConstSharedPtr address{}; - TRY { + TRY_ASSERT_MAIN_THREAD { // When an IP address is submitted to c-ares in DnsResolverImpl, c-ares synchronously returns // the IP without submitting a DNS query. Because Envoy has come to rely on this behavior, this // resolver implements a similar resolution path to avoid making improper DNS queries for diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index f6da5f0938d8..e0afd5ef6a19 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -238,7 +238,7 @@ bool SnapshotImpl::parseEntryDoubleValue(Entry& entry) { void SnapshotImpl::parseEntryFractionalPercentValue(Entry& entry) { envoy::type::v3::FractionalPercent converted_fractional_percent; - TRY { + TRY_ASSERT_MAIN_THREAD { MessageUtil::loadFromYamlAndValidate(entry.raw_string_value_, converted_fractional_percent, ProtobufMessage::getStrictValidationVisitor()); } @@ -572,7 +572,7 @@ SnapshotImplPtr LoaderImpl::createNewSnapshot() { path += "/" + service_cluster_; } if (api_.fileSystem().directoryExists(path)) { - TRY { + TRY_ASSERT_MAIN_THREAD { layers.emplace_back(std::make_unique(layer.name(), path, api_)); ++disk_layers; } diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 8f98a0d68770..a295340e685c 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -48,7 +48,7 @@ void SdsApi::resolveDataSource(const FileContentMap& files, void SdsApi::onWatchUpdate() { // Filesystem reads and update callbacks can fail if the key material is missing or bad. We're not // under an onConfigUpdate() context, so we need to catch these cases explicitly here. - TRY { + TRY_ASSERT_MAIN_THREAD { // Obtain a stable set of files. If a rotation happens while we're reading, // then we need to try again. uint64_t prev_hash = 0; diff --git a/source/common/upstream/cds_api_impl.cc b/source/common/upstream/cds_api_impl.cc index 56934e7e784e..66a26de99a65 100644 --- a/source/common/upstream/cds_api_impl.cc +++ b/source/common/upstream/cds_api_impl.cc @@ -77,7 +77,7 @@ void CdsApiImpl::onConfigUpdate(const std::vector& a bool any_applied = false; for (const auto& resource : added_resources) { envoy::config::cluster::v3::Cluster cluster; - TRY { + TRY_ASSERT_MAIN_THREAD { cluster = dynamic_cast(resource.get().resource()); if (!cluster_names.insert(cluster.name()).second) { // NOTE: at this point, the first of these duplicates has already been successfully applied. diff --git a/source/common/upstream/health_discovery_service.cc b/source/common/upstream/health_discovery_service.cc index 3a6e7fc21b14..01a171c3ef1d 100644 --- a/source/common/upstream/health_discovery_service.cc +++ b/source/common/upstream/health_discovery_service.cc @@ -293,7 +293,7 @@ void HdsDelegate::onReceiveMessage( } // Validate message fields - TRY { MessageUtil::validate(*message, validation_visitor_); } + TRY_ASSERT_MAIN_THREAD { MessageUtil::validate(*message, validation_visitor_); } END_TRY catch (const ProtoValidationException& ex) { // Increment error count diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index daa4b11b71f3..ecf9ce182746 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -1131,7 +1131,7 @@ void ClusterImplBase::reloadHealthyHostsHelper(const HostSharedPtr&) { const Network::Address::InstanceConstSharedPtr ClusterImplBase::resolveProtoAddress(const envoy::config::core::v3::Address& address) { - TRY { return Network::Address::resolveProtoAddress(address); } + TRY_ASSERT_MAIN_THREAD { return Network::Address::resolveProtoAddress(address); } END_TRY catch (EnvoyException& e) { if (info_->type() == envoy::config::cluster::v3::Cluster::STATIC || diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc index fc2999a62b1a..720b1c9d77dc 100644 --- a/source/exe/main_common.cc +++ b/source/exe/main_common.cc @@ -116,7 +116,7 @@ void MainCommonBase::configureHotRestarter(Random::RandomGenerator& random_gener // HotRestartImpl is going to multiply this value by 10, so leave head room. base_id = static_cast(random_generator.random()) & 0x0FFFFFFF; - TRY { + TRY_ASSERT_MAIN_THREAD { restarter = std::make_unique(base_id, 0, options_.socketPath(), options_.socketMode()); } @@ -215,7 +215,7 @@ int MainCommon::main(int argc, char** argv, PostServerHook hook) { // Initialize the server's main context under a try/catch loop and simply return EXIT_FAILURE // as needed. Whatever code in the initialization path that fails is expected to log an error // message so the user can diagnose. - TRY { + TRY_ASSERT_MAIN_THREAD { main_common = std::make_unique(argc, argv); Envoy::Server::Instance* server = main_common->server(); if (server != nullptr && hook != nullptr) { diff --git a/source/extensions/grpc_credentials/file_based_metadata/config.cc b/source/extensions/grpc_credentials/file_based_metadata/config.cc index da4809ec9b98..124961d1d57a 100644 --- a/source/extensions/grpc_credentials/file_based_metadata/config.cc +++ b/source/extensions/grpc_credentials/file_based_metadata/config.cc @@ -69,7 +69,7 @@ FileBasedMetadataAuthenticator::GetMetadata(grpc::string_ref, grpc::string_ref, if (!config_.header_key().empty()) { header_key = config_.header_key(); } - TRY { + TRY_ASSERT_MAIN_THREAD { std::string header_value = Envoy::Config::DataSource::read(config_.secret_data(), true, api_); metadata->insert(std::make_pair(header_key, header_prefix + header_value)); } diff --git a/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc b/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc index 84fe3b1aef97..46277bff6552 100644 --- a/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc +++ b/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc @@ -26,7 +26,7 @@ void InjectedResourceMonitor::onFileChanged() { file_changed_ = true; } void InjectedResourceMonitor::updateResourceUsage(Server::ResourceMonitor::Callbacks& callbacks) { if (file_changed_) { file_changed_ = false; - TRY { + TRY_ASSERT_MAIN_THREAD { const std::string contents = api_.fileSystem().fileReadToEnd(filename_); double pressure; if (absl::SimpleAtod(contents, &pressure)) { diff --git a/source/server/admin/runtime_handler.cc b/source/server/admin/runtime_handler.cc index 809f38e0d378..a827d8a0994b 100644 --- a/source/server/admin/runtime_handler.cc +++ b/source/server/admin/runtime_handler.cc @@ -99,7 +99,7 @@ Http::Code RuntimeHandler::handlerRuntimeModify(absl::string_view url, Http::Res } absl::node_hash_map overrides; overrides.insert(params.begin(), params.end()); - TRY { server_.runtime().mergeValues(overrides); } + TRY_ASSERT_MAIN_THREAD { server_.runtime().mergeValues(overrides); } END_TRY catch (const EnvoyException& e) { response.add(e.what()); diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index f06796ab7b26..50a53567bb37 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -24,7 +24,7 @@ bool validateConfig(const Options& options, Thread::MutexBasicLockable access_log_lock; Stats::IsolatedStoreImpl stats_store; - TRY { + TRY_ASSERT_MAIN_THREAD { Event::RealTimeSystem time_system; ValidationInstance server(options, time_system, local_address, stats_store, access_log_lock, component_factory, thread_factory, file_system); @@ -55,7 +55,7 @@ ValidationInstance::ValidationInstance( mutex_tracer_(nullptr), grpc_context_(stats_store_.symbolTable()), http_context_(stats_store_.symbolTable()), router_context_(stats_store_.symbolTable()), time_system_(time_system), server_contexts_(*this) { - TRY { initialize(options, local_address, component_factory); } + TRY_ASSERT_MAIN_THREAD { initialize(options, local_address, component_factory); } END_TRY catch (const EnvoyException& e) { ENVOY_LOG(critical, "error initializing configuration '{}': {}", options.configPath(), diff --git a/source/server/lds_api.cc b/source/server/lds_api.cc index 56c9ad0593f8..46194c4bee35 100644 --- a/source/server/lds_api.cc +++ b/source/server/lds_api.cc @@ -66,7 +66,7 @@ void LdsApiImpl::onConfigUpdate(const std::vector& a std::string message; for (const auto& resource : added_resources) { envoy::config::listener::v3::Listener listener; - TRY { + TRY_ASSERT_MAIN_THREAD { listener = dynamic_cast(resource.get().resource()); if (!listener_names.insert(listener.name()).second) { diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 2ee8bda44c51..d3ce0f7e4526 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -360,7 +360,7 @@ bool ListenerManagerImpl::addOrUpdateListener(const envoy::config::listener::v3: } auto it = error_state_tracker_.find(name); - TRY { return addOrUpdateListenerInternal(config, version_info, added_via_api, name); } + TRY_ASSERT_MAIN_THREAD { return addOrUpdateListenerInternal(config, version_info, added_via_api, name); } END_TRY catch (const EnvoyException& e) { if (it == error_state_tracker_.end()) { diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index e9a8f638d215..5082b357de08 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -158,13 +158,13 @@ OptionsImpl::OptionsImpl(std::vector args, "600", "string", cmd); cmd.setExceptionHandling(false); - TRY { + TRY_ASSERT_MAIN_THREAD { cmd.parse(args); count_ = cmd.getArgList().size(); } END_TRY catch (TCLAP::ArgException& e) { - TRY { cmd.getOutput()->failure(cmd, e); } + TRY_ASSERT_MAIN_THREAD { cmd.getOutput()->failure(cmd, e); } END_TRY catch (const TCLAP::ExitException&) { // failure() has already written an informative message to stderr, so all that's left to do diff --git a/source/server/server.cc b/source/server/server.cc index 07517b7f079f..9a94831dcfa0 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -90,9 +90,9 @@ InstanceImpl::InstanceImpl( grpc_context_(store.symbolTable()), http_context_(store.symbolTable()), router_context_(store.symbolTable()), process_context_(std::move(process_context)), hooks_(hooks), server_contexts_(*this) { - TRY { + TRY_ASSERT_MAIN_THREAD { if (!options.logPath().empty()) { - TRY { + TRY_ASSERT_MAIN_THREAD { file_logger_ = std::make_unique( options.logPath(), access_log_manager_, Logger::Registry::getSink()); } @@ -608,7 +608,7 @@ void InstanceImpl::onClusterManagerPrimaryInitializationComplete() { void InstanceImpl::onRuntimeReady() { // Begin initializing secondary clusters after RTDS configuration has been applied. // Initializing can throw exceptions, so catch these. - TRY { clusterManager().initializeSecondaryClusters(bootstrap_); } + TRY_ASSERT_MAIN_THREAD { clusterManager().initializeSecondaryClusters(bootstrap_); } END_TRY catch (const EnvoyException& e) { ENVOY_LOG(warn, "Skipping initialization of secondary cluster: {}", e.what()); @@ -619,7 +619,7 @@ void InstanceImpl::onRuntimeReady() { const auto& hds_config = bootstrap_.hds_config(); async_client_manager_ = std::make_unique( *config_.clusterManager(), thread_local_, time_source_, *api_, grpc_context_.statNames()); - TRY { + TRY_ASSERT_MAIN_THREAD { hds_delegate_ = std::make_unique( stats_store_, Config::Utility::factoryForGrpcApiConfigSource(*async_client_manager_, hds_config, From 5d7fbf3d541ddb5436e463c98686af8fc81e1013 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Mon, 1 Mar 2021 22:47:05 +0000 Subject: [PATCH 11/41] change macros name Signed-off-by: chaoqin-li1123 --- source/common/common/thread.h | 2 +- source/common/config/http_subscription_impl.cc | 4 +++- source/server/listener_manager_impl.cc | 4 +++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/source/common/common/thread.h b/source/common/common/thread.h index 5326bac1881d..a483e5f993b6 100644 --- a/source/common/common/thread.h +++ b/source/common/common/thread.h @@ -190,7 +190,7 @@ struct MainThread { std::thread::id main_thread_id_{std::this_thread::get_id()}; }; -#define TRY_ASSERT_MAIN_THREAD \ +#define TRY_ASSERT_MAIN_THREAD \ try { \ ASSERT(Thread::MainThread::isMainThread()); diff --git a/source/common/config/http_subscription_impl.cc b/source/common/config/http_subscription_impl.cc index e8b969f5f629..d8ad488e108a 100644 --- a/source/common/config/http_subscription_impl.cc +++ b/source/common/config/http_subscription_impl.cc @@ -82,7 +82,9 @@ void HttpSubscriptionImpl::createRequest(Http::RequestMessage& request) { void HttpSubscriptionImpl::parseResponse(const Http::ResponseMessage& response) { disableInitFetchTimeoutTimer(); envoy::service::discovery::v3::DiscoveryResponse message; - TRY_ASSERT_MAIN_THREAD { MessageUtil::loadFromJson(response.bodyAsString(), message, validation_visitor_); } + TRY_ASSERT_MAIN_THREAD { + MessageUtil::loadFromJson(response.bodyAsString(), message, validation_visitor_); + } END_TRY catch (const EnvoyException& e) { handleFailure(Config::ConfigUpdateFailureReason::UpdateRejected, &e); diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index d3ce0f7e4526..b89a679ddfd1 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -360,7 +360,9 @@ bool ListenerManagerImpl::addOrUpdateListener(const envoy::config::listener::v3: } auto it = error_state_tracker_.find(name); - TRY_ASSERT_MAIN_THREAD { return addOrUpdateListenerInternal(config, version_info, added_via_api, name); } + TRY_ASSERT_MAIN_THREAD { + return addOrUpdateListenerInternal(config, version_info, added_via_api, name); + } END_TRY catch (const EnvoyException& e) { if (it == error_state_tracker_.end()) { From 7bbfe9e1f656f728fde5269c04f64e40dbe43f8a Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 3 Mar 2021 03:19:29 +0000 Subject: [PATCH 12/41] add format checking Signed-off-by: chaoqin-li1123 --- .../access_log/access_log_manager_impl.cc | 5 ++- source/common/common/regex.cc | 15 ++++---- source/common/common/thread.h | 4 ++ source/common/http/utility.cc | 21 +++++++---- source/common/network/dns_impl.cc | 11 +++--- .../common/network/io_socket_handle_impl.cc | 5 ++- source/common/network/utility.cc | 8 ++-- source/common/protobuf/utility.cc | 37 ++++++++++--------- source/common/router/header_formatter.cc | 5 ++- source/server/admin/utils.cc | 5 +-- source/server/worker_impl.cc | 5 ++- tools/code_format/check_format.py | 3 ++ 12 files changed, 71 insertions(+), 53 deletions(-) diff --git a/source/common/access_log/access_log_manager_impl.cc b/source/common/access_log/access_log_manager_impl.cc index 0cd104ef0f49..4f3643cd372d 100644 --- a/source/common/access_log/access_log_manager_impl.cc +++ b/source/common/access_log/access_log_manager_impl.cc @@ -147,7 +147,7 @@ void AccessLogFileImpl::flushThreadFunc() { // if we failed to open file before, then simply ignore if (file_->isOpen()) { - try { + TRY_NEEDS_AUDIT { if (reopen_file_) { reopen_file_ = false; const Api::IoCallBoolResult result = file_->close(); @@ -157,7 +157,8 @@ void AccessLogFileImpl::flushThreadFunc() { } doWrite(about_to_write_buffer_); - } catch (const EnvoyException&) { + } + catch (const EnvoyException&) { stats_.reopen_failed_.inc(); } } diff --git a/source/common/common/regex.cc b/source/common/common/regex.cc index 735a24d25b8d..abb559146864 100644 --- a/source/common/common/regex.cc +++ b/source/common/common/regex.cc @@ -21,18 +21,18 @@ class CompiledStdMatcher : public CompiledMatcher { // CompiledMatcher bool match(absl::string_view value) const override { - try { - return std::regex_match(value.begin(), value.end(), regex_); - } catch (const std::regex_error& e) { + TRY_NEEDS_AUDIT { return std::regex_match(value.begin(), value.end(), regex_); } + catch (const std::regex_error& e) { return false; } } // CompiledMatcher std::string replaceAll(absl::string_view value, absl::string_view substitution) const override { - try { + TRY_NEEDS_AUDIT { return std::regex_replace(std::string(value), regex_, std::string(substitution)); - } catch (const std::regex_error& e) { + } + catch (const std::regex_error& e) { return std::string(value); } } @@ -136,9 +136,8 @@ std::regex Utility::parseStdRegex(const std::string& regex, std::regex::flag_typ // TODO(zuercher): In the future, PGV (https://github.com/envoyproxy/protoc-gen-validate) // annotations may allow us to remove this in favor of direct validation of regular // expressions. - try { - return std::regex(regex, flags); - } catch (const std::regex_error& e) { + TRY_NEEDS_AUDIT { return std::regex(regex, flags); } + catch (const std::regex_error& e) { throw EnvoyException(fmt::format("Invalid regex '{}': {}", regex, e.what())); } } diff --git a/source/common/common/thread.h b/source/common/common/thread.h index a483e5f993b6..7bd3693b5123 100644 --- a/source/common/common/thread.h +++ b/source/common/common/thread.h @@ -196,5 +196,9 @@ struct MainThread { #define END_TRY } +// TODO(chaoqinli-1123): Remove this macros after we have removed all the exceptions from data +// plane. +#define TRY_NEEDS_AUDIT try + } // namespace Thread } // namespace Envoy diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 86660479c683..7fabb4814593 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -39,17 +39,20 @@ namespace Utility { Http::Status exceptionToStatus(std::function dispatch, Buffer::Instance& data) { Http::Status status; - try { + TRY_NEEDS_AUDIT { status = dispatch(data); // TODO(#10878): Remove this when exception removal is complete. It is currently in migration, // so dispatch may either return an error status or throw an exception. Soon we won't need to // catch these exceptions, as all codec errors will be migrated to using error statuses that are // returned from dispatch. - } catch (FrameFloodException& e) { + } + catch (FrameFloodException& e) { status = bufferFloodError(e.what()); - } catch (CodecProtocolException& e) { + } + catch (CodecProtocolException& e) { status = codecProtocolError(e.what()); - } catch (PrematureResponseException& e) { + } + catch (PrematureResponseException& e) { status = prematureResponseError(e.what(), e.responseCode()); } return status; @@ -622,7 +625,7 @@ Utility::getLastAddressFromXFF(const Http::RequestHeaderMap& request_headers, xff_string = StringUtil::ltrim(xff_string); xff_string = StringUtil::rtrim(xff_string); - try { + TRY_NEEDS_AUDIT { // This technically requires a copy because inet_pton takes a null terminated string. In // practice, we are working with a view at the end of the owning string, and could pass the // raw pointer. @@ -630,7 +633,8 @@ Utility::getLastAddressFromXFF(const Http::RequestHeaderMap& request_headers, return { Network::Utility::parseInternetAddress(std::string(xff_string.data(), xff_string.size())), last_comma == std::string::npos && num_to_skip == 0}; - } catch (const EnvoyException&) { + } + catch (const EnvoyException&) { return {nullptr, false}; } } @@ -1031,7 +1035,7 @@ Utility::AuthorityAttributes Utility::parseAuthority(absl::string_view host) { // resolver flow. We could short-circuit the DNS resolver in this case, but the extra code to do // so is not worth it since the DNS resolver should handle it for us. bool is_ip_address = false; - try { + TRY_NEEDS_AUDIT { absl::string_view potential_ip_address = host_to_resolve; // TODO(mattklein123): Optimally we would support bracket parsing in parseInternetAddress(), // but we still need to trim the brackets to send the IPv6 address into the DNS resolver. For @@ -1045,7 +1049,8 @@ Utility::AuthorityAttributes Utility::parseAuthority(absl::string_view host) { Network::Utility::parseInternetAddress(std::string(potential_ip_address)); is_ip_address = true; host_to_resolve = potential_ip_address; - } catch (const EnvoyException&) { + } + catch (const EnvoyException&) { } return {is_ip_address, host_to_resolve, port}; diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index 590aef2048f3..7bf33b5409c8 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -168,15 +168,16 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i if (completed_) { if (!cancelled_) { - try { - callback_(resolution_status, std::move(address_list)); - } catch (const EnvoyException& e) { + TRY_NEEDS_AUDIT { callback_(resolution_status, std::move(address_list)); } + catch (const EnvoyException& e) { ENVOY_LOG(critical, "EnvoyException in c-ares callback: {}", e.what()); dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); }); - } catch (const std::exception& e) { + } + catch (const std::exception& e) { ENVOY_LOG(critical, "std::exception in c-ares callback: {}", e.what()); dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); }); - } catch (...) { + } + catch (...) { ENVOY_LOG(critical, "Unknown exception in c-ares callback"); dispatcher_.post([] { throw EnvoyException("unknown"); }); } diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index 300478e4a26e..151a3d04f0d1 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -277,7 +277,7 @@ Api::IoCallUint64Result IoSocketHandleImpl::sendmsg(const Buffer::RawSlice* slic Address::InstanceConstSharedPtr getAddressFromSockAddrOrDie(const sockaddr_storage& ss, socklen_t ss_len, os_fd_t fd) { - try { + TRY_NEEDS_AUDIT { // Set v6only to false so that mapped-v6 address can be normalize to v4 // address. Though dual stack may be disabled, it's still okay to assume the // address is from a dual stack socket. This is because mapped-v6 address @@ -287,7 +287,8 @@ Address::InstanceConstSharedPtr getAddressFromSockAddrOrDie(const sockaddr_stora // regarded as a v6 address from dual stack socket. However, this address is not going to be // used to create socket. Wrong knowledge of dual stack support won't hurt. return Address::addressFromSockAddr(ss, ss_len, /*v6only=*/false); - } catch (const EnvoyException& e) { + } + catch (const EnvoyException& e) { PANIC(fmt::format("Invalid address for fd: {}, error: {}", fd, e.what())); } } diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index faee96e38521..5702b3ff8cac 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -83,11 +83,11 @@ uint32_t portFromUrl(const std::string& url, absl::string_view scheme, throw EnvoyException(absl::StrCat("malformed url: ", url)); } - try { - return std::stoi(url.substr(colon_index + 1)); - } catch (const std::invalid_argument& e) { + TRY_NEEDS_AUDIT { return std::stoi(url.substr(colon_index + 1)); } + catch (const std::invalid_argument& e) { throw EnvoyException(e.what()); - } catch (const std::out_of_range& e) { + } + catch (const std::out_of_range& e) { throw EnvoyException(e.what()); } } diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 8c00c2fc810b..d35ca6401c1f 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -168,21 +168,21 @@ void tryWithApiBoosting(MessageXformFn f, Protobuf::Message& message) { Protobuf::DynamicMessageFactory dmf; auto earlier_message = ProtobufTypes::MessagePtr(dmf.GetPrototype(earlier_version_desc)->New()); ASSERT(earlier_message != nullptr); - try { + TRY_NEEDS_AUDIT { // Try apply f with an earlier version of the message, then upgrade the // result. f(*earlier_message, MessageVersion::EarlierVersion); // If we succeed at the earlier version, we ask the counterfactual, would this have worked at a // later version? If not, this is v2 only and we need to warn. This is a waste of CPU cycles but // we expect that JSON/YAML fragments will not be in use by any CPU limited use cases. - try { - f(message, MessageVersion::LatestVersionValidate); - } catch (EnvoyException& e) { + TRY_NEEDS_AUDIT { f(message, MessageVersion::LatestVersionValidate); } + catch (EnvoyException& e) { MessageUtil::onVersionUpgradeDeprecation(e.what()); } // Now we do the real work of upgrading. Config::VersionConverter::upgrade(*earlier_message, message); - } catch (ApiBoostRetryException&) { + } + catch (ApiBoostRetryException&) { // If we fail at the earlier version, try f at the current version of the // message. f(message, MessageVersion::LatestVersion); @@ -423,7 +423,7 @@ void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& messa // Attempt to parse the binary format. auto read_proto_binary = [&contents, &validation_visitor](Protobuf::Message& message, MessageVersion message_version) { - try { + TRY_NEEDS_AUDIT { if (message.ParseFromString(contents)) { MessageUtil::checkForUnexpectedFields( message, message_version == MessageVersion::LatestVersionValidate @@ -431,7 +431,8 @@ void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& messa : validation_visitor); } return; - } catch (EnvoyException& ex) { + } + catch (EnvoyException& ex) { if (message_version == MessageVersion::LatestVersion || message_version == MessageVersion::LatestVersionValidate) { // Failed reading the latest version - pass the same error upwards @@ -611,13 +612,14 @@ std::string MessageUtil::getYamlStringFromMessage(const Protobuf::Message& messa throw EnvoyException(json_or_error.status().ToString()); } YAML::Node node; - try { - node = YAML::Load(json_or_error.value()); - } catch (YAML::ParserException& e) { + TRY_NEEDS_AUDIT { node = YAML::Load(json_or_error.value()); } + catch (YAML::ParserException& e) { throw EnvoyException(e.what()); - } catch (YAML::BadConversion& e) { + } + catch (YAML::BadConversion& e) { throw EnvoyException(e.what()); - } catch (std::exception& e) { + } + catch (std::exception& e) { // There is a potentially wide space of exceptions thrown by the YAML parser, // and enumerating them all may be difficult. Envoy doesn't work well with // unhandled exceptions, so we capture them and record the exception name in @@ -884,13 +886,14 @@ void MessageUtil::redact(Protobuf::Message& message) { } ProtobufWkt::Value ValueUtil::loadFromYaml(const std::string& yaml) { - try { - return parseYamlNode(YAML::Load(yaml)); - } catch (YAML::ParserException& e) { + TRY_NEEDS_AUDIT { return parseYamlNode(YAML::Load(yaml)); } + catch (YAML::ParserException& e) { throw EnvoyException(e.what()); - } catch (YAML::BadConversion& e) { + } + catch (YAML::BadConversion& e) { throw EnvoyException(e.what()); - } catch (std::exception& e) { + } + catch (std::exception& e) { // There is a potentially wide space of exceptions thrown by the YAML parser, // and enumerating them all may be difficult. Envoy doesn't work well with // unhandled exceptions, so we capture them and record the exception name in diff --git a/source/common/router/header_formatter.cc b/source/common/router/header_formatter.cc index 81c3186bfb80..cfb005b0d768 100644 --- a/source/common/router/header_formatter.cc +++ b/source/common/router/header_formatter.cc @@ -76,13 +76,14 @@ parseMetadataField(absl::string_view params_str, bool upstream = true) { absl::string_view json = params_str.substr(1, params_str.size() - 2); // trim parens std::vector params; - try { + TRY_NEEDS_AUDIT { Json::ObjectSharedPtr parsed_params = Json::Factory::loadFromString(std::string(json)); for (const auto& param : parsed_params->asObjectArray()) { params.emplace_back(param->asString()); } - } catch (Json::Exception& e) { + } + catch (Json::Exception& e) { throw EnvoyException(formatUpstreamMetadataParseException(params_str, &e)); } diff --git a/source/server/admin/utils.cc b/source/server/admin/utils.cc index 4bc16f2cc515..16ff61690892 100644 --- a/source/server/admin/utils.cc +++ b/source/server/admin/utils.cc @@ -44,9 +44,8 @@ bool filterParam(Http::Utility::QueryParams params, Buffer::Instance& response, auto p = params.find("filter"); if (p != params.end()) { const std::string& pattern = p->second; - try { - regex = std::regex(pattern); - } catch (std::regex_error& error) { + TRY_NEEDS_AUDIT { regex = std::regex(pattern); } + catch (std::regex_error& error) { // Include the offending pattern in the log, but not the error message. response.add(fmt::format("Invalid regex: \"{}\"\n", error.what())); ENVOY_LOG_MISC(error, "admin: Invalid regex: \"{}\": {}", error.what(), pattern); diff --git a/source/server/worker_impl.cc b/source/server/worker_impl.cc index 95c24175321c..4ae49aa9149c 100644 --- a/source/server/worker_impl.cc +++ b/source/server/worker_impl.cc @@ -44,11 +44,12 @@ void WorkerImpl::addListener(absl::optional overridden_listener, // bind to an address, but then fail to listen() with `EADDRINUSE`. During initial startup, we // want to surface this. dispatcher_->post([this, overridden_listener, &listener, completion]() -> void { - try { + TRY_NEEDS_AUDIT { handler_->addListener(overridden_listener, listener); hooks_.onWorkerListenerAdded(); completion(true); - } catch (const Network::CreateListenerException& e) { + } + catch (const Network::CreateListenerException& e) { ENVOY_LOG(error, "failed to add listener on worker: {}", e.what()); completion(false); } diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index af6b1bd43b4b..d1ac068dc53d 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -756,6 +756,9 @@ def checkSourceLine(self, line, file_path, reportError): reportError("Don't use std::variant; use absl::variant instead") if self.tokenInLine("std::visit", line): reportError("Don't use std::visit; use absl::visit instead") + if " try {" in line and file_path.startswith("./source") and not file_path.startswith( + "./source/extensions") and not file_path == "./source/common/common/thread.h": + reportError("Don't use raw try, use TRY_ASSERT_MAIN_THREAD.") if "__attribute__((packed))" in line and file_path != "./include/envoy/common/platform.h": # __attribute__((packed)) is not supported by MSVC, we have a PACKED_STRUCT macro that # can be used instead From e600c7370e32722272b34440076d09b7324421d8 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 3 Mar 2021 16:49:17 +0000 Subject: [PATCH 13/41] sync Signed-off-by: chaoqin-li1123 --- source/common/router/route_config_update_receiver_impl.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/source/common/router/route_config_update_receiver_impl.cc b/source/common/router/route_config_update_receiver_impl.cc index 775d3aaf8ec6..8be65e247b89 100644 --- a/source/common/router/route_config_update_receiver_impl.cc +++ b/source/common/router/route_config_update_receiver_impl.cc @@ -7,6 +7,7 @@ #include "common/common/assert.h" #include "common/common/fmt.h" +#include "common/common/thread.h" #include "common/protobuf/utility.h" #include "common/router/config_impl.h" @@ -52,11 +53,12 @@ bool RouteConfigUpdateReceiverImpl::onVhdsUpdate( rebuildRouteConfig(rds_virtual_hosts_, *vhds_virtual_hosts_, *route_config_proto_); ConfigConstSharedPtr new_config; - try { + TRY_ASSERT_MAIN_THREAD { new_config = std::make_shared( *route_config_proto_, factory_context_, factory_context_.messageValidationContext().dynamicValidationVisitor(), false); - } catch (const Envoy::EnvoyException& e) { + } END_TRY + catch (const Envoy::EnvoyException& e) { // revert the changes that failed validation vhds_virtual_hosts_ = std::move(vhosts_before_this_update); route_config_proto_ = std::move(route_config_before_this_update); From e4b5d8ac1b57551c5d0a076853825c5b2f891adb Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 3 Mar 2021 23:19:32 +0000 Subject: [PATCH 14/41] fix format Signed-off-by: chaoqin-li1123 --- source/common/router/route_config_update_receiver_impl.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/common/router/route_config_update_receiver_impl.cc b/source/common/router/route_config_update_receiver_impl.cc index 8be65e247b89..f7682dd21a51 100644 --- a/source/common/router/route_config_update_receiver_impl.cc +++ b/source/common/router/route_config_update_receiver_impl.cc @@ -57,7 +57,8 @@ bool RouteConfigUpdateReceiverImpl::onVhdsUpdate( new_config = std::make_shared( *route_config_proto_, factory_context_, factory_context_.messageValidationContext().dynamicValidationVisitor(), false); - } END_TRY + } + END_TRY catch (const Envoy::EnvoyException& e) { // revert the changes that failed validation vhds_virtual_hosts_ = std::move(vhosts_before_this_update); From 92651db6fca3623d3f5c6cbb2109e743043db4c7 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Thu, 4 Mar 2021 04:01:37 +0000 Subject: [PATCH 15/41] fix text Signed-off-by: chaoqin-li1123 --- source/common/network/dns_impl.cc | 1 + tools/code_format/check_format.py | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index 7bf33b5409c8..a002c563202d 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -10,6 +10,7 @@ #include "common/common/assert.h" #include "common/common/fmt.h" +#include "common/common/thread.h" #include "common/network/address_impl.h" #include "common/network/utility.h" diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index c6bc538c37d8..135a1da79a94 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -760,7 +760,9 @@ def checkSourceLine(self, line, file_path, reportError): reportError("Don't use std::visit; use absl::visit instead") if " try {" in line and file_path.startswith("./source") and not file_path.startswith( "./source/extensions") and not file_path == "./source/common/common/thread.h": - reportError("Don't use raw try, use TRY_ASSERT_MAIN_THREAD.") + reportError( + "Don't use raw try, use TRY_ASSERT_MAIN_THREAD if on the main thread otherwise don't use exceptions." + ) if "__attribute__((packed))" in line and file_path != "./include/envoy/common/platform.h": # __attribute__((packed)) is not supported by MSVC, we have a PACKED_STRUCT macro that # can be used instead From 9aa81716df65040bb2417aae76c93b8308edd40e Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Mon, 8 Mar 2021 15:55:53 +0000 Subject: [PATCH 16/41] main thread singleton register test main thread id Signed-off-by: chaoqin-li1123 --- source/common/common/thread.h | 18 +++++++++++++++--- source/common/network/dns_impl.cc | 2 ++ source/common/protobuf/utility.cc | 6 ++++-- source/common/upstream/original_dst_cluster.cc | 2 +- source/server/admin/utils.cc | 3 ++- test/integration/base_integration_test.cc | 2 ++ .../integration/sds_static_integration_test.cc | 3 ++- 7 files changed, 28 insertions(+), 8 deletions(-) diff --git a/source/common/common/thread.h b/source/common/common/thread.h index 7bd3693b5123..ff12ed4f2cf4 100644 --- a/source/common/common/thread.h +++ b/source/common/common/thread.h @@ -171,8 +171,20 @@ class AtomicPtr : private AtomicPtrArray { struct MainThread { using MainThreadSingleton = InjectableSingleton; - bool inMainThread() const { return main_thread_id_ == std::this_thread::get_id(); } - static void init() { MainThreadSingleton::initialize(new MainThread()); } + bool inMainThread() const { + std::thread::id cur_id = std::this_thread::get_id(); + return std::find(main_thread_id_.begin(), main_thread_id_.end(), cur_id) != + main_thread_id_.end(); + } + void registerMainThread() { main_thread_id_.push_back(std::this_thread::get_id()); } + static bool initialized() { return MainThreadSingleton::getExisting() != nullptr; } + static void init() { + if (!initialized()) { + MainThreadSingleton::initialize(new MainThread()); + } else { + MainThreadSingleton::get().registerMainThread(); + } + } static void clear() { delete MainThreadSingleton::getExisting(); MainThreadSingleton::clear(); @@ -187,7 +199,7 @@ struct MainThread { } private: - std::thread::id main_thread_id_{std::this_thread::get_id()}; + std::vector main_thread_id_{std::this_thread::get_id()}; }; #define TRY_ASSERT_MAIN_THREAD \ diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index a002c563202d..9ed09a320dc7 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -1,5 +1,7 @@ #include "common/network/dns_impl.h" +#include + #include #include #include diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index d35ca6401c1f..b0b9c7b199a8 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -168,20 +168,22 @@ void tryWithApiBoosting(MessageXformFn f, Protobuf::Message& message) { Protobuf::DynamicMessageFactory dmf; auto earlier_message = ProtobufTypes::MessagePtr(dmf.GetPrototype(earlier_version_desc)->New()); ASSERT(earlier_message != nullptr); - TRY_NEEDS_AUDIT { + TRY_ASSERT_MAIN_THREAD { // Try apply f with an earlier version of the message, then upgrade the // result. f(*earlier_message, MessageVersion::EarlierVersion); // If we succeed at the earlier version, we ask the counterfactual, would this have worked at a // later version? If not, this is v2 only and we need to warn. This is a waste of CPU cycles but // we expect that JSON/YAML fragments will not be in use by any CPU limited use cases. - TRY_NEEDS_AUDIT { f(message, MessageVersion::LatestVersionValidate); } + TRY_ASSERT_MAIN_THREAD { f(message, MessageVersion::LatestVersionValidate); } + END_TRY catch (EnvoyException& e) { MessageUtil::onVersionUpgradeDeprecation(e.what()); } // Now we do the real work of upgrading. Config::VersionConverter::upgrade(*earlier_message, message); } + END_TRY catch (ApiBoostRetryException&) { // If we fail at the earlier version, try f at the current version of the // message. diff --git a/source/common/upstream/original_dst_cluster.cc b/source/common/upstream/original_dst_cluster.cc index a3c1f8cbfabc..22313edebe5d 100644 --- a/source/common/upstream/original_dst_cluster.cc +++ b/source/common/upstream/original_dst_cluster.cc @@ -94,7 +94,7 @@ OriginalDstCluster::LoadBalancer::requestOverrideHost(LoadBalancerContext* conte const std::string request_override_host(override_header[0]->value().getStringView()); request_host = Network::Utility::parseInternetAddressAndPortNoThrow(request_override_host, false); - if (request_host) { + if (request_host != nullptr) { ENVOY_LOG(debug, "Using request override host {}.", request_override_host); } else { ENVOY_LOG(debug, "original_dst_load_balancer: invalid override header value. {}", diff --git a/source/server/admin/utils.cc b/source/server/admin/utils.cc index 16ff61690892..2402afc47ec2 100644 --- a/source/server/admin/utils.cc +++ b/source/server/admin/utils.cc @@ -44,7 +44,8 @@ bool filterParam(Http::Utility::QueryParams params, Buffer::Instance& response, auto p = params.find("filter"); if (p != params.end()) { const std::string& pattern = p->second; - TRY_NEEDS_AUDIT { regex = std::regex(pattern); } + TRY_ASSERT_MAIN_THREAD { regex = std::regex(pattern); } + END_TRY catch (std::regex_error& error) { // Include the offending pattern in the log, but not the error message. response.add(fmt::format("Invalid regex: \"{}\"\n", error.what())); diff --git a/test/integration/base_integration_test.cc b/test/integration/base_integration_test.cc index a483a33f1941..9c491d57955c 100644 --- a/test/integration/base_integration_test.cc +++ b/test/integration/base_integration_test.cc @@ -17,6 +17,7 @@ #include "common/common/assert.h" #include "common/common/fmt.h" +#include "common/common/thread.h" #include "common/config/api_version.h" #include "common/event/libevent.h" #include "common/network/utility.h" @@ -93,6 +94,7 @@ Network::ClientConnectionPtr BaseIntegrationTest::makeClientConnectionWithOption } void BaseIntegrationTest::initialize() { + Thread::MainThread::init(); RELEASE_ASSERT(!initialized_, ""); RELEASE_ASSERT(Event::Libevent::Global::initialized(), ""); initialized_ = true; diff --git a/test/integration/sds_static_integration_test.cc b/test/integration/sds_static_integration_test.cc index 172e7b0ca2f6..8e0be765e98f 100644 --- a/test/integration/sds_static_integration_test.cc +++ b/test/integration/sds_static_integration_test.cc @@ -5,6 +5,7 @@ #include "envoy/extensions/transport_sockets/tls/v3/cert.pb.h" #include "envoy/stats/scope.h" +#include "common/common/thread.h" #include "common/event/dispatcher_impl.h" #include "common/network/connection_impl.h" #include "common/network/utility.h" @@ -68,7 +69,7 @@ class SdsStaticDownstreamIntegrationTest tls_certificate->mutable_private_key()->set_filename( TestEnvironment::runfilesPath("test/config/integration/certs/serverkey.pem")); }); - + ASSERT(Thread::MainThread::isMainThread()); HttpIntegrationTest::initialize(); registerTestServerPorts({"http"}); From 7554922cc8cf1c44919eaef76cd236d20e165133 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Mon, 8 Mar 2021 17:27:11 +0000 Subject: [PATCH 17/41] fix format Signed-off-by: chaoqin-li1123 --- source/exe/win32/service_base.cc | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/source/exe/win32/service_base.cc b/source/exe/win32/service_base.cc index 9304d0403e9b..1a08dabf80b2 100644 --- a/source/exe/win32/service_base.cc +++ b/source/exe/win32/service_base.cc @@ -3,6 +3,7 @@ #include "common/buffer/buffer_impl.h" #include "common/common/assert.h" +#include "common/common/thread.h" #include "common/event/signal_impl.h" #include "exe/main_common.h" @@ -65,7 +66,7 @@ DWORD ServiceBase::Start(std::vector args) { // Initialize the server's main context under a try/catch loop and simply return `EXIT_FAILURE` // as needed. Whatever code in the initialization path that fails is expected to log an error // message so the user can diagnose. - try { + TRY_ASSERT_MAIN_THREAD { main_common = std::make_shared(args); Envoy::Server::Instance* server = main_common->server(); if (!server->options().signalHandlingEnabled()) { @@ -77,11 +78,15 @@ DWORD ServiceBase::Start(std::vector args) { server->shutdown(); }); } - } catch (const Envoy::NoServingException& e) { + } + END_TRY + catch (const Envoy::NoServingException& e) { return S_OK; - } catch (const Envoy::MalformedArgvException& e) { + } + catch (const Envoy::MalformedArgvException& e) { return E_INVALIDARG; - } catch (const Envoy::EnvoyException& e) { + } + catch (const Envoy::EnvoyException& e) { ENVOY_LOG_MISC(warn, "Envoy failed to start with {}", e.what()); return E_FAIL; } From a0b5caf415d7729014387889156f0246f51aee35 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Thu, 11 Mar 2021 01:40:11 +0000 Subject: [PATCH 18/41] workaround for intialize in test thread and more comments Signed-off-by: chaoqin-li1123 --- source/common/common/thread.h | 29 +++++++++++++------ source/common/protobuf/utility.cc | 5 ++-- .../common/thread_local/thread_local_impl.h | 2 +- test/integration/base_integration_test.cc | 2 +- 4 files changed, 25 insertions(+), 13 deletions(-) diff --git a/source/common/common/thread.h b/source/common/common/thread.h index ff12ed4f2cf4..2e7f2b5fa4f2 100644 --- a/source/common/common/thread.h +++ b/source/common/common/thread.h @@ -172,18 +172,25 @@ class AtomicPtr : private AtomicPtrArray { struct MainThread { using MainThreadSingleton = InjectableSingleton; bool inMainThread() const { - std::thread::id cur_id = std::this_thread::get_id(); - return std::find(main_thread_id_.begin(), main_thread_id_.end(), cur_id) != - main_thread_id_.end(); + return main_thread_id_ == std::this_thread::get_id(); } - void registerMainThread() { main_thread_id_.push_back(std::this_thread::get_id()); } + bool inTestThread() const { + return test_thread_id_.has_value() && (test_thread_id_.value() == std::this_thread::get_id()); + } + void registerTestThread() { test_thread_id_ = std::this_thread::get_id(); } + void registerMainThread() { main_thread_id_ = std::this_thread::get_id(); } static bool initialized() { return MainThreadSingleton::getExisting() != nullptr; } - static void init() { + static void initMainThread() { + if (!initialized()) { + MainThreadSingleton::initialize(new MainThread()); + } + MainThreadSingleton::get().registerMainThread(); + } + static void initTestThread() { if (!initialized()) { MainThreadSingleton::initialize(new MainThread()); - } else { - MainThreadSingleton::get().registerMainThread(); } + MainThreadSingleton::get().registerTestThread(); } static void clear() { delete MainThreadSingleton::getExisting(); @@ -195,13 +202,17 @@ struct MainThread { return true; } // When threading is on, compare thread id with main thread id. - return MainThreadSingleton::get().inMainThread(); + return MainThreadSingleton::get().inMainThread() || MainThreadSingleton::get().inTestThread(); } private: - std::vector main_thread_id_{std::this_thread::get_id()}; + std::thread::id main_thread_id_; + absl::optional test_thread_id_{}; }; + +// To improve exception safety in data plane, we plan to forbid the use of raw try in the core code base. +// This macros uses main thread assertion to make sure that exceptions aren't thrown from worker thread. #define TRY_ASSERT_MAIN_THREAD \ try { \ ASSERT(Thread::MainThread::isMainThread()); diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index b0b9c7b199a8..a78f447973b9 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -19,6 +19,7 @@ #include "common/runtime/runtime_features.h" #include "absl/strings/match.h" +#include "source/common/common/_virtual_includes/thread_lib/common/common/thread.h" #include "udpa/annotations/sensitive.pb.h" #include "yaml-cpp/yaml.h" @@ -425,7 +426,7 @@ void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& messa // Attempt to parse the binary format. auto read_proto_binary = [&contents, &validation_visitor](Protobuf::Message& message, MessageVersion message_version) { - TRY_NEEDS_AUDIT { + TRY_ASSERT_MAIN_THREAD { if (message.ParseFromString(contents)) { MessageUtil::checkForUnexpectedFields( message, message_version == MessageVersion::LatestVersionValidate @@ -433,7 +434,7 @@ void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& messa : validation_visitor); } return; - } + } END_TRY catch (EnvoyException& ex) { if (message_version == MessageVersion::LatestVersion || message_version == MessageVersion::LatestVersionValidate) { diff --git a/source/common/thread_local/thread_local_impl.h b/source/common/thread_local/thread_local_impl.h index e098723d7232..db9cd63e1023 100644 --- a/source/common/thread_local/thread_local_impl.h +++ b/source/common/thread_local/thread_local_impl.h @@ -19,7 +19,7 @@ namespace ThreadLocal { */ class InstanceImpl : Logger::Loggable, public NonCopyable, public Instance { public: - InstanceImpl() { Thread::MainThread::init(); } + InstanceImpl() { Thread::MainThread::initMainThread(); } ~InstanceImpl() override; // ThreadLocal::Instance diff --git a/test/integration/base_integration_test.cc b/test/integration/base_integration_test.cc index 9c491d57955c..33ab7516bca3 100644 --- a/test/integration/base_integration_test.cc +++ b/test/integration/base_integration_test.cc @@ -94,7 +94,7 @@ Network::ClientConnectionPtr BaseIntegrationTest::makeClientConnectionWithOption } void BaseIntegrationTest::initialize() { - Thread::MainThread::init(); + Thread::MainThread::initTestThread(); RELEASE_ASSERT(!initialized_, ""); RELEASE_ASSERT(Event::Libevent::Global::initialized(), ""); initialized_ = true; From 34ffdd307209020e0755a15bcfaeee46db07eb25 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Thu, 11 Mar 2021 01:49:37 +0000 Subject: [PATCH 19/41] fix format Signed-off-by: chaoqin-li1123 --- source/common/common/thread.h | 10 ++++------ source/common/protobuf/utility.cc | 4 ++-- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/source/common/common/thread.h b/source/common/common/thread.h index 2e7f2b5fa4f2..fa5f45ca386d 100644 --- a/source/common/common/thread.h +++ b/source/common/common/thread.h @@ -171,9 +171,7 @@ class AtomicPtr : private AtomicPtrArray { struct MainThread { using MainThreadSingleton = InjectableSingleton; - bool inMainThread() const { - return main_thread_id_ == std::this_thread::get_id(); - } + bool inMainThread() const { return main_thread_id_ == std::this_thread::get_id(); } bool inTestThread() const { return test_thread_id_.has_value() && (test_thread_id_.value() == std::this_thread::get_id()); } @@ -210,9 +208,9 @@ struct MainThread { absl::optional test_thread_id_{}; }; - -// To improve exception safety in data plane, we plan to forbid the use of raw try in the core code base. -// This macros uses main thread assertion to make sure that exceptions aren't thrown from worker thread. +// To improve exception safety in data plane, we plan to forbid the use of raw try in the core code +// base. This macros uses main thread assertion to make sure that exceptions aren't thrown from +// worker thread. #define TRY_ASSERT_MAIN_THREAD \ try { \ ASSERT(Thread::MainThread::isMainThread()); diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index a78f447973b9..e3743b635d24 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -19,7 +19,6 @@ #include "common/runtime/runtime_features.h" #include "absl/strings/match.h" -#include "source/common/common/_virtual_includes/thread_lib/common/common/thread.h" #include "udpa/annotations/sensitive.pb.h" #include "yaml-cpp/yaml.h" @@ -434,7 +433,8 @@ void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& messa : validation_visitor); } return; - } END_TRY + } + END_TRY catch (EnvoyException& ex) { if (message_version == MessageVersion::LatestVersion || message_version == MessageVersion::LatestVersionValidate) { From 76422cf53b50e4fff32f4f4599d0c78e730991e1 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Thu, 11 Mar 2021 02:30:02 +0000 Subject: [PATCH 20/41] address some comment Signed-off-by: chaoqin-li1123 --- source/common/formatter/substitution_formatter.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc index 4117e2f6af99..3b986f2299f3 100644 --- a/source/common/formatter/substitution_formatter.cc +++ b/source/common/formatter/substitution_formatter.cc @@ -6,6 +6,7 @@ #include #include +#include "common/common/thread.h" #include "envoy/config/core/v3/base.pb.h" #include "envoy/upstream/upstream.h" @@ -1294,8 +1295,7 @@ ProtobufWkt::Value FilterStateFormatter::formatValue(const Http::RequestHeaderMa } ProtobufWkt::Value val; - TRY_ASSERT_MAIN_THREAD { MessageUtil::jsonConvertValue(*proto, val); } - END_TRY + TRY_NEEDS_AUDIT { MessageUtil::jsonConvertValue(*proto, val); } catch (EnvoyException& ex) { return unspecifiedValue(); } From 0a3efd51f1c44ff41469241b773bea01d62f00f7 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Thu, 11 Mar 2021 03:14:02 +0000 Subject: [PATCH 21/41] format Signed-off-by: chaoqin-li1123 --- source/common/formatter/substitution_formatter.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc index 3b986f2299f3..4a7acb051bf6 100644 --- a/source/common/formatter/substitution_formatter.cc +++ b/source/common/formatter/substitution_formatter.cc @@ -6,7 +6,6 @@ #include #include -#include "common/common/thread.h" #include "envoy/config/core/v3/base.pb.h" #include "envoy/upstream/upstream.h" @@ -14,6 +13,7 @@ #include "common/common/assert.h" #include "common/common/empty_string.h" #include "common/common/fmt.h" +#include "common/common/thread.h" #include "common/common/utility.h" #include "common/config/metadata.h" #include "common/grpc/common.h" From a6852eae53ae5b1b34ab68b76ac748f9cfc779cf Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 17 Mar 2021 16:55:34 +0000 Subject: [PATCH 22/41] format Signed-off-by: chaoqin-li1123 --- tools/code_format/check_format.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index 24714d2510e7..39973b38ac07 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -770,7 +770,8 @@ def checkSourceLine(self, line, file_path, reportError): reportError("Don't use std::variant; use absl::variant instead") if self.tokenInLine("std::visit", line): reportError("Don't use std::visit; use absl::visit instead") - if " try {" in line and file_path.startswith("./source") and not self.allowlistedForRawTry(file_path): + if " try {" in line and file_path.startswith( + "./source") and not self.allowlistedForRawTry(file_path): reportError( "Don't use raw try, use TRY_ASSERT_MAIN_THREAD if on the main thread otherwise don't use exceptions." ) From c2b7dbacfabafdd5ffa68111eb5a55f744cdaab2 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 17 Mar 2021 22:11:10 +0000 Subject: [PATCH 23/41] format Signed-off-by: chaoqin-li1123 --- source/common/common/BUILD | 1 + source/common/common/thread.cc | 35 +++++++++++++++++++++++++++++++ source/common/common/thread.h | 28 ++++--------------------- source/common/http/utility.cc | 22 +++++++++---------- source/common/protobuf/utility.cc | 6 ++++-- tools/code_format/check_format.py | 1 + 6 files changed, 55 insertions(+), 38 deletions(-) create mode 100644 source/common/common/thread.cc diff --git a/source/common/common/BUILD b/source/common/common/BUILD index 64ecb08d7b21..ab347df1a83d 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -326,6 +326,7 @@ envoy_cc_library( envoy_cc_library( name = "thread_lib", + srcs = ["thread.cc"], hdrs = ["thread.h"], external_deps = ["abseil_synchronization"], deps = envoy_cc_platform_dep("thread_impl_lib") + [ diff --git a/source/common/common/thread.cc b/source/common/common/thread.cc new file mode 100644 index 000000000000..e2eea34c145b --- /dev/null +++ b/source/common/common/thread.cc @@ -0,0 +1,35 @@ +#include "common/common/thread.h" + +namespace Envoy { +namespace Thread { + +bool MainThread::isMainThread() { + // If threading is off, only main thread is running. + if (MainThreadSingleton::getExisting() == nullptr) { + return true; + } + // When threading is on, compare thread id with main thread id. + return MainThreadSingleton::get().inMainThread() || MainThreadSingleton::get().inTestThread(); +} + +void MainThread::clear() { + delete MainThreadSingleton::getExisting(); + MainThreadSingleton::clear(); +} + +void MainThread::initTestThread() { + if (!initialized()) { + MainThreadSingleton::initialize(new MainThread()); + } + MainThreadSingleton::get().registerTestThread(); +} + +void MainThread::initMainThread() { + if (!initialized()) { + MainThreadSingleton::initialize(new MainThread()); + } + MainThreadSingleton::get().registerMainThread(); +} + +} // namespace Thread +} // namespace Envoy diff --git a/source/common/common/thread.h b/source/common/common/thread.h index fa5f45ca386d..d46979c48539 100644 --- a/source/common/common/thread.h +++ b/source/common/common/thread.h @@ -178,30 +178,10 @@ struct MainThread { void registerTestThread() { test_thread_id_ = std::this_thread::get_id(); } void registerMainThread() { main_thread_id_ = std::this_thread::get_id(); } static bool initialized() { return MainThreadSingleton::getExisting() != nullptr; } - static void initMainThread() { - if (!initialized()) { - MainThreadSingleton::initialize(new MainThread()); - } - MainThreadSingleton::get().registerMainThread(); - } - static void initTestThread() { - if (!initialized()) { - MainThreadSingleton::initialize(new MainThread()); - } - MainThreadSingleton::get().registerTestThread(); - } - static void clear() { - delete MainThreadSingleton::getExisting(); - MainThreadSingleton::clear(); - } - static bool isMainThread() { - // If threading is off, only main thread is running. - if (MainThreadSingleton::getExisting() == nullptr) { - return true; - } - // When threading is on, compare thread id with main thread id. - return MainThreadSingleton::get().inMainThread() || MainThreadSingleton::get().inTestThread(); - } + static void initMainThread(); + static void initTestThread(); + static void clear(); + static bool isMainThread(); private: std::thread::id main_thread_id_; diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 5f7fe01c3e0f..6883b5ec6df5 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -626,18 +626,16 @@ Utility::getLastAddressFromXFF(const Http::RequestHeaderMap& request_headers, xff_string = StringUtil::ltrim(xff_string); xff_string = StringUtil::rtrim(xff_string); - TRY_NEEDS_AUDIT { - // This technically requires a copy because inet_pton takes a null terminated string. In - // practice, we are working with a view at the end of the owning string, and could pass the - // raw pointer. - // TODO(mattklein123) PERF: Avoid the copy here. - return { - Network::Utility::parseInternetAddress(std::string(xff_string.data(), xff_string.size())), - last_comma == std::string::npos && num_to_skip == 0}; - } - catch (const EnvoyException&) { - return {nullptr, false}; - } + // This technically requires a copy because inet_pton takes a null terminated string. In + // practice, we are working with a view at the end of the owning string, and could pass the + // raw pointer. + // TODO(mattklein123) PERF: Avoid the copy here. + Network::Address::InstanceConstSharedPtr address = Network::Utility::parseInternetAddressNoThrow( + std::string(xff_string.data(), xff_string.size())); + if (address != nullptr) { + return {address, last_comma == std::string::npos && num_to_skip == 0}; + } + return {nullptr, false}; } bool Utility::sanitizeConnectionHeader(Http::RequestHeaderMap& headers) { diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 5991eaac39ed..cda179fc505c 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -615,7 +615,8 @@ std::string MessageUtil::getYamlStringFromMessage(const Protobuf::Message& messa throw EnvoyException(json_or_error.status().ToString()); } YAML::Node node; - TRY_NEEDS_AUDIT { node = YAML::Load(json_or_error.value()); } + TRY_ASSERT_MAIN_THREAD { node = YAML::Load(json_or_error.value()); } + END_TRY catch (YAML::ParserException& e) { throw EnvoyException(e.what()); } @@ -889,7 +890,8 @@ void MessageUtil::redact(Protobuf::Message& message) { } ProtobufWkt::Value ValueUtil::loadFromYaml(const std::string& yaml) { - TRY_NEEDS_AUDIT { return parseYamlNode(YAML::Load(yaml)); } + TRY_ASSERT_MAIN_THREAD { return parseYamlNode(YAML::Load(yaml)); } + END_TRY catch (YAML::ParserException& e) { throw EnvoyException(e.what()); } diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index 39973b38ac07..ec4b70950294 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -452,6 +452,7 @@ def denylistedForExceptions(self, file_path): def allowlistedForBuildUrls(self, file_path): return file_path in BUILD_URLS_ALLOWLIST + # TODO(chaoqin-li1123): Exclude some important extensions from the allow list. def allowlistedForRawTry(self, file_path): return file_path in RAW_TRY_ALLOWLIST or file_path.startswith("./source/extensions") From 4e999550d6c423bef165a0979d27863eb545e504 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 17 Mar 2021 23:00:46 +0000 Subject: [PATCH 24/41] format Signed-off-by: chaoqin-li1123 --- .../http/filter_config_discovery_impl.cc | 6 +- tools/code_format/check_format.py | 543 +++++++++--------- 2 files changed, 277 insertions(+), 272 deletions(-) diff --git a/source/common/filter/http/filter_config_discovery_impl.cc b/source/common/filter/http/filter_config_discovery_impl.cc index 0a23b991aca7..ea97e001a75a 100644 --- a/source/common/filter/http/filter_config_discovery_impl.cc +++ b/source/common/filter/http/filter_config_discovery_impl.cc @@ -3,6 +3,7 @@ #include "envoy/config/core/v3/extension.pb.validate.h" #include "envoy/server/filter_config.h" +#include "common/common/thread.h" #include "common/config/utility.h" #include "common/grpc/common.h" #include "common/protobuf/utility.h" @@ -230,10 +231,11 @@ FilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFilterConf // and the applied config eventually converges once ECDS update arrives. bool last_config_valid = false; if (subscription->lastConfig().has_value()) { - try { + TRY_ASSERT_MAIN_THREAD { provider->validateTypeUrl(subscription->lastTypeUrl()); last_config_valid = true; - } catch (const EnvoyException& e) { + } + END_TRY catch (const EnvoyException& e) { ENVOY_LOG(debug, "ECDS subscription {} is invalid in a listener context: {}.", filter_config_name, e.what()); subscription->incrementConflictCounter(); diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index f3770a1ec3a4..bac61cf90fd1 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -91,8 +91,6 @@ "request_time_ms", "upstream_cx_connect_ms", "upstream_cx_length_ms") -RAW_TRY_ALLOWLIST = ("./source/common/common/regex.cc", "./source/common/common/thread.h") - # Files in these paths can use std::regex STD_REGEX_ALLOWLIST = ( "./source/common/common/utility.cc", "./source/common/common/regex.h", @@ -117,6 +115,8 @@ EXCEPTION_DENYLIST = ("./source/common/http/http2/codec_impl.h", "./source/common/http/http2/codec_impl.cc") +RAW_TRY_ALLOWLIST = ("./source/common/common/regex.cc", "./source/common/common/thread.h") + # Header files that can throw exceptions. These should be limited; the only # valid situation identified so far is template functions used for config # processing. @@ -138,12 +138,12 @@ ) CLANG_FORMAT_PATH = os.getenv("CLANG_FORMAT", "clang-format-10") -BUILDIFIER_PATH = paths.getBuildifier() -BUILDOZER_PATH = paths.getBuildozer() +BUILDIFIER_PATH = paths.get_buildifier() +BUILDOZER_PATH = paths.get_buildozer() ENVOY_BUILD_FIXER_PATH = os.path.join(os.path.dirname(os.path.abspath(sys.argv[0])), "envoy_build_fixer.py") HEADER_ORDER_PATH = os.path.join(os.path.dirname(os.path.abspath(sys.argv[0])), "header_order.py") -SUBDIR_SET = set(common.includeDirOrder()) +SUBDIR_SET = set(common.include_dir_order()) INCLUDE_ANGLE = "#include <" INCLUDE_ANGLE_LEN = len(INCLUDE_ANGLE) PROTO_PACKAGE_REGEX = re.compile(r"^package (\S+);\n*", re.MULTILINE) @@ -276,11 +276,11 @@ def __init__(self, args): # Map a line transformation function across each line of a file, # writing the result lines as requested. # If there is a clang format nesting or mismatch error, return the first occurrence - def evaluateLines(self, path, line_xform, write=True): + def evaluate_lines(self, path, line_xform, write=True): error_message = None format_flag = True output_lines = [] - for line_number, line in enumerate(self.readLines(path)): + for line_number, line in enumerate(self.read_lines(path)): if line.find("// clang-format off") != -1: if not format_flag and error_message is None: error_message = "%s:%d: %s" % (path, line_number + 1, "clang-format nested off") @@ -302,37 +302,37 @@ def evaluateLines(self, path, line_xform, write=True): return error_message # Obtain all the lines in a given file. - def readLines(self, path): - return self.readFile(path).split('\n') + def read_lines(self, path): + return self.read_file(path).split('\n') # Read a UTF-8 encoded file as a str. - def readFile(self, path): + def read_file(self, path): return pathlib.Path(path).read_text(encoding='utf-8') - # lookPath searches for the given executable in all directories in PATH + # look_path searches for the given executable in all directories in PATH # environment variable. If it cannot be found, empty string is returned. - def lookPath(self, executable): + def look_path(self, executable): return shutil.which(executable) or '' - # pathExists checks whether the given path exists. This function assumes that + # path_exists checks whether the given path exists. This function assumes that # the path is absolute and evaluates environment variables. - def pathExists(self, executable): + def path_exists(self, executable): return os.path.exists(os.path.expandvars(executable)) - # executableByOthers checks whether the given path has execute permission for + # executable_by_others checks whether the given path has execute permission for # others. - def executableByOthers(self, executable): + def executable_by_others(self, executable): st = os.stat(os.path.expandvars(executable)) return bool(st.st_mode & stat.S_IXOTH) # Check whether all needed external tools (clang-format, buildifier, buildozer) are # available. - def checkTools(self): + def check_tools(self): error_messages = [] - clang_format_abs_path = self.lookPath(CLANG_FORMAT_PATH) + clang_format_abs_path = self.look_path(CLANG_FORMAT_PATH) if clang_format_abs_path: - if not self.executableByOthers(clang_format_abs_path): + if not self.executable_by_others(clang_format_abs_path): error_messages.append("command {} exists, but cannot be executed by other " "users".format(CLANG_FORMAT_PATH)) else: @@ -346,14 +346,14 @@ def checkTools(self): " export CLANG_FORMAT=/usr/local/opt/llvm@11/bin/clang-format".format( CLANG_FORMAT_PATH)) - def checkBazelTool(name, path, var): - bazel_tool_abs_path = self.lookPath(path) + def check_bazel_tool(name, path, var): + bazel_tool_abs_path = self.look_path(path) if bazel_tool_abs_path: - if not self.executableByOthers(bazel_tool_abs_path): + if not self.executable_by_others(bazel_tool_abs_path): error_messages.append("command {} exists, but cannot be executed by other " "users".format(path)) - elif self.pathExists(path): - if not self.executableByOthers(path): + elif self.path_exists(path): + if not self.executable_by_others(path): error_messages.append("command {} exists, but cannot be executed by other " "users".format(path)) else: @@ -366,18 +366,18 @@ def checkBazelTool(name, path, var): " go get -u github.com/bazelbuild/buildtools/{}".format( path, name, var, var, name, name, name)) - checkBazelTool('buildifier', BUILDIFIER_PATH, 'BUILDIFIER_BIN') - checkBazelTool('buildozer', BUILDOZER_PATH, 'BUILDOZER_BIN') + check_bazel_tool('buildifier', BUILDIFIER_PATH, 'BUILDIFIER_BIN') + check_bazel_tool('buildozer', BUILDOZER_PATH, 'BUILDOZER_BIN') return error_messages - def checkNamespace(self, file_path): + def check_namespace(self, file_path): for excluded_path in self.namespace_check_excluded_paths: if file_path.startswith(excluded_path): return [] nolint = "NOLINT(namespace-%s)" % self.namespace_check.lower() - text = self.readFile(file_path) + text = self.read_file(file_path) if not re.search("^\s*namespace\s+%s\s*{" % self.namespace_check, text, re.MULTILINE) and \ not nolint in text: return [ @@ -386,10 +386,10 @@ def checkNamespace(self, file_path): ] return [] - def packageNameForProto(self, file_path): + def package_name_for_proto(self, file_path): package_name = None error_message = [] - result = PROTO_PACKAGE_REGEX.search(self.readFile(file_path)) + result = PROTO_PACKAGE_REGEX.search(self.read_file(file_path)) if result is not None and len(result.groups()) == 1: package_name = result.group(1) if package_name is None: @@ -398,90 +398,86 @@ def packageNameForProto(self, file_path): return [package_name, error_message] # To avoid breaking the Lyft import, we just check for path inclusion here. - def allowlistedForProtobufDeps(self, file_path): + def allow_listed_for_protobuf_deps(self, file_path): return (file_path.endswith(PROTO_SUFFIX) or file_path.endswith(REPOSITORIES_BZL) or \ any(path_segment in file_path for path_segment in GOOGLE_PROTOBUF_ALLOWLIST)) # Real-world time sources should not be instantiated in the source, except for a few # specific cases. They should be passed down from where they are instantied to where # they need to be used, e.g. through the ServerInstance, Dispatcher, or ClusterManager. - def allowlistedForRealTime(self, file_path): + def allow_listed_for_realtime(self, file_path): if file_path.endswith(".md"): return True return file_path in REAL_TIME_ALLOWLIST - def allowlistedForRegisterFactory(self, file_path): + def allow_listed_for_register_factory(self, file_path): if not file_path.startswith("./test/"): return True return any(file_path.startswith(prefix) for prefix in REGISTER_FACTORY_TEST_ALLOWLIST) - def allowlistedForSerializeAsString(self, file_path): + def allow_listed_for_serialize_as_string(self, file_path): return file_path in SERIALIZE_AS_STRING_ALLOWLIST or file_path.endswith(DOCS_SUFFIX) - def allowlistedForJsonStringToMessage(self, file_path): + def allow_listed_for_json_string_to_message(self, file_path): return file_path in JSON_STRING_TO_MESSAGE_ALLOWLIST - def allowlistedForHistogramSiSuffix(self, name): + def allow_listed_for_histogram_si_suffix(self, name): return name in HISTOGRAM_WITH_SI_SUFFIX_ALLOWLIST - def allowlistedForStdRegex(self, file_path): + def allow_listed_for_std_regex(self, file_path): return file_path.startswith("./test") or file_path in STD_REGEX_ALLOWLIST or file_path.endswith( DOCS_SUFFIX) - def allowlistedForGrpcInit(self, file_path): + def allow_listed_for_grpc_init(self, file_path): return file_path in GRPC_INIT_ALLOWLIST - def whitelistedForMemcpy(self, file_path): - return file_path in MEMCPY_WHITELIST - - def allowlistedForUnpackTo(self, file_path): + def allow_listed_for_unpack_to(self, file_path): return file_path.startswith("./test") or file_path in [ "./source/common/protobuf/utility.cc", "./source/common/protobuf/utility.h" ] - def denylistedForExceptions(self, file_path): + def deny_listed_for_exceptions(self, file_path): # Returns true when it is a non test header file or the file_path is in DENYLIST or # it is under tools/testdata subdirectory. if file_path.endswith(DOCS_SUFFIX): return False return (file_path.endswith('.h') and not file_path.startswith("./test/") and not file_path in EXCEPTION_ALLOWLIST) or file_path in EXCEPTION_DENYLIST \ - or self.isInSubdir(file_path, 'tools/testdata') + or self.is_in_subdir(file_path, 'tools/testdata') - def allowlistedForBuildUrls(self, file_path): - return file_path in BUILD_URLS_ALLOWLIST - - # TODO(chaoqin-li1123): Exclude some important extensions from allowlist. - def allowlistedForRawTry(self, file_path): + def allow_listed_for_raw_try(self, file_path): return file_path in RAW_TRY_ALLOWLIST or file_path.startswith("./source/extensions") - def isApiFile(self, file_path): + def allow_listed_for_build_urls(self, file_path): + return file_path in BUILD_URLS_ALLOWLIST + + def is_api_file(self, file_path): return file_path.startswith(self.api_prefix) or file_path.startswith(self.api_shadow_root) - def isBuildFile(self, file_path): + def is_build_file(self, file_path): basename = os.path.basename(file_path) if basename in {"BUILD", "BUILD.bazel"} or basename.endswith(".BUILD"): return True return False - def isExternalBuildFile(self, file_path): - return self.isBuildFile(file_path) and (file_path.startswith("./bazel/external/") or - file_path.startswith("./tools/clang_tools")) + def is_external_build_file(self, file_path): + return self.is_build_file(file_path) and (file_path.startswith("./bazel/external/") or + file_path.startswith("./tools/clang_tools")) - def isStarlarkFile(self, file_path): + def is_starlark_file(self, file_path): return file_path.endswith(".bzl") - def isWorkspaceFile(self, file_path): + def is_workspace_file(self, file_path): return os.path.basename(file_path) == "WORKSPACE" - def isBuildFixerExcludedFile(self, file_path): + def is_build_fixer_excluded_file(self, file_path): for excluded_path in self.build_fixer_check_excluded_paths: if file_path.startswith(excluded_path): return True return False - def hasInvalidAngleBracketDirectory(self, line): + def has_invalid_angle_bracket_directory(self, line): if not line.startswith(INCLUDE_ANGLE): return False path = line[INCLUDE_ANGLE_LEN:] @@ -491,12 +487,12 @@ def hasInvalidAngleBracketDirectory(self, line): subdir = path[0:slash] return subdir in SUBDIR_SET - def checkCurrentReleaseNotes(self, file_path, error_messages): + def check_current_release_notes(self, file_path, error_messages): first_word_of_prior_line = '' next_word_to_check = '' # first word after : prior_line = '' - def endsWithPeriod(prior_line): + def ends_with_period(prior_line): if not prior_line: return True # Don't punctuation-check empty lines. if prior_line.endswith('.'): @@ -505,9 +501,9 @@ def endsWithPeriod(prior_line): return True # The text in the :ref ends with a . return False - for line_number, line in enumerate(self.readLines(file_path)): + for line_number, line in enumerate(self.read_lines(file_path)): - def reportError(message): + def report_error(message): error_messages.append("%s:%d: %s" % (file_path, line_number + 1, message)) if VERSION_HISTORY_SECTION_NAME.match(line): @@ -522,36 +518,36 @@ def reportError(message): invalid_reflink_match = INVALID_REFLINK.match(line) if invalid_reflink_match: - reportError("Found text \" ref:\". This should probably be \" :ref:\"\n%s" % line) + report_error("Found text \" ref:\". This should probably be \" :ref:\"\n%s" % line) # make sure flags are surrounded by ``s flag_match = RELOADABLE_FLAG_REGEX.match(line) if flag_match: if not flag_match.groups()[0].startswith(' `'): - reportError("Flag `%s` should be enclosed in a single set of back ticks" % - flag_match.groups()[1]) + report_error("Flag `%s` should be enclosed in a single set of back ticks" % + flag_match.groups()[1]) if line.startswith("* "): - if not endsWithPeriod(prior_line): - reportError("The following release note does not end with a '.'\n %s" % prior_line) + if not ends_with_period(prior_line): + report_error("The following release note does not end with a '.'\n %s" % prior_line) match = VERSION_HISTORY_NEW_LINE_REGEX.match(line) if not match: - reportError("Version history line malformed. " - "Does not match VERSION_HISTORY_NEW_LINE_REGEX in check_format.py\n %s\n" - "Please use messages in the form 'category: feature explanation.', " - "starting with a lower-cased letter and ending with a period." % line) + report_error("Version history line malformed. " + "Does not match VERSION_HISTORY_NEW_LINE_REGEX in check_format.py\n %s\n" + "Please use messages in the form 'category: feature explanation.', " + "starting with a lower-cased letter and ending with a period." % line) else: first_word = match.groups()[0] next_word = match.groups()[1] # Do basic alphabetization checks of the first word on the line and the # first word after the : if first_word_of_prior_line and first_word_of_prior_line > first_word: - reportError( + report_error( "Version history not in alphabetical order (%s vs %s): please check placement of line\n %s. " % (first_word_of_prior_line, first_word, line)) if first_word_of_prior_line == first_word and next_word_to_check and next_word_to_check > next_word: - reportError( + report_error( "Version history not in alphabetical order (%s vs %s): please check placement of line\n %s. " % (next_word_to_check, next_word, line)) first_word_of_prior_line = first_word @@ -560,40 +556,40 @@ def reportError(message): prior_line = line elif not line: # If we hit the end of this release note block block, check the prior line. - if not endsWithPeriod(prior_line): - reportError("The following release note does not end with a '.'\n %s" % prior_line) + if not ends_with_period(prior_line): + report_error("The following release note does not end with a '.'\n %s" % prior_line) prior_line = '' elif prior_line: prior_line += line - def checkFileContents(self, file_path, checker): + def check_file_contents(self, file_path, checker): error_messages = [] if file_path.endswith("version_history/current.rst"): # Version file checking has enough special cased logic to merit its own checks. # This only validates entries for the current release as very old release # notes have a different format. - self.checkCurrentReleaseNotes(file_path, error_messages) + self.check_current_release_notes(file_path, error_messages) - def checkFormatErrors(line, line_number): + def check_format_errors(line, line_number): - def reportError(message): + def report_error(message): error_messages.append("%s:%d: %s" % (file_path, line_number + 1, message)) - checker(line, file_path, reportError) + checker(line, file_path, report_error) - evaluate_failure = self.evaluateLines(file_path, checkFormatErrors, False) + evaluate_failure = self.evaluate_lines(file_path, check_format_errors, False) if evaluate_failure is not None: error_messages.append(evaluate_failure) return error_messages - def fixSourceLine(self, line, line_number): + def fix_source_line(self, line, line_number): # Strip double space after '.' This may prove overenthusiastic and need to # be restricted to comments and metadata files but works for now. line = re.sub(DOT_MULTI_SPACE_REGEX, ". ", line) - if self.hasInvalidAngleBracketDirectory(line): + if self.has_invalid_angle_bracket_directory(line): line = line.replace("<", '"').replace(">", '"') # Fix incorrect protobuf namespace references. @@ -615,7 +611,7 @@ def fixSourceLine(self, line, line_number): # pick up time_system_.waitFor(...), and we don't want to return true for that # pattern. But in that case there is a strong pattern of using time_system in # various spellings as the variable name. - def hasCondVarWaitFor(self, line): + def has_cond_var_wait_for(self, line): wait_for = line.find(".waitFor(") if wait_for == -1: return False @@ -628,7 +624,7 @@ def hasCondVarWaitFor(self, line): # Determines whether the filename is either in the specified subdirectory, or # at the top level. We consider files in the top level for the benefit of # the check_format testcases in tools/testdata/check_format. - def isInSubdir(self, filename, *subdirs): + def is_in_subdir(self, filename, *subdirs): # Skip this check for check_format's unit-tests. if filename.count("/") <= 1: return True @@ -639,7 +635,7 @@ def isInSubdir(self, filename, *subdirs): # Determines if given token exists in line without leading or trailing token characters # e.g. will return True for a line containing foo() but not foo_bar() or baz_foo - def tokenInLine(self, token, line): + def token_in_line(self, token, line): index = 0 while True: index = line.find(token, index) @@ -656,184 +652,185 @@ def tokenInLine(self, token, line): index = index + 1 return False - def checkSourceLine(self, line, file_path, reportError): + def check_source_line(self, line, file_path, report_error): # Check fixable errors. These may have been fixed already. if line.find(". ") != -1: - reportError("over-enthusiastic spaces") - if self.isInSubdir(file_path, 'source', 'include') and X_ENVOY_USED_DIRECTLY_REGEX.match(line): - reportError( + report_error("over-enthusiastic spaces") + if self.is_in_subdir(file_path, 'source', + 'include') and X_ENVOY_USED_DIRECTLY_REGEX.match(line): + report_error( "Please do not use the raw literal x-envoy in source code. See Envoy::Http::PrefixValue." ) - if self.hasInvalidAngleBracketDirectory(line): - reportError("envoy includes should not have angle brackets") + if self.has_invalid_angle_bracket_directory(line): + report_error("envoy includes should not have angle brackets") for invalid_construct, valid_construct in PROTOBUF_TYPE_ERRORS.items(): if invalid_construct in line: - reportError("incorrect protobuf type reference %s; " - "should be %s" % (invalid_construct, valid_construct)) + report_error("incorrect protobuf type reference %s; " + "should be %s" % (invalid_construct, valid_construct)) for invalid_construct, valid_construct in LIBCXX_REPLACEMENTS.items(): if invalid_construct in line: - reportError("term %s should be replaced with standard library term %s" % - (invalid_construct, valid_construct)) + report_error("term %s should be replaced with standard library term %s" % + (invalid_construct, valid_construct)) for invalid_construct, valid_construct in CODE_CONVENTION_REPLACEMENTS.items(): if invalid_construct in line: - reportError("term %s should be replaced with preferred term %s" % - (invalid_construct, valid_construct)) + report_error("term %s should be replaced with preferred term %s" % + (invalid_construct, valid_construct)) # Do not include the virtual_includes headers. if re.search("#include.*/_virtual_includes/", line): - reportError("Don't include the virtual includes headers.") + report_error("Don't include the virtual includes headers.") # Some errors cannot be fixed automatically, and actionable, consistent, # navigable messages should be emitted to make it easy to find and fix # the errors by hand. - if not self.allowlistedForProtobufDeps(file_path): + if not self.allow_listed_for_protobuf_deps(file_path): if '"google/protobuf' in line or "google::protobuf" in line: - reportError("unexpected direct dependency on google.protobuf, use " - "the definitions in common/protobuf/protobuf.h instead.") + report_error("unexpected direct dependency on google.protobuf, use " + "the definitions in common/protobuf/protobuf.h instead.") if line.startswith("#include ") or line.startswith("#include or , switch to " - "Thread::MutexBasicLockable in source/common/common/thread.h") + report_error("Don't use or , switch to " + "Thread::MutexBasicLockable in source/common/common/thread.h") if line.startswith("#include "): # We don't check here for std::shared_timed_mutex because that may # legitimately show up in comments, for example this one. - reportError("Don't use , use absl::Mutex for reader/writer locks.") - if not self.allowlistedForRealTime(file_path) and not "NO_CHECK_FORMAT(real_time)" in line: + report_error("Don't use , use absl::Mutex for reader/writer locks.") + if not self.allow_listed_for_realtime(file_path) and not "NO_CHECK_FORMAT(real_time)" in line: if "RealTimeSource" in line or \ ("RealTimeSystem" in line and not "TestRealTimeSystem" in line) or \ "std::chrono::system_clock::now" in line or "std::chrono::steady_clock::now" in line or \ - "std::this_thread::sleep_for" in line or self.hasCondVarWaitFor(line): - reportError("Don't reference real-world time sources from production code; use injection") + "std::this_thread::sleep_for" in line or self.has_cond_var_wait_for(line): + report_error("Don't reference real-world time sources from production code; use injection") duration_arg = DURATION_VALUE_REGEX.search(line) if duration_arg and duration_arg.group(1) != "0" and duration_arg.group(1) != "0.0": # Matching duration(int-const or float-const) other than zero - reportError( + report_error( "Don't use ambiguous duration(value), use an explicit duration type, e.g. Event::TimeSystem::Milliseconds(value)" ) - if not self.allowlistedForRegisterFactory(file_path): + if not self.allow_listed_for_register_factory(file_path): if "Registry::RegisterFactory<" in line or "REGISTER_FACTORY" in line: - reportError("Don't use Registry::RegisterFactory or REGISTER_FACTORY in tests, " - "use Registry::InjectFactory instead.") - if not self.allowlistedForUnpackTo(file_path): + report_error("Don't use Registry::RegisterFactory or REGISTER_FACTORY in tests, " + "use Registry::InjectFactory instead.") + if not self.allow_listed_for_unpack_to(file_path): if "UnpackTo" in line: - reportError("Don't use UnpackTo() directly, use MessageUtil::unpackTo() instead") + report_error("Don't use UnpackTo() directly, use MessageUtil::unpackTo() instead") # Check that we use the absl::Time library - if self.tokenInLine("std::get_time", line): + if self.token_in_line("std::get_time", line): if "test/" in file_path: - reportError("Don't use std::get_time; use TestUtility::parseTime in tests") + report_error("Don't use std::get_time; use TestUtility::parseTime in tests") else: - reportError("Don't use std::get_time; use the injectable time system") - if self.tokenInLine("std::put_time", line): - reportError("Don't use std::put_time; use absl::Time equivalent instead") - if self.tokenInLine("gmtime", line): - reportError("Don't use gmtime; use absl::Time equivalent instead") - if self.tokenInLine("mktime", line): - reportError("Don't use mktime; use absl::Time equivalent instead") - if self.tokenInLine("localtime", line): - reportError("Don't use localtime; use absl::Time equivalent instead") - if self.tokenInLine("strftime", line): - reportError("Don't use strftime; use absl::FormatTime instead") - if self.tokenInLine("strptime", line): - reportError("Don't use strptime; use absl::FormatTime instead") - if self.tokenInLine("strerror", line): - reportError("Don't use strerror; use Envoy::errorDetails instead") + report_error("Don't use std::get_time; use the injectable time system") + if self.token_in_line("std::put_time", line): + report_error("Don't use std::put_time; use absl::Time equivalent instead") + if self.token_in_line("gmtime", line): + report_error("Don't use gmtime; use absl::Time equivalent instead") + if self.token_in_line("mktime", line): + report_error("Don't use mktime; use absl::Time equivalent instead") + if self.token_in_line("localtime", line): + report_error("Don't use localtime; use absl::Time equivalent instead") + if self.token_in_line("strftime", line): + report_error("Don't use strftime; use absl::FormatTime instead") + if self.token_in_line("strptime", line): + report_error("Don't use strptime; use absl::FormatTime instead") + if self.token_in_line("strerror", line): + report_error("Don't use strerror; use Envoy::errorDetails instead") # Prefer using abseil hash maps/sets over std::unordered_map/set for performance optimizations and # non-deterministic iteration order that exposes faulty assertions. # See: https://abseil.io/docs/cpp/guides/container#hash-tables if "std::unordered_map" in line: - reportError("Don't use std::unordered_map; use absl::flat_hash_map instead or " - "absl::node_hash_map if pointer stability of keys/values is required") + report_error("Don't use std::unordered_map; use absl::flat_hash_map instead or " + "absl::node_hash_map if pointer stability of keys/values is required") if "std::unordered_set" in line: - reportError("Don't use std::unordered_set; use absl::flat_hash_set instead or " - "absl::node_hash_set if pointer stability of keys/values is required") + report_error("Don't use std::unordered_set; use absl::flat_hash_set instead or " + "absl::node_hash_set if pointer stability of keys/values is required") if "std::atomic_" in line: # The std::atomic_* free functions are functionally equivalent to calling # operations on std::atomic objects, so prefer to use that instead. - reportError("Don't use free std::atomic_* functions, use std::atomic members instead.") + report_error("Don't use free std::atomic_* functions, use std::atomic members instead.") # Block usage of certain std types/functions as iOS 11 and macOS 10.13 # do not support these at runtime. # See: https://github.com/envoyproxy/envoy/issues/12341 - if self.tokenInLine("std::any", line): - reportError("Don't use std::any; use absl::any instead") - if self.tokenInLine("std::get_if", line): - reportError("Don't use std::get_if; use absl::get_if instead") - if self.tokenInLine("std::holds_alternative", line): - reportError("Don't use std::holds_alternative; use absl::holds_alternative instead") - if self.tokenInLine("std::make_optional", line): - reportError("Don't use std::make_optional; use absl::make_optional instead") - if self.tokenInLine("std::monostate", line): - reportError("Don't use std::monostate; use absl::monostate instead") - if self.tokenInLine("std::optional", line): - reportError("Don't use std::optional; use absl::optional instead") - if self.tokenInLine("std::string_view", line): - reportError("Don't use std::string_view; use absl::string_view instead") - if self.tokenInLine("std::variant", line): - reportError("Don't use std::variant; use absl::variant instead") - if self.tokenInLine("std::visit", line): - reportError("Don't use std::visit; use absl::visit instead") - if " try {" in line and file_path.startswith( - "./source") and not self.allowlistedForRawTry(file_path): - reportError( + if self.token_in_line("std::any", line): + report_error("Don't use std::any; use absl::any instead") + if self.token_in_line("std::get_if", line): + report_error("Don't use std::get_if; use absl::get_if instead") + if self.token_in_line("std::holds_alternative", line): + report_error("Don't use std::holds_alternative; use absl::holds_alternative instead") + if self.token_in_line("std::make_optional", line): + report_error("Don't use std::make_optional; use absl::make_optional instead") + if self.token_in_line("std::monostate", line): + report_error("Don't use std::monostate; use absl::monostate instead") + if self.token_in_line("std::optional", line): + report_error("Don't use std::optional; use absl::optional instead") + if self.token_in_line("std::string_view", line): + report_error("Don't use std::string_view; use absl::string_view instead") + if self.token_in_line("std::variant", line): + report_error("Don't use std::variant; use absl::variant instead") + if self.token_in_line("std::visit", line): + report_error("Don't use std::visit; use absl::visit instead") + if " try {" in line and file_path.startswith("./source") and not self.allow_listed_for_raw_try(file_path): + report_error( "Don't use raw try, use TRY_ASSERT_MAIN_THREAD if on the main thread otherwise don't use exceptions." ) if "__attribute__((packed))" in line and file_path != "./include/envoy/common/platform.h": # __attribute__((packed)) is not supported by MSVC, we have a PACKED_STRUCT macro that # can be used instead - reportError("Don't use __attribute__((packed)), use the PACKED_STRUCT macro defined " - "in include/envoy/common/platform.h instead") + report_error("Don't use __attribute__((packed)), use the PACKED_STRUCT macro defined " + "in include/envoy/common/platform.h instead") if DESIGNATED_INITIALIZER_REGEX.search(line): # Designated initializers are not part of the C++14 standard and are not supported # by MSVC - reportError("Don't use designated initializers in struct initialization, " - "they are not part of C++14") + report_error("Don't use designated initializers in struct initialization, " + "they are not part of C++14") if " ?: " in line: # The ?: operator is non-standard, it is a GCC extension - reportError("Don't use the '?:' operator, it is a non-standard GCC extension") + report_error("Don't use the '?:' operator, it is a non-standard GCC extension") if line.startswith("using testing::Test;"): - reportError("Don't use 'using testing::Test;, elaborate the type instead") + report_error("Don't use 'using testing::Test;, elaborate the type instead") if line.startswith("using testing::TestWithParams;"): - reportError("Don't use 'using testing::Test;, elaborate the type instead") + report_error("Don't use 'using testing::Test;, elaborate the type instead") if TEST_NAME_STARTING_LOWER_CASE_REGEX.search(line): # Matches variants of TEST(), TEST_P(), TEST_F() etc. where the test name begins # with a lowercase letter. - reportError("Test names should be CamelCase, starting with a capital letter") + report_error("Test names should be CamelCase, starting with a capital letter") if OLD_MOCK_METHOD_REGEX.search(line): - reportError("The MOCK_METHODn() macros should not be used, use MOCK_METHOD() instead") + report_error("The MOCK_METHODn() macros should not be used, use MOCK_METHOD() instead") if FOR_EACH_N_REGEX.search(line): - reportError("std::for_each_n should not be used, use an alternative for loop instead") + report_error("std::for_each_n should not be used, use an alternative for loop instead") - if not self.allowlistedForSerializeAsString(file_path) and "SerializeAsString" in line: + if not self.allow_listed_for_serialize_as_string(file_path) and "SerializeAsString" in line: # The MessageLite::SerializeAsString doesn't generate deterministic serialization, # use MessageUtil::hash instead. - reportError( + report_error( "Don't use MessageLite::SerializeAsString for generating deterministic serialization, use MessageUtil::hash instead." ) - if not self.allowlistedForJsonStringToMessage(file_path) and "JsonStringToMessage" in line: + if not self.allow_listed_for_json_string_to_message( + file_path) and "JsonStringToMessage" in line: # Centralize all usage of JSON parsing so it is easier to make changes in JSON parsing # behavior. - reportError("Don't use Protobuf::util::JsonStringToMessage, use TestUtility::loadFromJson.") + report_error("Don't use Protobuf::util::JsonStringToMessage, use TestUtility::loadFromJson.") - if self.isInSubdir(file_path, 'source') and file_path.endswith('.cc') and \ + if self.is_in_subdir(file_path, 'source') and file_path.endswith('.cc') and \ ('.counterFromString(' in line or '.gaugeFromString(' in line or \ '.histogramFromString(' in line or '.textReadoutFromString(' in line or \ '->counterFromString(' in line or '->gaugeFromString(' in line or \ '->histogramFromString(' in line or '->textReadoutFromString(' in line): - reportError("Don't lookup stats by name at runtime; use StatName saved during construction") + report_error("Don't lookup stats by name at runtime; use StatName saved during construction") if MANGLED_PROTOBUF_NAME_REGEX.search(line): - reportError("Don't use mangled Protobuf names for enum constants") + report_error("Don't use mangled Protobuf names for enum constants") hist_m = HISTOGRAM_SI_SUFFIX_REGEX.search(line) - if hist_m and not self.allowlistedForHistogramSiSuffix(hist_m.group(0)): - reportError( + if hist_m and not self.allow_listed_for_histogram_si_suffix(hist_m.group(0)): + report_error( "Don't suffix histogram names with the unit symbol, " "it's already part of the histogram object and unit-supporting sinks can use this information natively, " "other sinks can add the suffix automatically on flush should they prefer to do so.") - if not self.allowlistedForStdRegex(file_path) and "std::regex" in line: - reportError("Don't use std::regex in code that handles untrusted input. Use RegexMatcher") + if not self.allow_listed_for_std_regex(file_path) and "std::regex" in line: + report_error("Don't use std::regex in code that handles untrusted input. Use RegexMatcher") - if not self.allowlistedForGrpcInit(file_path): + if not self.allow_listed_for_grpc_init(file_path): grpc_init_or_shutdown = line.find("grpc_init()") grpc_shutdown = line.find("grpc_shutdown()") if grpc_init_or_shutdown == -1 or (grpc_shutdown != -1 and @@ -842,27 +839,27 @@ def checkSourceLine(self, line, file_path, reportError): if grpc_init_or_shutdown != -1: comment = line.find("// ") if comment == -1 or comment > grpc_init_or_shutdown: - reportError("Don't call grpc_init() or grpc_shutdown() directly, instantiate " + - "Grpc::GoogleGrpcContext. See #8282") + report_error("Don't call grpc_init() or grpc_shutdown() directly, instantiate " + + "Grpc::GoogleGrpcContext. See #8282") - if not self.whitelistedForMemcpy(file_path) and \ + if not self.whitelisted_for_memcpy(file_path) and \ not ("test/" in file_path) and \ ("memcpy(" in line) and \ not ("NOLINT(safe-memcpy)" in line): - reportError( + report_error( "Don't call memcpy() directly; use safeMemcpy, safeMemcpyUnsafeSrc, safeMemcpyUnsafeDst or MemBlockBuilder instead." ) - if self.denylistedForExceptions(file_path): + if self.deny_listed_for_exceptions(file_path): # Skpping cases where 'throw' is a substring of a symbol like in "foothrowBar". if "throw" in line.split(): comment_match = COMMENT_REGEX.search(line) if comment_match is None or comment_match.start(0) > line.find("throw"): - reportError("Don't introduce throws into exception-free files, use error " + - "statuses instead.") + report_error("Don't introduce throws into exception-free files, use error " + + "statuses instead.") if "lua_pushlightuserdata" in line: - reportError( + report_error( "Don't use lua_pushlightuserdata, since it can cause unprotected error in call to" + "Lua API (bad light userdata pointer) on ARM64 architecture. See " + "https://github.com/LuaJIT/LuaJIT/issues/450#issuecomment-433659873 for details.") @@ -872,37 +869,39 @@ def checkSourceLine(self, line, file_path, reportError): result = PROTO_VALIDATION_STRING.search(line) if result is not None: if not any(x in file_path for x in exclude_path): - reportError("min_bytes is DEPRECATED, Use min_len.") + report_error("min_bytes is DEPRECATED, Use min_len.") - def checkBuildLine(self, line, file_path, reportError): - if "@bazel_tools" in line and not (self.isStarlarkFile(file_path) or + def check_build_line(self, line, file_path, report_error): + if "@bazel_tools" in line and not (self.is_starlark_file(file_path) or file_path.startswith("./bazel/") or "python/runfiles" in line): - reportError("unexpected @bazel_tools reference, please indirect via a definition in //bazel") - if not self.allowlistedForProtobufDeps(file_path) and '"protobuf"' in line: - reportError("unexpected direct external dependency on protobuf, use " - "//source/common/protobuf instead.") - if (self.envoy_build_rule_check and not self.isStarlarkFile(file_path) and - not self.isWorkspaceFile(file_path) and not self.isExternalBuildFile(file_path) and + report_error("unexpected @bazel_tools reference, please indirect via a definition in //bazel") + if not self.allow_listed_for_protobuf_deps(file_path) and '"protobuf"' in line: + report_error("unexpected direct external dependency on protobuf, use " + "//source/common/protobuf instead.") + if (self.envoy_build_rule_check and not self.is_starlark_file(file_path) and + not self.is_workspace_file(file_path) and not self.is_external_build_file(file_path) and "@envoy//" in line): - reportError("Superfluous '@envoy//' prefix") - if not self.allowlistedForBuildUrls(file_path) and (" urls = " in line or " url = " in line): - reportError("Only repository_locations.bzl may contains URL references") - - def fixBuildLine(self, file_path, line, line_number): - if (self.envoy_build_rule_check and not self.isStarlarkFile(file_path) and - not self.isWorkspaceFile(file_path) and not self.isExternalBuildFile(file_path)): + report_error("Superfluous '@envoy//' prefix") + if not self.allow_listed_for_build_urls(file_path) and (" urls = " in line or + " url = " in line): + report_error("Only repository_locations.bzl may contains URL references") + + def fix_build_line(self, file_path, line, line_number): + if (self.envoy_build_rule_check and not self.is_starlark_file(file_path) and + not self.is_workspace_file(file_path) and not self.is_external_build_file(file_path)): line = line.replace("@envoy//", "//") return line - def fixBuildPath(self, file_path): - self.evaluateLines(file_path, functools.partial(self.fixBuildLine, file_path)) + def fix_build_path(self, file_path): + self.evaluate_lines(file_path, functools.partial(self.fix_build_line, file_path)) error_messages = [] # TODO(htuch): Add API specific BUILD fixer script. - if not self.isBuildFixerExcludedFile(file_path) and not self.isApiFile( - file_path) and not self.isStarlarkFile(file_path) and not self.isWorkspaceFile(file_path): + if not self.is_build_fixer_excluded_file(file_path) and not self.is_api_file( + file_path) and not self.is_starlark_file(file_path) and not self.is_workspace_file( + file_path): if os.system("%s %s %s" % (ENVOY_BUILD_FIXER_PATH, file_path, file_path)) != 0: error_messages += ["envoy_build_fixer rewrite failed for file: %s" % file_path] @@ -910,18 +909,19 @@ def fixBuildPath(self, file_path): error_messages += ["buildifier rewrite failed for file: %s" % file_path] return error_messages - def checkBuildPath(self, file_path): + def check_build_path(self, file_path): error_messages = [] - if not self.isBuildFixerExcludedFile(file_path) and not self.isApiFile( - file_path) and not self.isStarlarkFile(file_path) and not self.isWorkspaceFile(file_path): + if not self.is_build_fixer_excluded_file(file_path) and not self.is_api_file( + file_path) and not self.is_starlark_file(file_path) and not self.is_workspace_file( + file_path): command = "%s %s | diff %s -" % (ENVOY_BUILD_FIXER_PATH, file_path, file_path) - error_messages += self.executeCommand(command, "envoy_build_fixer check failed", file_path) + error_messages += self.execute_command(command, "envoy_build_fixer check failed", file_path) - if self.isBuildFile(file_path) and (file_path.startswith(self.api_prefix + "envoy") or - file_path.startswith(self.api_shadow_root + "envoy")): + if self.is_build_file(file_path) and (file_path.startswith(self.api_prefix + "envoy") or + file_path.startswith(self.api_shadow_root + "envoy")): found = False - for line in self.readLines(file_path): + for line in self.read_lines(file_path): if "api_proto_package(" in line: found = True break @@ -929,39 +929,39 @@ def checkBuildPath(self, file_path): error_messages += ["API build file does not provide api_proto_package()"] command = "%s -mode=diff %s" % (BUILDIFIER_PATH, file_path) - error_messages += self.executeCommand(command, "buildifier check failed", file_path) - error_messages += self.checkFileContents(file_path, self.checkBuildLine) + error_messages += self.execute_command(command, "buildifier check failed", file_path) + error_messages += self.check_file_contents(file_path, self.check_build_line) return error_messages - def fixSourcePath(self, file_path): - self.evaluateLines(file_path, self.fixSourceLine) + def fix_source_path(self, file_path): + self.evaluate_lines(file_path, self.fix_source_line) error_messages = [] if not file_path.endswith(DOCS_SUFFIX): if not file_path.endswith(PROTO_SUFFIX): - error_messages += self.fixHeaderOrder(file_path) - error_messages += self.clangFormat(file_path) - if file_path.endswith(PROTO_SUFFIX) and self.isApiFile(file_path): - package_name, error_message = self.packageNameForProto(file_path) + error_messages += self.fix_header_order(file_path) + error_messages += self.clang_format(file_path) + if file_path.endswith(PROTO_SUFFIX) and self.is_api_file(file_path): + package_name, error_message = self.package_name_for_proto(file_path) if package_name is None: error_messages += error_message return error_messages - def checkSourcePath(self, file_path): - error_messages = self.checkFileContents(file_path, self.checkSourceLine) + def check_source_path(self, file_path): + error_messages = self.check_file_contents(file_path, self.check_source_line) if not file_path.endswith(DOCS_SUFFIX): if not file_path.endswith(PROTO_SUFFIX): - error_messages += self.checkNamespace(file_path) + error_messages += self.check_namespace(file_path) command = ("%s --include_dir_order %s --path %s | diff %s -" % (HEADER_ORDER_PATH, self.include_dir_order, file_path, file_path)) - error_messages += self.executeCommand(command, "header_order.py check failed", file_path) + error_messages += self.execute_command(command, "header_order.py check failed", file_path) command = ("%s %s | diff %s -" % (CLANG_FORMAT_PATH, file_path, file_path)) - error_messages += self.executeCommand(command, "clang-format check failed", file_path) + error_messages += self.execute_command(command, "clang-format check failed", file_path) - if file_path.endswith(PROTO_SUFFIX) and self.isApiFile(file_path): - package_name, error_message = self.packageNameForProto(file_path) + if file_path.endswith(PROTO_SUFFIX) and self.is_api_file(file_path): + package_name, error_message = self.package_name_for_proto(file_path) if package_name is None: error_messages += error_message return error_messages @@ -970,11 +970,11 @@ def checkSourcePath(self, file_path): # - "26,27c26" # - "12,13d13" # - "7a8,9" - def executeCommand(self, - command, - error_message, - file_path, - regex=re.compile(r"^(\d+)[a|c|d]?\d*(?:,\d+[a|c|d]?\d*)?$")): + def execute_command(self, + command, + error_message, + file_path, + regex=re.compile(r"^(\d+)[a|c|d]?\d*(?:,\d+[a|c|d]?\d*)?$")): try: output = subprocess.check_output(command, shell=True, stderr=subprocess.STDOUT).strip() if output: @@ -990,20 +990,20 @@ def executeCommand(self, error_messages.append(" %s:%s" % (file_path, num)) return error_messages - def fixHeaderOrder(self, file_path): + def fix_header_order(self, file_path): command = "%s --rewrite --include_dir_order %s --path %s" % (HEADER_ORDER_PATH, self.include_dir_order, file_path) if os.system(command) != 0: return ["header_order.py rewrite error: %s" % (file_path)] return [] - def clangFormat(self, file_path): + def clang_format(self, file_path): command = "%s -i %s" % (CLANG_FORMAT_PATH, file_path) if os.system(command) != 0: return ["clang-format rewrite error: %s" % (file_path)] return [] - def checkFormat(self, file_path): + def check_format(self, file_path): if file_path.startswith(EXCLUDED_PREFIXES): return [] @@ -1014,28 +1014,28 @@ def checkFormat(self, file_path): # Apply fixes first, if asked, and then run checks. If we wind up attempting to fix # an issue, but there's still an error, that's a problem. try_to_fix = self.operation_type == "fix" - if self.isBuildFile(file_path) or self.isStarlarkFile(file_path) or self.isWorkspaceFile( + if self.is_build_file(file_path) or self.is_starlark_file(file_path) or self.is_workspace_file( file_path): if try_to_fix: - error_messages += self.fixBuildPath(file_path) - error_messages += self.checkBuildPath(file_path) + error_messages += self.fix_build_path(file_path) + error_messages += self.check_build_path(file_path) else: if try_to_fix: - error_messages += self.fixSourcePath(file_path) - error_messages += self.checkSourcePath(file_path) + error_messages += self.fix_source_path(file_path) + error_messages += self.check_source_path(file_path) if error_messages: return ["From %s" % file_path] + error_messages return error_messages - def checkFormatReturnTraceOnError(self, file_path): - """Run checkFormat and return the traceback of any exception.""" + def check_format_return_trace_on_error(self, file_path): + """Run check_format and return the traceback of any exception.""" try: - return self.checkFormat(file_path) + return self.check_format(file_path) except: return traceback.format_exc().split("\n") - def checkOwners(self, dir_name, owned_directories, error_messages): + def check_owners(self, dir_name, owned_directories, error_messages): """Checks to make sure a given directory is present either in CODEOWNERS or OWNED_EXTENSIONS Args: dir_name: the directory being checked. @@ -1049,13 +1049,13 @@ def checkOwners(self, dir_name, owned_directories, error_messages): if not found and dir_name not in UNOWNED_EXTENSIONS: error_messages.append("New directory %s appears to not have owners in CODEOWNERS" % dir_name) - def checkApiShadowStarlarkFiles(self, file_path, error_messages): + def check_api_shadow_starlark_files(self, file_path, error_messages): command = "diff -u " command += file_path + " " api_shadow_starlark_path = self.api_shadow_root + re.sub(r"\./api/", '', file_path) command += api_shadow_starlark_path - error_message = self.executeCommand(command, "invalid .bzl in generated_api_shadow", file_path) + error_message = self.execute_command(command, "invalid .bzl in generated_api_shadow", file_path) if self.operation_type == "check": error_messages += error_message elif self.operation_type == "fix" and len(error_message) != 0: @@ -1063,8 +1063,8 @@ def checkApiShadowStarlarkFiles(self, file_path, error_messages): return error_messages - def checkFormatVisitor(self, arg, dir_name, names): - """Run checkFormat in parallel for the given files. + def check_format_visitor(self, arg, dir_name, names): + """Run check_format in parallel for the given files. Args: arg: a tuple (pool, result_list, owned_directories, error_messages) pool and result_list are for starting tasks asynchronously. @@ -1076,7 +1076,7 @@ def checkFormatVisitor(self, arg, dir_name, names): # Unpack the multiprocessing.Pool process pool and list of results. Since # python lists are passed as references, this is used to collect the list of - # async results (futures) from running checkFormat and passing them back to + # async results (futures) from running check_format and passing them back to # the caller. pool, result_list, owned_directories, error_messages = arg @@ -1088,26 +1088,29 @@ def checkFormatVisitor(self, arg, dir_name, names): # Also ignore top level directories under /source/extensions since we don't # need owners for source/extensions/access_loggers etc, just the subdirectories. if dir_name.startswith(full_prefix) and '/' in dir_name[len(full_prefix):]: - self.checkOwners(dir_name[len(source_prefix):], owned_directories, error_messages) + self.check_owners(dir_name[len(source_prefix):], owned_directories, error_messages) for file_name in names: - if dir_name.startswith("./api") and self.isStarlarkFile(file_name): - result = pool.apply_async(self.checkApiShadowStarlarkFiles, + if dir_name.startswith("./api") and self.is_starlark_file(file_name): + result = pool.apply_async(self.check_api_shadow_starlark_files, args=(dir_name + "/" + file_name, error_messages)) result_list.append(result) - result = pool.apply_async(self.checkFormatReturnTraceOnError, + result = pool.apply_async(self.check_format_return_trace_on_error, args=(dir_name + "/" + file_name,)) result_list.append(result) - # checkErrorMessages iterates over the list with error messages and prints + # check_error_messages iterates over the list with error messages and prints # errors and returns a bool based on whether there were any errors. - def checkErrorMessages(self, error_messages): + def check_error_messages(self, error_messages): if error_messages: for e in error_messages: print("ERROR: %s" % e) return True return False + def whitelisted_for_memcpy(self, file_path): + return file_path in MEMCPY_WHITELIST + if __name__ == "__main__": parser = argparse.ArgumentParser(description="Check or fix file format.") @@ -1160,7 +1163,7 @@ def checkErrorMessages(self, error_messages): help="exclude paths from bazel_tools check.") parser.add_argument("--include_dir_order", type=str, - default=",".join(common.includeDirOrder()), + default=",".join(common.include_dir_order()), help="specify the header block include directory order.") args = parser.parse_args() if args.add_excluded_prefixes: @@ -1168,13 +1171,13 @@ def checkErrorMessages(self, error_messages): format_checker = FormatChecker(args) # Check whether all needed external tools are available. - ct_error_messages = format_checker.checkTools() - if format_checker.checkErrorMessages(ct_error_messages): + ct_error_messages = format_checker.check_tools() + if format_checker.check_error_messages(ct_error_messages): sys.exit(1) # Returns the list of directories with owners listed in CODEOWNERS. May append errors to # error_messages. - def ownedDirectories(error_messages): + def owned_directories(error_messages): owned = [] maintainers = [ '@mattklein123', '@htuch', '@alyssawilk', '@zuercher', '@lizan', '@snowp', '@asraa', @@ -1204,20 +1207,20 @@ def ownedDirectories(error_messages): # Calculate the list of owned directories once per run. error_messages = [] - owned_directories = ownedDirectories(error_messages) + owned_directories = owned_directories(error_messages) if os.path.isfile(args.target_path): - error_messages += format_checker.checkFormat("./" + args.target_path) + error_messages += format_checker.check_format("./" + args.target_path) else: results = [] - def PooledCheckFormat(path_predicate): + def pooled_check_format(path_predicate): pool = multiprocessing.Pool(processes=args.num_workers) # For each file in target_path, start a new task in the pool and collect the # results (results is passed by reference, and is used as an output). for root, _, files in os.walk(args.target_path): - format_checker.checkFormatVisitor((pool, results, owned_directories, error_messages), root, - [f for f in files if path_predicate(f)]) + format_checker.check_format_visitor((pool, results, owned_directories, error_messages), + root, [f for f in files if path_predicate(f)]) # Close the pool to new tasks, wait for all of the running tasks to finish, # then collect the error messages. @@ -1227,14 +1230,14 @@ def PooledCheckFormat(path_predicate): # We first run formatting on non-BUILD files, since the BUILD file format # requires analysis of srcs/hdrs in the BUILD file, and we don't want these # to be rewritten by other multiprocessing pooled processes. - PooledCheckFormat(lambda f: not format_checker.isBuildFile(f)) - PooledCheckFormat(lambda f: format_checker.isBuildFile(f)) + pooled_check_format(lambda f: not format_checker.is_build_file(f)) + pooled_check_format(lambda f: format_checker.is_build_file(f)) error_messages += sum((r.get() for r in results), []) - if format_checker.checkErrorMessages(error_messages): + if format_checker.check_error_messages(error_messages): print("ERROR: check format failed. run 'tools/code_format/check_format.py fix'") sys.exit(1) if args.operation_type == "check": - print("PASS") + print("PASS") \ No newline at end of file From 9258ce4fe6067b7619acab630382dbec8e732017 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 17 Mar 2021 23:06:49 +0000 Subject: [PATCH 25/41] format Signed-off-by: chaoqin-li1123 --- tools/code_format/check_format.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index bac61cf90fd1..e3b6b298677d 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -447,6 +447,7 @@ def deny_listed_for_exceptions(self, file_path): or self.is_in_subdir(file_path, 'tools/testdata') def allow_listed_for_raw_try(self, file_path): + # TODO(chaoqin-li1123): Exclude some important extensions from ALLOWLIST. return file_path in RAW_TRY_ALLOWLIST or file_path.startswith("./source/extensions") def allow_listed_for_build_urls(self, file_path): From da9386e0612757018276046d7f27a90a2b03c8f9 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 17 Mar 2021 23:29:49 +0000 Subject: [PATCH 26/41] fix python format messed up Signed-off-by: chaoqin-li1123 --- tools/code_format/check_format.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index e3b6b298677d..9c9be2705e39 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -769,7 +769,8 @@ def check_source_line(self, line, file_path, report_error): report_error("Don't use std::variant; use absl::variant instead") if self.token_in_line("std::visit", line): report_error("Don't use std::visit; use absl::visit instead") - if " try {" in line and file_path.startswith("./source") and not self.allow_listed_for_raw_try(file_path): + if " try {" in line and file_path.startswith( + "./source") and not self.allow_listed_for_raw_try(file_path): report_error( "Don't use raw try, use TRY_ASSERT_MAIN_THREAD if on the main thread otherwise don't use exceptions." ) @@ -1241,4 +1242,4 @@ def pooled_check_format(path_predicate): sys.exit(1) if args.operation_type == "check": - print("PASS") \ No newline at end of file + print("PASS") From 0e20b2bd69df0fc7d45659b443bb5ed7ab2ae82b Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Thu, 18 Mar 2021 00:00:11 +0000 Subject: [PATCH 27/41] more cleanup Signed-off-by: chaoqin-li1123 --- source/common/network/utility.cc | 8 ++++---- source/common/router/header_formatter.cc | 4 +++- tools/code_format/check_format.py | 3 ++- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index f2295e4d167f..de6401e6b166 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -83,11 +83,11 @@ uint32_t portFromUrl(const std::string& url, absl::string_view scheme, throw EnvoyException(absl::StrCat("malformed url: ", url)); } - TRY_NEEDS_AUDIT { return std::stoi(url.substr(colon_index + 1)); } - catch (const std::invalid_argument& e) { + try { + return std::stoi(url.substr(colon_index + 1)); + } catch (const std::invalid_argument& e) { throw EnvoyException(e.what()); - } - catch (const std::out_of_range& e) { + } catch (const std::out_of_range& e) { throw EnvoyException(e.what()); } } diff --git a/source/common/router/header_formatter.cc b/source/common/router/header_formatter.cc index cfb005b0d768..aa464deab783 100644 --- a/source/common/router/header_formatter.cc +++ b/source/common/router/header_formatter.cc @@ -6,6 +6,7 @@ #include "common/common/fmt.h" #include "common/common/logger.h" +#include "common/common/thread.h" #include "common/common/utility.h" #include "common/config/metadata.h" #include "common/formatter/substitution_formatter.h" @@ -76,13 +77,14 @@ parseMetadataField(absl::string_view params_str, bool upstream = true) { absl::string_view json = params_str.substr(1, params_str.size() - 2); // trim parens std::vector params; - TRY_NEEDS_AUDIT { + TRY_ASSERT_MAIN_THREAD { Json::ObjectSharedPtr parsed_params = Json::Factory::loadFromString(std::string(json)); for (const auto& param : parsed_params->asObjectArray()) { params.emplace_back(param->asString()); } } + END_TRY catch (Json::Exception& e) { throw EnvoyException(formatUpstreamMetadataParseException(params_str, &e)); } diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index 9c9be2705e39..d9fc36e6c799 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -115,7 +115,8 @@ EXCEPTION_DENYLIST = ("./source/common/http/http2/codec_impl.h", "./source/common/http/http2/codec_impl.cc") -RAW_TRY_ALLOWLIST = ("./source/common/common/regex.cc", "./source/common/common/thread.h") +RAW_TRY_ALLOWLIST = ("./source/common/common/regex.cc", "./source/common/common/thread.h", + "./source/common/network/utility.cc") # Header files that can throw exceptions. These should be limited; the only # valid situation identified so far is template functions used for config From 23669dfb852c985155caf1fc56fad69061ac8de5 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Sat, 20 Mar 2021 16:43:00 +0000 Subject: [PATCH 28/41] add format checking tools Signed-off-by: chaoqin-li1123 --- source/common/network/dns_impl.cc | 2 -- tools/code_format/check_format_test_helper.py | 4 ++++ tools/testdata/check_format/source/raw_try.cc | 15 +++++++++++++++ 3 files changed, 19 insertions(+), 2 deletions(-) create mode 100644 tools/testdata/check_format/source/raw_try.cc diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index 9ed09a320dc7..a002c563202d 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -1,7 +1,5 @@ #include "common/network/dns_impl.h" -#include - #include #include #include diff --git a/tools/code_format/check_format_test_helper.py b/tools/code_format/check_format_test_helper.py index 030dc665404e..b277499db09c 100755 --- a/tools/code_format/check_format_test_helper.py +++ b/tools/code_format/check_format_test_helper.py @@ -225,6 +225,10 @@ def run_checks(): "grpc_shutdown.cc", "Don't call grpc_init() or grpc_shutdown() directly, instantiate Grpc::GoogleGrpcContext. " + "See #8282") + errors += check_unfixable_error( + "source/raw_try.cc", + "Don't use raw try, use TRY_ASSERT_MAIN_THREAD if on the main thread otherwise don't use exceptions." + ) errors += check_unfixable_error("clang_format_double_off.cc", "clang-format nested off") errors += check_unfixable_error("clang_format_trailing_off.cc", "clang-format remains off") errors += check_unfixable_error("clang_format_double_on.cc", "clang-format nested on") diff --git a/tools/testdata/check_format/source/raw_try.cc b/tools/testdata/check_format/source/raw_try.cc new file mode 100644 index 000000000000..29232bebe0cc --- /dev/null +++ b/tools/testdata/check_format/source/raw_try.cc @@ -0,0 +1,15 @@ +#include +#include + +namespace Envoy { + +struct Try { + Try(std::string s) { + try { + std::stoi(s); + } + catch (std::exception&) {} + } +}; + +} // namespace Envoy \ No newline at end of file From 13513302ae0cd462f1eafb38730ce6353e6acae9 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Mon, 22 Mar 2021 21:16:32 +0000 Subject: [PATCH 29/41] remove try in flushing log Signed-off-by: chaoqin-li1123 --- .../access_log/access_log_manager_impl.cc | 40 +++++++++---------- .../access_log/access_log_manager_impl.h | 2 +- source/common/common/thread.cc | 5 ++- source/common/common/thread.h | 14 ++++++- 4 files changed, 37 insertions(+), 24 deletions(-) diff --git a/source/common/access_log/access_log_manager_impl.cc b/source/common/access_log/access_log_manager_impl.cc index 338a81f16995..14bcb798b06d 100644 --- a/source/common/access_log/access_log_manager_impl.cc +++ b/source/common/access_log/access_log_manager_impl.cc @@ -2,6 +2,8 @@ #include +#include "envoy/common/exception.h" + #include "common/common/assert.h" #include "common/common/fmt.h" #include "common/common/lock_guard.h" @@ -50,7 +52,11 @@ AccessLogFileImpl::AccessLogFileImpl(Filesystem::FilePtr&& file, Event::Dispatch })), thread_factory_(thread_factory), flush_interval_msec_(flush_interval_msec), stats_(stats) { flush_timer_->enableTimer(flush_interval_msec_); - open(); + auto open_result = open(); + if (!open_result.rc_) { + throw EnvoyException(fmt::format("unable to open file '{}': {}", file_->path(), + open_result.err_->getErrorDetails())); + } } Filesystem::FlagSet AccessLogFileImpl::defaultFlags() { @@ -61,12 +67,9 @@ Filesystem::FlagSet AccessLogFileImpl::defaultFlags() { return default_flags; } -void AccessLogFileImpl::open() { - const Api::IoCallBoolResult result = file_->open(defaultFlags()); - if (!result.rc_) { - throw EnvoyException( - fmt::format("unable to open file '{}': {}", file_->path(), result.err_->getErrorDetails())); - } +Api::IoCallBoolResult AccessLogFileImpl::open() { + Api::IoCallBoolResult result = file_->open(defaultFlags()); + return result; } void AccessLogFileImpl::reopen() { reopen_file_ = true; } @@ -87,7 +90,6 @@ AccessLogFileImpl::~AccessLogFileImpl() { if (flush_buffer_.length() > 0) { doWrite(flush_buffer_); } - const Api::IoCallBoolResult result = file_->close(); ASSERT(result.rc_, fmt::format("unable to close file '{}': {}", file_->path(), result.err_->getErrorDetails())); @@ -149,20 +151,18 @@ void AccessLogFileImpl::flushThreadFunc() { // if we failed to open file before, then simply ignore if (file_->isOpen()) { - TRY_NEEDS_AUDIT { - if (reopen_file_) { - reopen_file_ = false; - const Api::IoCallBoolResult result = file_->close(); - ASSERT(result.rc_, fmt::format("unable to close file '{}': {}", file_->path(), - result.err_->getErrorDetails())); - open(); + if (reopen_file_) { + reopen_file_ = false; + const Api::IoCallBoolResult result = file_->close(); + ASSERT(result.rc_, fmt::format("unable to close file '{}': {}", file_->path(), + result.err_->getErrorDetails())); + const Api::IoCallBoolResult open_result = open(); + if (!open_result.rc_) { + stats_.reopen_failed_.inc(); + return; } - - doWrite(about_to_write_buffer_); - } - catch (const EnvoyException&) { - stats_.reopen_failed_.inc(); } + doWrite(about_to_write_buffer_); } } } diff --git a/source/common/access_log/access_log_manager_impl.h b/source/common/access_log/access_log_manager_impl.h index be15a219a910..6a94ccedf275 100644 --- a/source/common/access_log/access_log_manager_impl.h +++ b/source/common/access_log/access_log_manager_impl.h @@ -84,7 +84,7 @@ class AccessLogFileImpl : public AccessLogFile { private: void doWrite(Buffer::Instance& buffer); void flushThreadFunc(); - void open(); + Api::IoCallBoolResult open(); void createFlushStructures(); // return default flags set which used by open diff --git a/source/common/common/thread.cc b/source/common/common/thread.cc index e2eea34c145b..d32907be0676 100644 --- a/source/common/common/thread.cc +++ b/source/common/common/thread.cc @@ -5,11 +5,12 @@ namespace Thread { bool MainThread::isMainThread() { // If threading is off, only main thread is running. - if (MainThreadSingleton::getExisting() == nullptr) { + auto main_thread_singleton = MainThreadSingleton::getExisting(); + if (main_thread_singleton == nullptr) { return true; } // When threading is on, compare thread id with main thread id. - return MainThreadSingleton::get().inMainThread() || MainThreadSingleton::get().inTestThread(); + return main_thread_singleton->inMainThread() || main_thread_singleton->inTestThread(); } void MainThread::clear() { diff --git a/source/common/common/thread.h b/source/common/common/thread.h index d46979c48539..7437fe68b6e4 100644 --- a/source/common/common/thread.h +++ b/source/common/common/thread.h @@ -178,14 +178,26 @@ struct MainThread { void registerTestThread() { test_thread_id_ = std::this_thread::get_id(); } void registerMainThread() { main_thread_id_ = std::this_thread::get_id(); } static bool initialized() { return MainThreadSingleton::getExisting() != nullptr; } + /* + * Reigister the main thread id, should be called in main thread before threading is on. Currently + * called in ThreadLocal::InstanceImpl(). + */ static void initMainThread(); + /* + * Reigister the test thread id, should be called in test thread before threading is on. Allow + * some main thread only code to be executed on test thread. + */ static void initTestThread(); + /* + * Delete the injectable main thread singleton, should be called in main thread after threading + * has been shut down. Currently called in ~ThreadLocal::InstanceImpl(). + */ static void clear(); static bool isMainThread(); private: std::thread::id main_thread_id_; - absl::optional test_thread_id_{}; + absl::optional test_thread_id_; }; // To improve exception safety in data plane, we plan to forbid the use of raw try in the core code From 198d40943f9e12696b3249dde8d879bf81a91935 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Tue, 23 Mar 2021 21:36:44 +0000 Subject: [PATCH 30/41] add todo and comments Signed-off-by: chaoqin-li1123 --- source/common/formatter/substitution_formatter.cc | 1 + source/common/network/dns_impl.cc | 12 ++++++------ source/common/network/io_socket_handle_impl.cc | 1 + source/server/worker_impl.cc | 2 ++ tools/code_format/check_format.py | 8 ++++++-- 5 files changed, 16 insertions(+), 8 deletions(-) diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc index 4a7acb051bf6..5bd34d64e85b 100644 --- a/source/common/formatter/substitution_formatter.cc +++ b/source/common/formatter/substitution_formatter.cc @@ -1295,6 +1295,7 @@ ProtobufWkt::Value FilterStateFormatter::formatValue(const Http::RequestHeaderMa } ProtobufWkt::Value val; + // TODO(chaoqin-li1123): make this conversion return an error status instead of throwing. TRY_NEEDS_AUDIT { MessageUtil::jsonConvertValue(*proto, val); } catch (EnvoyException& ex) { return unspecifiedValue(); diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index a002c563202d..f9571fcd4425 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -169,16 +169,16 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i if (completed_) { if (!cancelled_) { - TRY_NEEDS_AUDIT { callback_(resolution_status, std::move(address_list)); } - catch (const EnvoyException& e) { + // TODO(chaoqin-li1123): remove this exception catching since the process is dying anyway. + try { + callback_(resolution_status, std::move(address_list)); + } catch (const EnvoyException& e) { ENVOY_LOG(critical, "EnvoyException in c-ares callback: {}", e.what()); dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); }); - } - catch (const std::exception& e) { + } catch (const std::exception& e) { ENVOY_LOG(critical, "std::exception in c-ares callback: {}", e.what()); dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); }); - } - catch (...) { + } catch (...) { ENVOY_LOG(critical, "Unknown exception in c-ares callback"); dispatcher_.post([] { throw EnvoyException("unknown"); }); } diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index 151a3d04f0d1..6029501b8df0 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -277,6 +277,7 @@ Api::IoCallUint64Result IoSocketHandleImpl::sendmsg(const Buffer::RawSlice* slic Address::InstanceConstSharedPtr getAddressFromSockAddrOrDie(const sockaddr_storage& ss, socklen_t ss_len, os_fd_t fd) { + // TODO(chaoqin-li1123): remove this exception catching since the process is dying anyway. TRY_NEEDS_AUDIT { // Set v6only to false so that mapped-v6 address can be normalize to v4 // address. Though dual stack may be disabled, it's still okay to assume the diff --git a/source/server/worker_impl.cc b/source/server/worker_impl.cc index 974651b8dbda..bcca70c186f9 100644 --- a/source/server/worker_impl.cc +++ b/source/server/worker_impl.cc @@ -44,6 +44,8 @@ void WorkerImpl::addListener(absl::optional overridden_listener, // bind to an address, but then fail to listen() with `EADDRINUSE`. During initial startup, we // want to surface this. dispatcher_->post([this, overridden_listener, &listener, completion]() -> void { + // TODO(chaoqin-li1123): Make addlistener return a error status instead of catching an + // exception. TRY_NEEDS_AUDIT { handler_->addListener(overridden_listener, listener); hooks_.onWorkerListenerAdded(); diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index e39cc69c1dbe..31e6828fc72d 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -115,8 +115,12 @@ EXCEPTION_DENYLIST = ("./source/common/http/http2/codec_impl.h", "./source/common/http/http2/codec_impl.cc") -RAW_TRY_ALLOWLIST = ("./source/common/common/regex.cc", "./source/common/common/thread.h", - "./source/common/network/utility.cc") +RAW_TRY_ALLOWLIST = ( + "./source/common/common/regex.cc", + "./source/common/common/thread.h", + "./source/common/network/utility.cc", + "./source/common/network/dns_impl.cc", +) # Header files that can throw exceptions. These should be limited; the only # valid situation identified so far is template functions used for config From f5bd51546164a1ba94d4838b8fb629460488375d Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 24 Mar 2021 05:18:13 +0000 Subject: [PATCH 31/41] format Signed-off-by: chaoqin-li1123 --- source/server/worker_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/server/worker_impl.cc b/source/server/worker_impl.cc index bcca70c186f9..68fd72defdb9 100644 --- a/source/server/worker_impl.cc +++ b/source/server/worker_impl.cc @@ -44,7 +44,7 @@ void WorkerImpl::addListener(absl::optional overridden_listener, // bind to an address, but then fail to listen() with `EADDRINUSE`. During initial startup, we // want to surface this. dispatcher_->post([this, overridden_listener, &listener, completion]() -> void { - // TODO(chaoqin-li1123): Make addlistener return a error status instead of catching an + // TODO(chaoqin-li1123): Make add listener return a error status instead of catching an // exception. TRY_NEEDS_AUDIT { handler_->addListener(overridden_listener, listener); From 497ad5d8e66c86752eac92a46146ae5613cc6400 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 24 Mar 2021 21:00:51 +0000 Subject: [PATCH 32/41] add missing test coverage Signed-off-by: chaoqin-li1123 --- .../formatter/substitution_formatter_test.cc | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/common/formatter/substitution_formatter_test.cc b/test/common/formatter/substitution_formatter_test.cc index c367741bae81..9dc1cce5b2f3 100644 --- a/test/common/formatter/substitution_formatter_test.cc +++ b/test/common/formatter/substitution_formatter_test.cc @@ -3,6 +3,7 @@ #include #include +#include "envoy/common/exception.h" #include "envoy/config/core/v3/base.pb.h" #include "envoy/stream_info/stream_info.h" @@ -1351,6 +1352,11 @@ TEST(SubstitutionFormatterTest, FilterStateFormatter) { stream_info.filter_state_->setData("key-no-serialization", std::make_unique(), StreamInfo::FilterState::StateType::ReadOnly); + /* +stream_info.filter_state_->setData("serialized", + std::make_unique(), + StreamInfo::FilterState::StateType::ReadOnly, + StreamInfo::FilterState::LifeSpan::FilterChain);*/ stream_info.filter_state_->setData( "key-serialization-error", std::make_unique(std::chrono::seconds(-281474976710656)), @@ -1625,6 +1631,14 @@ TEST(SubstitutionFormatterTest, GrpcStatusFormatterTest) { formatter.formatValue(request_header, response_header, response_trailer, stream_info, body), ProtoEq(ValueUtil::stringValue(grpc_statuses[i]))); } + { + response_trailer = Http::TestResponseTrailerMapImpl{{"not-a-grpc-status", "13"}}; + EXPECT_EQ(absl::nullopt, formatter.format(request_header, response_header, response_trailer, + stream_info, body)); + EXPECT_THAT( + formatter.formatValue(request_header, response_header, response_trailer, stream_info, body), + ProtoEq(ValueUtil::nullValue())); + } { response_trailer = Http::TestResponseTrailerMapImpl{{"grpc-status", "-1"}}; EXPECT_EQ("-1", formatter.format(request_header, response_header, response_trailer, stream_info, From 670b5b7aba2773aedd21f3155823f4fe985cb112 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Wed, 24 Mar 2021 21:05:32 +0000 Subject: [PATCH 33/41] add missing test coverage Signed-off-by: chaoqin-li1123 --- source/common/protobuf/utility.cc | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index cda179fc505c..0b49ad7d9273 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -624,10 +624,7 @@ std::string MessageUtil::getYamlStringFromMessage(const Protobuf::Message& messa throw EnvoyException(e.what()); } catch (std::exception& e) { - // There is a potentially wide space of exceptions thrown by the YAML parser, - // and enumerating them all may be difficult. Envoy doesn't work well with - // unhandled exceptions, so we capture them and record the exception name in - // the Envoy Exception text. + // Catch unknown YAML parsing exceptions. throw EnvoyException(fmt::format("Unexpected YAML exception: {}", +e.what())); } if (block_print) { From dd7f8c278a982d3a8cf22907ffd2378da7aa872d Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Thu, 25 Mar 2021 01:15:10 +0000 Subject: [PATCH 34/41] fix format Signed-off-by: chaoqin-li1123 --- source/common/common/thread.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/common/thread.h b/source/common/common/thread.h index 7437fe68b6e4..8a7f87bc2fcc 100644 --- a/source/common/common/thread.h +++ b/source/common/common/thread.h @@ -179,12 +179,12 @@ struct MainThread { void registerMainThread() { main_thread_id_ = std::this_thread::get_id(); } static bool initialized() { return MainThreadSingleton::getExisting() != nullptr; } /* - * Reigister the main thread id, should be called in main thread before threading is on. Currently + * Register the main thread id, should be called in main thread before threading is on. Currently * called in ThreadLocal::InstanceImpl(). */ static void initMainThread(); /* - * Reigister the test thread id, should be called in test thread before threading is on. Allow + * Register the test thread id, should be called in test thread before threading is on. Allow * some main thread only code to be executed on test thread. */ static void initTestThread(); From b20f13314409e8934018db2fe59c3a6717a05fcb Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Thu, 25 Mar 2021 05:23:23 +0000 Subject: [PATCH 35/41] fix format Signed-off-by: chaoqin-li1123 --- source/common/common/thread.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/common/thread.h b/source/common/common/thread.h index 8a7f87bc2fcc..e84fb7b38bf7 100644 --- a/source/common/common/thread.h +++ b/source/common/common/thread.h @@ -189,7 +189,7 @@ struct MainThread { */ static void initTestThread(); /* - * Delete the injectable main thread singleton, should be called in main thread after threading + * Delete the main thread singleton, should be called in main thread after threading * has been shut down. Currently called in ~ThreadLocal::InstanceImpl(). */ static void clear(); From fab050c989d59eeca1843ba694f167ec140dabf6 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Thu, 25 Mar 2021 05:29:08 +0000 Subject: [PATCH 36/41] clean up Signed-off-by: chaoqin-li1123 --- test/common/formatter/substitution_formatter_test.cc | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/test/common/formatter/substitution_formatter_test.cc b/test/common/formatter/substitution_formatter_test.cc index 9dc1cce5b2f3..dbec222a08de 100644 --- a/test/common/formatter/substitution_formatter_test.cc +++ b/test/common/formatter/substitution_formatter_test.cc @@ -1352,11 +1352,7 @@ TEST(SubstitutionFormatterTest, FilterStateFormatter) { stream_info.filter_state_->setData("key-no-serialization", std::make_unique(), StreamInfo::FilterState::StateType::ReadOnly); - /* -stream_info.filter_state_->setData("serialized", - std::make_unique(), - StreamInfo::FilterState::StateType::ReadOnly, - StreamInfo::FilterState::LifeSpan::FilterChain);*/ + stream_info.filter_state_->setData( "key-serialization-error", std::make_unique(std::chrono::seconds(-281474976710656)), From d14310d9348cf64e1f3d845d551bdcb2fc77ae84 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Thu, 25 Mar 2021 17:12:14 +0000 Subject: [PATCH 37/41] add test coverage Signed-off-by: chaoqin-li1123 --- test/common/protobuf/utility_test.cc | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index b548c9267c18..7a6dc637b36d 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -1201,7 +1201,7 @@ TEST_F(ProtobufUtilityTest, ValueUtilLoadFromYamlObject) { "struct_value { fields { key: \"foo\" value { string_value: \"bar\" } } }"); } -TEST_F(ProtobufUtilityTest, ValueUtilLoadFromYamlException) { +TEST(LoadFromYamlExceptionTest, BadConversion) { std::string bad_yaml = R"EOF( admin: access_log_path: /dev/null @@ -1216,6 +1216,20 @@ TEST_F(ProtobufUtilityTest, ValueUtilLoadFromYamlException) { "Unexpected YAML exception"); } +TEST(LoadFromYamlExceptionTest, ParserException) { + std::string bad_yaml = R"EOF( +systemLog: + destination: file + path:"G:\file\path" +storage: + dbPath:"G:\db\data" +)EOF"; + + EXPECT_THROW_WITH_REGEX(ValueUtil::loadFromYaml(bad_yaml), EnvoyException, "illegal map value"); + EXPECT_THROW_WITHOUT_REGEX(ValueUtil::loadFromYaml(bad_yaml), EnvoyException, + "Unexpected YAML exception"); +} + TEST_F(ProtobufUtilityTest, HashedValue) { ProtobufWkt::Value v1, v2, v3; v1.set_string_value("s"); From b32053acd24f9d969e4a0d1a3ec05c0c26d842b3 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Thu, 25 Mar 2021 22:52:41 +0000 Subject: [PATCH 38/41] fix comment Signed-off-by: chaoqin-li1123 --- source/common/formatter/substitution_formatter.cc | 2 ++ source/common/network/io_socket_handle_impl.cc | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc index 5bd34d64e85b..b8cf77b48573 100644 --- a/source/common/formatter/substitution_formatter.cc +++ b/source/common/formatter/substitution_formatter.cc @@ -1296,6 +1296,8 @@ ProtobufWkt::Value FilterStateFormatter::formatValue(const Http::RequestHeaderMa ProtobufWkt::Value val; // TODO(chaoqin-li1123): make this conversion return an error status instead of throwing. + // Access logger conversion from protobufs occurs via json intermediate state, which can throw + // when converting that to a structure. TRY_NEEDS_AUDIT { MessageUtil::jsonConvertValue(*proto, val); } catch (EnvoyException& ex) { return unspecifiedValue(); diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index 6029501b8df0..b8bd7f2ac1c9 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -277,7 +277,8 @@ Api::IoCallUint64Result IoSocketHandleImpl::sendmsg(const Buffer::RawSlice* slic Address::InstanceConstSharedPtr getAddressFromSockAddrOrDie(const sockaddr_storage& ss, socklen_t ss_len, os_fd_t fd) { - // TODO(chaoqin-li1123): remove this exception catching since the process is dying anyway. + // TODO(chaoqin-li1123): remove exception catching and make Address::addressFromSockAddr return + // null on error. TRY_NEEDS_AUDIT { // Set v6only to false so that mapped-v6 address can be normalize to v4 // address. Though dual stack may be disabled, it's still okay to assume the From 9ed6baa9b4d7073118329f5e6da6909170827c21 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Mon, 29 Mar 2021 03:51:30 +0000 Subject: [PATCH 39/41] clean up Signed-off-by: chaoqin-li1123 --- source/common/http/utility.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index b80c33cd651f..ea2af7f05075 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -634,8 +634,8 @@ Utility::getLastAddressFromXFF(const Http::RequestHeaderMap& request_headers, // practice, we are working with a view at the end of the owning string, and could pass the // raw pointer. // TODO(mattklein123) PERF: Avoid the copy here. - Network::Address::InstanceConstSharedPtr address = Network::Utility::parseInternetAddressNoThrow( - std::string(xff_string.data(), xff_string.size())); + Network::Address::InstanceConstSharedPtr address = + Network::Utility::parseInternetAddressNoThrow(std::string(xff_string)); if (address != nullptr) { return {address, last_comma == std::string::npos && num_to_skip == 0}; } From a2716ac5c15a21b72d653cdc526b9f020f444ec7 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Mon, 29 Mar 2021 17:37:30 +0000 Subject: [PATCH 40/41] fix comment Signed-off-by: chaoqin-li1123 --- source/common/network/dns_impl.cc | 15 +++++++++------ tools/code_format/check_format.py | 1 - 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index f9571fcd4425..9f6e8bcbc22d 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -169,16 +169,19 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i if (completed_) { if (!cancelled_) { - // TODO(chaoqin-li1123): remove this exception catching since the process is dying anyway. - try { - callback_(resolution_status, std::move(address_list)); - } catch (const EnvoyException& e) { + // TODO(chaoqin-li1123): remove this exception catching by refactoring. + // We can add a main thread assertion here because both this code is reused by dns filter and + // executed in both main thread and worker thread. + TRY_NEEDS_AUDIT { callback_(resolution_status, std::move(address_list)); } + catch (const EnvoyException& e) { ENVOY_LOG(critical, "EnvoyException in c-ares callback: {}", e.what()); dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); }); - } catch (const std::exception& e) { + } + catch (const std::exception& e) { ENVOY_LOG(critical, "std::exception in c-ares callback: {}", e.what()); dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); }); - } catch (...) { + } + catch (...) { ENVOY_LOG(critical, "Unknown exception in c-ares callback"); dispatcher_.post([] { throw EnvoyException("unknown"); }); } diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index 31e6828fc72d..167a266bdaf5 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -119,7 +119,6 @@ "./source/common/common/regex.cc", "./source/common/common/thread.h", "./source/common/network/utility.cc", - "./source/common/network/dns_impl.cc", ) # Header files that can throw exceptions. These should be limited; the only From afc6e1d43b6d669d70b64f8c8fa742dc2ae9d5b0 Mon Sep 17 00:00:00 2001 From: chaoqin-li1123 Date: Mon, 29 Mar 2021 19:46:33 +0000 Subject: [PATCH 41/41] fix comment Signed-off-by: chaoqin-li1123 --- source/common/network/dns_impl.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index 9f6e8bcbc22d..c2b0411a9696 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -170,8 +170,9 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i if (completed_) { if (!cancelled_) { // TODO(chaoqin-li1123): remove this exception catching by refactoring. - // We can add a main thread assertion here because both this code is reused by dns filter and - // executed in both main thread and worker thread. + // We can't add a main thread assertion here because both this code is reused by dns filter + // and executed in both main thread and worker thread. Maybe split the code for filter and + // main thread. TRY_NEEDS_AUDIT { callback_(resolution_status, std::move(address_list)); } catch (const EnvoyException& e) { ENVOY_LOG(critical, "EnvoyException in c-ares callback: {}", e.what());