Reviewer comments
diff --git a/src/core/ext/filters/client_channel/client_channel_plugin.cc b/src/core/ext/filters/client_channel/client_channel_plugin.cc
index 3d6fa2e..8e76c4c 100644
--- a/src/core/ext/filters/client_channel/client_channel_plugin.cc
+++ b/src/core/ext/filters/client_channel/client_channel_plugin.cc
@@ -49,7 +49,7 @@
}
void grpc_client_channel_init(void) {
- grpc_core::ServiceConfig::ResetServiceConfigParsers();
+ grpc_core::ServiceConfig::Init();
grpc_core::LoadBalancingPolicyRegistry::Builder::InitRegistry();
grpc_core::ResolverRegistry::Builder::InitRegistry();
grpc_core::internal::ServerRetryThrottleMap::Init();
@@ -69,5 +69,5 @@
grpc_core::internal::ServerRetryThrottleMap::Shutdown();
grpc_core::ResolverRegistry::Builder::ShutdownRegistry();
grpc_core::LoadBalancingPolicyRegistry::Builder::ShutdownRegistry();
- grpc_core::ServiceConfig::ResetServiceConfigParsers();
+ grpc_core::ServiceConfig::Shutdown();
}
diff --git a/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc b/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc
index 7de1c22..7ce7067 100644
--- a/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc
+++ b/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc
@@ -308,7 +308,8 @@
if (service_config_string != nullptr) {
GRPC_CARES_TRACE_LOG("resolver:%p selected service config choice: %s",
r, service_config_string);
- result.service_config = ServiceConfig::Create(service_config_string);
+ result.service_config =
+ ServiceConfig::Create(service_config_string, nullptr);
}
gpr_free(service_config_string);
}
diff --git a/src/core/ext/filters/client_channel/resolver_result_parsing.cc b/src/core/ext/filters/client_channel/resolver_result_parsing.cc
index daac4f0..f523ffe 100644
--- a/src/core/ext/filters/client_channel/resolver_result_parsing.cc
+++ b/src/core/ext/filters/client_channel/resolver_result_parsing.cc
@@ -52,7 +52,7 @@
const char* service_config_json = grpc_channel_arg_get_string(
grpc_channel_args_find(resolver_result->args, GRPC_ARG_SERVICE_CONFIG));
if (service_config_json != nullptr) {
- service_config_ = ServiceConfig::Create(service_config_json);
+ service_config_ = ServiceConfig::Create(service_config_json, nullptr);
}
} else {
// Add the service config JSON to channel args so that it's
diff --git a/src/core/ext/filters/client_channel/service_config.cc b/src/core/ext/filters/client_channel/service_config.cc
index f47b170..fbeb578 100644
--- a/src/core/ext/filters/client_channel/service_config.cc
+++ b/src/core/ext/filters/client_channel/service_config.cc
@@ -33,11 +33,11 @@
namespace grpc_core {
-int ServiceConfig::registered_parsers_count_ = 0;
-ServiceConfigParser
- ServiceConfig::registered_parsers_[ServiceConfigParser::kMaxParsers];
+ServiceConfig::ServiceConfigParserList* ServiceConfig::registered_parsers_ =
+ nullptr;
-RefCountedPtr<ServiceConfig> ServiceConfig::Create(const char* json) {
+RefCountedPtr<ServiceConfig> ServiceConfig::Create(const char* json,
+ grpc_error** error) {
UniquePtr<char> service_config_json(gpr_strdup(json));
UniquePtr<char> json_string(gpr_strdup(json));
grpc_json* json_tree = grpc_json_parse_string(json_string.get());
@@ -45,137 +45,153 @@
gpr_log(GPR_INFO, "failed to parse JSON for service config");
return nullptr;
}
- bool success;
+
+ grpc_error* create_error;
auto return_value = MakeRefCounted<ServiceConfig>(
std::move(service_config_json), std::move(json_string), json_tree,
- &success);
-
- return success ? return_value : nullptr;
+ &create_error);
+ if (create_error != GRPC_ERROR_NONE) {
+ if (error != nullptr) {
+ *error = create_error;
+ } else {
+ GRPC_ERROR_UNREF(create_error);
+ }
+ return nullptr;
+ }
+ if (error != nullptr) {
+ *error = GRPC_ERROR_NONE;
+ }
+ return return_value;
}
ServiceConfig::ServiceConfig(UniquePtr<char> service_config_json,
UniquePtr<char> json_string, grpc_json* json_tree,
- bool* success)
+ grpc_error** error)
: service_config_json_(std::move(service_config_json)),
json_string_(std::move(json_string)),
json_tree_(json_tree) {
- GPR_DEBUG_ASSERT(success != nullptr);
if (json_tree->type != GRPC_JSON_OBJECT || json_tree->key != nullptr) {
- *success = false;
+ if (error != nullptr) {
+ *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+ "Malformed service Config JSON object");
+ }
return;
}
- ParseGlobalParams(json_tree, success);
- if (!*success) {
- return;
+ grpc_error* parser_error = ParseGlobalParams(json_tree);
+ if (parser_error != GRPC_ERROR_NONE) {
+ parser_error = ParsePerMethodParams(json_tree);
}
- ParsePerMethodParams(json_tree, success);
- if (!*success) {
- return;
+ if (error != nullptr) {
+ *error = parser_error;
+ } else {
+ GRPC_ERROR_UNREF(parser_error);
}
}
-void ServiceConfig::ParseGlobalParams(const grpc_json* json_tree,
- bool* success) {
- GPR_DEBUG_ASSERT(success != nullptr);
+grpc_error* ServiceConfig::ParseGlobalParams(const grpc_json* json_tree) {
GPR_DEBUG_ASSERT(json_tree_->type == GRPC_JSON_OBJECT);
GPR_DEBUG_ASSERT(json_tree_->key == nullptr);
- for (auto i = 0; i < registered_parsers_count_; i++) {
+ for (size_t i = 0; i < registered_parsers_->size(); i++) {
+ grpc_error* error;
auto parsed_obj =
- registered_parsers_[i].ParseGlobalParams(json_tree, success);
- if (!*success) {
- return;
+ (*registered_parsers_)[i].ParseGlobalParams(json_tree, &error);
+ if (error != GRPC_ERROR_NONE) {
+ return error;
}
- parsed_global_service_config_objects_.push_back(parsed_obj);
+ parsed_global_service_config_objects_.push_back(std::move(parsed_obj));
}
- *success = true;
+ return GRPC_ERROR_NONE;
}
-bool ServiceConfig::ParseJsonMethodConfigToServiceConfigObjectsTable(
+grpc_error* ServiceConfig::ParseJsonMethodConfigToServiceConfigObjectsTable(
const grpc_json* json,
- SliceHashTable<RefCountedPtr<ServiceConfigObjectsVector>>::Entry* entries,
+ SliceHashTable<const ServiceConfigObjectsVector*>::Entry* entries,
size_t* idx) {
- auto objs_vector = MakeRefCounted<ServiceConfigObjectsVector>();
- for (auto i = 0; i < registered_parsers_count_; i++) {
- bool success;
+ auto objs_vector = MakeUnique<ServiceConfigObjectsVector>();
+ for (size_t i = 0; i < registered_parsers_->size(); i++) {
+ grpc_error* error;
auto parsed_obj =
- registered_parsers_[i].ParsePerMethodParams(json, &success);
- if (!success) {
- return false;
+ (*registered_parsers_)[i].ParsePerMethodParams(json, &error);
+ if (error != GRPC_ERROR_NONE) {
+ return error;
}
- objs_vector->vector.push_back(parsed_obj);
+ objs_vector->push_back(std::move(parsed_obj));
}
+ const auto* vector_ptr = objs_vector.get();
+ service_config_objects_vectors_storage_.push_back(std::move(objs_vector));
// Construct list of paths.
InlinedVector<UniquePtr<char>, 10> paths;
for (grpc_json* child = json->child; child != nullptr; child = child->next) {
if (child->key == nullptr) continue;
if (strcmp(child->key, "name") == 0) {
- if (child->type != GRPC_JSON_ARRAY) return false;
+ if (child->type != GRPC_JSON_ARRAY) {
+ return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+ "name should be of type Array");
+ }
for (grpc_json* name = child->child; name != nullptr; name = name->next) {
UniquePtr<char> path = ParseJsonMethodName(name);
- if (path == nullptr) return false;
+ if (path == nullptr) {
+ return GRPC_ERROR_CREATE_FROM_STATIC_STRING("Failed to parse name");
+ }
paths.push_back(std::move(path));
}
}
}
- if (paths.size() == 0) return false; // No names specified.
+ if (paths.size() == 0) {
+ return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+ "No names specified in methodConfig");
+ }
// Add entry for each path.
for (size_t i = 0; i < paths.size(); ++i) {
entries[*idx].key = grpc_slice_from_copied_string(paths[i].get());
- entries[*idx].value = objs_vector; // Takes a new ref.
+ entries[*idx].value = vector_ptr; // Takes a new ref.
++*idx;
}
- return true;
+ return GRPC_ERROR_NONE;
}
-void ServiceConfig::ParsePerMethodParams(const grpc_json* json_tree,
- bool* success) {
- GPR_DEBUG_ASSERT(success != nullptr);
+grpc_error* ServiceConfig::ParsePerMethodParams(const grpc_json* json_tree) {
GPR_DEBUG_ASSERT(json_tree_->type == GRPC_JSON_OBJECT);
GPR_DEBUG_ASSERT(json_tree_->key == nullptr);
- SliceHashTable<RefCountedPtr<ServiceConfigObjectsVector>>::Entry* entries =
- nullptr;
+ SliceHashTable<const ServiceConfigObjectsVector*>::Entry* entries = nullptr;
size_t num_entries = 0;
for (grpc_json* field = json_tree->child; field != nullptr;
field = field->next) {
if (field->key == nullptr) {
- *success = false;
- return;
+ return GRPC_ERROR_CREATE_FROM_STATIC_STRING("Illegal key value - NULL");
}
if (strcmp(field->key, "methodConfig") == 0) {
if (entries != nullptr) {
GPR_ASSERT(false);
}
if (field->type != GRPC_JSON_ARRAY) {
- *success = false;
- return;
+ return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+ "methodConfig is not of type Array");
}
for (grpc_json* method = field->child; method != nullptr;
method = method->next) {
int count = CountNamesInMethodConfig(method);
if (count <= 0) {
- *success = false;
- return;
+ return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+ "No names found in methodConfig");
}
num_entries += static_cast<size_t>(count);
}
entries = static_cast<
- SliceHashTable<RefCountedPtr<ServiceConfigObjectsVector>>::Entry*>(
- gpr_zalloc(
- num_entries *
- sizeof(SliceHashTable<
- RefCountedPtr<ServiceConfigObjectsVector>>::Entry)));
+ SliceHashTable<const ServiceConfigObjectsVector*>::Entry*>(gpr_zalloc(
+ num_entries *
+ sizeof(SliceHashTable<const ServiceConfigObjectsVector*>::Entry)));
size_t idx = 0;
for (grpc_json* method = field->child; method != nullptr;
method = method->next) {
- if (!ParseJsonMethodConfigToServiceConfigObjectsTable(method, entries,
- &idx)) {
+ grpc_error* error = ParseJsonMethodConfigToServiceConfigObjectsTable(
+ method, entries, &idx);
+ if (error != GRPC_ERROR_NONE) {
for (size_t i = 0; i < idx; ++i) {
grpc_slice_unref_internal(entries[i].key);
- entries[i].value.reset();
}
gpr_free(entries);
- *success = false;
- return;
+ return error;
}
}
GPR_DEBUG_ASSERT(idx == num_entries);
@@ -184,11 +200,11 @@
}
if (entries != nullptr) {
parsed_method_service_config_objects_table_ =
- SliceHashTable<RefCountedPtr<ServiceConfigObjectsVector>>::Create(
+ SliceHashTable<const ServiceConfigObjectsVector*>::Create(
num_entries, entries, nullptr);
gpr_free(entries);
}
- *success = true;
+ return GRPC_ERROR_NONE;
}
ServiceConfig::~ServiceConfig() { grpc_json_destroy(json_tree_); }
@@ -248,4 +264,27 @@
return UniquePtr<char>(path);
}
+const ServiceConfig::ServiceConfigObjectsVector* const*
+ServiceConfig::GetMethodServiceConfigObjectsVector(const grpc_slice& path) {
+ const auto* value = parsed_method_service_config_objects_table_->Get(path);
+ // If we didn't find a match for the path, try looking for a wildcard
+ // entry (i.e., change "/service/method" to "/service/*").
+ if (value == nullptr) {
+ char* path_str = grpc_slice_to_c_string(path);
+ const char* sep = strrchr(path_str, '/') + 1;
+ const size_t len = (size_t)(sep - path_str);
+ char* buf = (char*)gpr_malloc(len + 2); // '*' and NUL
+ memcpy(buf, path_str, len);
+ buf[len] = '*';
+ buf[len + 1] = '\0';
+ grpc_slice wildcard_path = grpc_slice_from_copied_string(buf);
+ gpr_free(buf);
+ value = parsed_method_service_config_objects_table_->Get(wildcard_path);
+ grpc_slice_unref_internal(wildcard_path);
+ gpr_free(path_str);
+ if (value == nullptr) return nullptr;
+ }
+ return value;
+}
+
} // namespace grpc_core
diff --git a/src/core/ext/filters/client_channel/service_config.h b/src/core/ext/filters/client_channel/service_config.h
index 9752bf7..90f1a2d 100644
--- a/src/core/ext/filters/client_channel/service_config.h
+++ b/src/core/ext/filters/client_channel/service_config.h
@@ -25,6 +25,7 @@
#include "src/core/lib/gprpp/inlined_vector.h"
#include "src/core/lib/gprpp/ref_counted.h"
#include "src/core/lib/gprpp/ref_counted_ptr.h"
+#include "src/core/lib/iomgr/error.h"
#include "src/core/lib/json/json.h"
#include "src/core/lib/slice/slice_hash_table.h"
@@ -56,11 +57,9 @@
/// This is the base class that all service config parsers MUST use to store
/// parsed service config data.
-class ServiceConfigParsedObject : public RefCounted<ServiceConfigParsedObject> {
+class ServiceConfigParsedObject {
public:
virtual ~ServiceConfigParsedObject() = default;
-
- GRPC_ABSTRACT_BASE_CLASS;
};
/// This is the base class that all service config parsers should derive from.
@@ -68,40 +67,31 @@
public:
virtual ~ServiceConfigParser() = default;
- virtual RefCountedPtr<ServiceConfigParsedObject> ParseGlobalParams(
- const grpc_json* json, bool* success) {
- if (success != nullptr) {
- *success = true;
+ virtual UniquePtr<ServiceConfigParsedObject> ParseGlobalParams(
+ const grpc_json* json, grpc_error** error) {
+ if (error != nullptr) {
+ *error = GRPC_ERROR_NONE;
}
return nullptr;
}
- virtual RefCountedPtr<ServiceConfigParsedObject> ParsePerMethodParams(
- const grpc_json* json, bool* success) {
- if (success != nullptr) {
- *success = true;
+ virtual UniquePtr<ServiceConfigParsedObject> ParsePerMethodParams(
+ const grpc_json* json, grpc_error** error) {
+ if (error != nullptr) {
+ *error = GRPC_ERROR_NONE;
}
return nullptr;
}
static constexpr int kMaxParsers = 32;
-
- GRPC_ABSTRACT_BASE_CLASS;
-};
-
-class ServiceConfigObjectsVector
- : public RefCounted<ServiceConfigObjectsVector> {
- public:
- grpc_core::InlinedVector<RefCountedPtr<ServiceConfigParsedObject>,
- ServiceConfigParser::kMaxParsers>
- vector;
};
class ServiceConfig : public RefCounted<ServiceConfig> {
public:
/// Creates a new service config from parsing \a json_string.
/// Returns null on parse error.
- static RefCountedPtr<ServiceConfig> Create(const char* json);
+ static RefCountedPtr<ServiceConfig> Create(const char* json,
+ grpc_error** error);
~ServiceConfig();
@@ -141,34 +131,19 @@
/// Retrieves the parsed global service config object at index \a index.
ServiceConfigParsedObject* GetParsedGlobalServiceConfigObject(int index) {
- GPR_DEBUG_ASSERT(index < registered_parsers_count_);
+ GPR_DEBUG_ASSERT(
+ index < static_cast<int>(parsed_global_service_config_objects_.size()));
return parsed_global_service_config_objects_[index].get();
}
+ static constexpr int kMaxParsers = 32;
+ typedef InlinedVector<UniquePtr<ServiceConfigParsedObject>, kMaxParsers>
+ ServiceConfigObjectsVector;
+
/// Retrieves the vector of method service config objects for a given path \a
/// path.
- const RefCountedPtr<ServiceConfigObjectsVector>*
- GetMethodServiceConfigObjectsVector(const grpc_slice& path) {
- const auto* value = parsed_method_service_config_objects_table_->Get(path);
- // If we didn't find a match for the path, try looking for a wildcard
- // entry (i.e., change "/service/method" to "/service/*").
- if (value == nullptr) {
- char* path_str = grpc_slice_to_c_string(path);
- const char* sep = strrchr(path_str, '/') + 1;
- const size_t len = (size_t)(sep - path_str);
- char* buf = (char*)gpr_malloc(len + 2); // '*' and NUL
- memcpy(buf, path_str, len);
- buf[len] = '*';
- buf[len + 1] = '\0';
- grpc_slice wildcard_path = grpc_slice_from_copied_string(buf);
- gpr_free(buf);
- value = parsed_method_service_config_objects_table_->Get(wildcard_path);
- grpc_slice_unref_internal(wildcard_path);
- gpr_free(path_str);
- if (value == nullptr) return nullptr;
- }
- return value;
- }
+ const ServiceConfigObjectsVector* const* GetMethodServiceConfigObjectsVector(
+ const grpc_slice& path);
/// Globally register a service config parser. On successful registration, it
/// returns the index at which the parser was registered. On failure, -1 is
@@ -177,12 +152,22 @@
/// config json and returning a parsed object. This parsed object can later be
/// retrieved using the same index that was returned at registration time.
static int RegisterParser(const ServiceConfigParser& parser) {
- registered_parsers_[registered_parsers_count_] = parser;
- return registered_parsers_count_++;
+ registered_parsers_->push_back(parser);
+ return registered_parsers_->size() - 1;
}
- /// Should be called on initialization and shutdown.
- static void ResetServiceConfigParsers() { registered_parsers_count_ = 0; }
+ typedef InlinedVector<ServiceConfigParser, kMaxParsers>
+ ServiceConfigParserList;
+
+ static void Init() {
+ GPR_ASSERT(registered_parsers_ == nullptr);
+ registered_parsers_ = New<ServiceConfigParserList>();
+ }
+
+ static void Shutdown() {
+ Delete(registered_parsers_);
+ registered_parsers_ = nullptr;
+ }
private:
// So New() can call our private ctor.
@@ -192,11 +177,11 @@
// Takes ownership of \a json_tree.
ServiceConfig(UniquePtr<char> service_config_json,
UniquePtr<char> json_string, grpc_json* json_tree,
- bool* success);
+ grpc_error** error);
// Helper functions to parse the service config
- void ParseGlobalParams(const grpc_json* json_tree, bool* success);
- void ParsePerMethodParams(const grpc_json* json_tree, bool* success);
+ grpc_error* ParseGlobalParams(const grpc_json* json_tree);
+ grpc_error* ParsePerMethodParams(const grpc_json* json_tree);
// Returns the number of names specified in the method config \a json.
static int CountNamesInMethodConfig(grpc_json* json);
@@ -213,24 +198,23 @@
grpc_json* json, CreateValue<T> create_value,
typename SliceHashTable<RefCountedPtr<T>>::Entry* entries, size_t* idx);
- static bool ParseJsonMethodConfigToServiceConfigObjectsTable(
+ grpc_error* ParseJsonMethodConfigToServiceConfigObjectsTable(
const grpc_json* json,
- SliceHashTable<RefCountedPtr<ServiceConfigObjectsVector>>::Entry* entries,
+ SliceHashTable<const ServiceConfigObjectsVector*>::Entry* entries,
size_t* idx);
- static int registered_parsers_count_;
- static ServiceConfigParser
- registered_parsers_[ServiceConfigParser::kMaxParsers];
+ static ServiceConfigParserList* registered_parsers_;
UniquePtr<char> service_config_json_;
UniquePtr<char> json_string_; // Underlying storage for json_tree.
grpc_json* json_tree_;
- InlinedVector<RefCountedPtr<ServiceConfigParsedObject>,
- ServiceConfigParser::kMaxParsers>
+ InlinedVector<UniquePtr<ServiceConfigParsedObject>, kMaxParsers>
parsed_global_service_config_objects_;
- RefCountedPtr<SliceHashTable<RefCountedPtr<ServiceConfigObjectsVector>>>
+ RefCountedPtr<SliceHashTable<const ServiceConfigObjectsVector*>>
parsed_method_service_config_objects_table_;
+ InlinedVector<UniquePtr<ServiceConfigObjectsVector>, kMaxParsers>
+ service_config_objects_vectors_storage_;
};
//
diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc
index 8bb0c4c..d66779e 100644
--- a/src/core/ext/filters/client_channel/subchannel.cc
+++ b/src/core/ext/filters/client_channel/subchannel.cc
@@ -591,7 +591,7 @@
grpc_channel_args_find(args_, GRPC_ARG_SERVICE_CONFIG));
if (service_config_json != nullptr) {
RefCountedPtr<ServiceConfig> service_config =
- ServiceConfig::Create(service_config_json);
+ ServiceConfig::Create(service_config_json, nullptr);
if (service_config != nullptr) {
HealthCheckParams params;
service_config->ParseGlobalParams(HealthCheckParams::Parse, ¶ms);
diff --git a/src/core/ext/filters/message_size/message_size_filter.cc b/src/core/ext/filters/message_size/message_size_filter.cc
index 8a422dd..8c2a32c 100644
--- a/src/core/ext/filters/message_size/message_size_filter.cc
+++ b/src/core/ext/filters/message_size/message_size_filter.cc
@@ -320,7 +320,7 @@
const char* service_config_str = grpc_channel_arg_get_string(channel_arg);
if (service_config_str != nullptr) {
grpc_core::RefCountedPtr<grpc_core::ServiceConfig> service_config =
- grpc_core::ServiceConfig::Create(service_config_str);
+ grpc_core::ServiceConfig::Create(service_config_str, nullptr);
if (service_config != nullptr) {
chand->method_limit_table = service_config->CreateMethodConfigTable(
grpc_core::MessageSizeLimits::CreateFromJson);
