Uniquify channel args keys (#28799 An error occurred )

Ensures only one value for each key in channel args... may cause breakage for some usages where two values are being passed in a channel_arg and whatever arbitrary order we were reading them was used. Workarounds are in place for things we know about.

src/core/lib/channel/channel_args.cc

@@ -23,6 +23,7 @@
 #include <limits.h>
 #include <string.h>
 
+#include <map>
 #include <vector>
 
 #include "absl/strings/match.h"
@@ -382,6 +383,65 @@ const grpc_channel_args* RemoveGrpcInternalArgs(const grpc_channel_args* src) {
   }
   return dst;
 }
+
+const grpc_channel_args* UniquifyChannelArgKeys(const grpc_channel_args* src) {
+  if (src == nullptr) return nullptr;
+  std::map<absl::string_view, const grpc_arg*> values;
+  std::map<absl::string_view, std::vector<std::string>> concatenated_values;
+  for (size_t i = 0; i < src->num_args; i++) {
+    absl::string_view key = src->args[i].key;
+    // User-agent strings were traditionally multi-valued and concatenated.
+    // We preserve this behavior for backwards compatibility.
+    if (key == GRPC_ARG_PRIMARY_USER_AGENT_STRING ||
+        key == GRPC_ARG_SECONDARY_USER_AGENT_STRING) {
+      if (src->args[i].type != GRPC_ARG_STRING) {
+        gpr_log(GPR_ERROR, "Channel argument '%s' should be a string",
+                std::string(key).c_str());
+      } else {
+        concatenated_values[key].push_back(src->args[i].value.string);
+      }
+      continue;
+    }
+    auto it = values.find(key);
+    if (it == values.end()) {
+      values[key] = &src->args[i];
+    } else {
+      // Traditional grpc_channel_args_find behavior was to pick the first
+      // value.
+      // For compatibility with existing users, we will do the same here.
+    }
+  }
+  if (values.size() + concatenated_values.size() == src->num_args) {
+    return grpc_channel_args_copy(src);
+  }
+  // Concatenate the concatenated values.
+  std::map<absl::string_view, std::string> concatenated_values_str;
+  for (const auto& concatenated_value : concatenated_values) {
+    concatenated_values_str[concatenated_value.first] =
+        absl::StrJoin(concatenated_value.second, " ");
+  }
+  // Create the result
+  std::vector<grpc_arg> argv;
+  argv.reserve(values.size());
+  for (const auto& a : values) {
+    argv.push_back(*a.second);
+  }
+  for (const auto& a : concatenated_values_str) {
+    argv.push_back(
+        grpc_channel_arg_string_create(const_cast<char*>(a.first.data()),
+                                       const_cast<char*>(a.second.c_str())));
+  }
+  grpc_channel_args args = {argv.size(), argv.data()};
+  // Log that we're mutating things
+  gpr_log(GPR_INFO,
+          "Uniquification pass on channel args is mutating them: {%s} is being "
+          "changed to {%s}",
+          grpc_channel_args_string(src).c_str(),
+          grpc_channel_args_string(&args).c_str());
+  // Return the result (note we need to copy because we're borrowing the args
+  // from src still!)
+  return grpc_channel_args_copy(&args);
+}
 }  // namespace grpc_core
 
 namespace {

src/core/lib/channel/channel_args.h

@@ -127,6 +127,8 @@ namespace grpc_core {
  *  Does not take ownership of \a src.
  *  Should be called by any public API that receives channel args. */
 const grpc_channel_args* RemoveGrpcInternalArgs(const grpc_channel_args* src);
+/** Ensure no duplicate channel args, in preparation for moving to a map<> */
+const grpc_channel_args* UniquifyChannelArgKeys(const grpc_channel_args* src);
 }  // namespace grpc_core
 
 // Takes ownership of the old_args

src/core/lib/channel/channel_args_preconditioning.cc

@@ -27,6 +27,7 @@ void ChannelArgsPreconditioning::Builder::RegisterStage(Stage stage) {
 ChannelArgsPreconditioning ChannelArgsPreconditioning::Builder::Build() {
   // TODO(ctiller): should probably make this registered too.
   stages_.emplace_back(RemoveGrpcInternalArgs);
+  stages_.emplace_back(UniquifyChannelArgKeys);
 
   ChannelArgsPreconditioning preconditioning;
   preconditioning.stages_ = std::move(stages_);