Merge branch 'master' of https://github.com/grpc/grpc into channelz-subchannels

6 files changed, 91 insertions, 40 deletions

diff --git a/include/grpc/grpc_security_constants.h b/include/grpc/grpc_security_constants.h
index 944a1e927f..f935557f2d 100644
--- a/include/grpc/grpc_security_constants.h
+++ b/include/grpc/grpc_security_constants.h
@@ -57,46 +57,51 @@ typedef enum {
 } grpc_ssl_certificate_config_reload_status;
 
 typedef enum {
-  /** Server does not request client certificate. A client can present a self
-     signed or signed certificates if it wishes to do so and they would be
-     accepted. */
+  /** Server does not request client certificate.
+     The certificate presented by the client is not checked by the server at
+     all. (A client may present a self signed or signed certificate or not
+     present a certificate at all and any of those option would be accepted) */
   GRPC_SSL_DONT_REQUEST_CLIENT_CERTIFICATE,
   /** Server requests client certificate but does not enforce that the client
      presents a certificate.
 
      If the client presents a certificate, the client authentication is left to
-     the application based on the metadata like certificate etc.
+     the application (the necessary metadata will be available to the
+     application via authentication context properties, see grpc_auth_context).
 
-     The key cert pair should still be valid for the SSL connection to be
-     established. */
+     The client's key certificate pair must be valid for the SSL connection to
+     be established. */
   GRPC_SSL_REQUEST_CLIENT_CERTIFICATE_BUT_DONT_VERIFY,
   /** Server requests client certificate but does not enforce that the client
      presents a certificate.
 
      If the client presents a certificate, the client authentication is done by
-     grpc framework (The client needs to either present a signed cert or skip no
-     certificate for a successful connection).
+     the gRPC framework. (For a successful connection the client needs to either
+     present a certificate that can be verified against the root certificate
+     configured by the server or not present a certificate at all)
 
-     The key cert pair should still be valid for the SSL connection to be
-     established. */
+     The client's key certificate pair must be valid for the SSL connection to
+     be established. */
   GRPC_SSL_REQUEST_CLIENT_CERTIFICATE_AND_VERIFY,
-  /** Server requests client certificate but enforces that the client presents a
+  /** Server requests client certificate and enforces that the client presents a
      certificate.
 
      If the client presents a certificate, the client authentication is left to
-     the application based on the metadata like certificate etc.
+     the application (the necessary metadata will be available to the
+     application via authentication context properties, see grpc_auth_context).
 
-     The key cert pair should still be valid for the SSL connection to be
-     established. */
+     The client's key certificate pair must be valid for the SSL connection to
+     be established. */
   GRPC_SSL_REQUEST_AND_REQUIRE_CLIENT_CERTIFICATE_BUT_DONT_VERIFY,
-  /** Server requests client certificate but enforces that the client presents a
+  /** Server requests client certificate and enforces that the client presents a
      certificate.
 
-     The cerificate presented by the client is verified by grpc framework (The
-     client needs to present signed certs for a successful connection).
+     The cerificate presented by the client is verified by the gRPC framework.
+     (For a successful connection the client needs to present a certificate that
+     can be verified against the root certificate configured by the server)
 
-     The key cert pair should still be valid for the SSL connection to be
-     established. */
+     The client's key certificate pair must be valid for the SSL connection to
+     be established. */
   GRPC_SSL_REQUEST_AND_REQUIRE_CLIENT_CERTIFICATE_AND_VERIFY
 } grpc_ssl_client_certificate_request_type;
 
diff --git a/include/grpcpp/impl/codegen/async_stream.h b/include/grpcpp/impl/codegen/async_stream.h
index b2134590c3..b306cd3604 100644
--- a/include/grpcpp/impl/codegen/async_stream.h
+++ b/include/grpcpp/impl/codegen/async_stream.h
@@ -195,6 +195,13 @@ class ClientAsyncReader final : public ClientAsyncReaderInterface<R> {
     assert(size == sizeof(ClientAsyncReader));
   }
 
+  // This operator should never be called as the memory should be freed as part
+  // of the arena destruction. It only exists to provide a matching operator
+  // delete to the operator new so that some compilers will not complain (see
+  // https://github.com/grpc/grpc/issues/11301) Note at the time of adding this
+  // there are no tests catching the compiler warning.
+  static void operator delete(void*, void*) { assert(0); }
+
   void StartCall(void* tag) override {
     assert(!started_);
     started_ = true;
@@ -336,6 +343,13 @@ class ClientAsyncWriter final : public ClientAsyncWriterInterface<W> {
     assert(size == sizeof(ClientAsyncWriter));
   }
 
+  // This operator should never be called as the memory should be freed as part
+  // of the arena destruction. It only exists to provide a matching operator
+  // delete to the operator new so that some compilers will not complain (see
+  // https://github.com/grpc/grpc/issues/11301) Note at the time of adding this
+  // there are no tests catching the compiler warning.
+  static void operator delete(void*, void*) { assert(0); }
+
   void StartCall(void* tag) override {
     assert(!started_);
     started_ = true;
@@ -496,6 +510,13 @@ class ClientAsyncReaderWriter final
     assert(size == sizeof(ClientAsyncReaderWriter));
   }
 
+  // This operator should never be called as the memory should be freed as part
+  // of the arena destruction. It only exists to provide a matching operator
+  // delete to the operator new so that some compilers will not complain (see
+  // https://github.com/grpc/grpc/issues/11301) Note at the time of adding this
+  // there are no tests catching the compiler warning.
+  static void operator delete(void*, void*) { assert(0); }
+
   void StartCall(void* tag) override {
     assert(!started_);
     started_ = true;
diff --git a/include/grpcpp/impl/codegen/call.h b/include/grpcpp/impl/codegen/call.h
index a5e930aaa5..e94adada94 100644
--- a/include/grpcpp/impl/codegen/call.h
+++ b/include/grpcpp/impl/codegen/call.h
@@ -50,8 +50,6 @@ namespace internal {
 class Call;
 class CallHook;
 
-const char kBinaryErrorDetailsKey[] = "grpc-status-details-bin";
-
 // TODO(yangg) if the map is changed before we send, the pointers will be a
 // mess. Make sure it does not happen.
 inline grpc_metadata* FillMetadataArray(
@@ -531,7 +529,6 @@ class CallOpRecvInitialMetadata {
 
   void FinishOp(bool* status) {
     if (metadata_map_ == nullptr) return;
-    metadata_map_->FillMap();
     metadata_map_ = nullptr;
   }
 
@@ -566,13 +563,7 @@ class CallOpClientRecvStatus {
 
   void FinishOp(bool* status) {
     if (recv_status_ == nullptr) return;
-    metadata_map_->FillMap();
-    grpc::string binary_error_details;
-    auto iter = metadata_map_->map()->find(kBinaryErrorDetailsKey);
-    if (iter != metadata_map_->map()->end()) {
-      binary_error_details =
-          grpc::string(iter->second.begin(), iter->second.length());
-    }
+    grpc::string binary_error_details = metadata_map_->GetBinaryErrorDetails();
     *recv_status_ =
         Status(static_cast<StatusCode>(status_code_),
                GRPC_SLICE_IS_EMPTY(error_message_)
diff --git a/include/grpcpp/impl/codegen/client_context.h b/include/grpcpp/impl/codegen/client_context.h
index 9dda4c7fac..c6c9540950 100644
--- a/include/grpcpp/impl/codegen/client_context.h
+++ b/include/grpcpp/impl/codegen/client_context.h
@@ -425,8 +425,8 @@ class ClientContext {
   mutable std::shared_ptr<const AuthContext> auth_context_;
   struct census_context* census_context_;
   std::multimap<grpc::string, grpc::string> send_initial_metadata_;
-  internal::MetadataMap recv_initial_metadata_;
-  internal::MetadataMap trailing_metadata_;
+  mutable internal::MetadataMap recv_initial_metadata_;
+  mutable internal::MetadataMap trailing_metadata_;
 
   grpc_call* propagate_from_call_;
   PropagationOptions propagation_options_;
diff --git a/include/grpcpp/impl/codegen/metadata_map.h b/include/grpcpp/impl/codegen/metadata_map.h
index 0866539d88..5e062a50f8 100644
--- a/include/grpcpp/impl/codegen/metadata_map.h
+++ b/include/grpcpp/impl/codegen/metadata_map.h
@@ -19,11 +19,15 @@
 #ifndef GRPCPP_IMPL_CODEGEN_METADATA_MAP_H
 #define GRPCPP_IMPL_CODEGEN_METADATA_MAP_H
 
+#include <grpc/impl/codegen/log.h>
 #include <grpcpp/impl/codegen/slice.h>
 
 namespace grpc {
 
 namespace internal {
+
+const char kBinaryErrorDetailsKey[] = "grpc-status-details-bin";
+
 class MetadataMap {
  public:
   MetadataMap() { memset(&arr_, 0, sizeof(arr_)); }
@@ -32,24 +36,54 @@ class MetadataMap {
     g_core_codegen_interface->grpc_metadata_array_destroy(&arr_);
   }
 
-  void FillMap() {
-    for (size_t i = 0; i < arr_.count; i++) {
-      // TODO(yangg) handle duplicates?
-      map_.insert(std::pair<grpc::string_ref, grpc::string_ref>(
-          StringRefFromSlice(&arr_.metadata[i].key),
-          StringRefFromSlice(&arr_.metadata[i].value)));
+  grpc::string GetBinaryErrorDetails() {
+    // if filled_, extract from the multimap for O(log(n))
+    if (filled_) {
+      auto iter = map_.find(kBinaryErrorDetailsKey);
+      if (iter != map_.end()) {
+        return grpc::string(iter->second.begin(), iter->second.length());
+      }
+    }
+    // if not yet filled, take the O(n) lookup to avoid allocating the
+    // multimap until it is requested.
+    // TODO(ncteisen): plumb this through core as a first class object, just
+    // like code and message.
+    else {
+      for (size_t i = 0; i < arr_.count; i++) {
+        if (strncmp(reinterpret_cast<const char*>(
+                        GRPC_SLICE_START_PTR(arr_.metadata[i].key)),
+                    kBinaryErrorDetailsKey,
+                    GRPC_SLICE_LENGTH(arr_.metadata[i].key)) == 0) {
+          return grpc::string(reinterpret_cast<const char*>(
+                                  GRPC_SLICE_START_PTR(arr_.metadata[i].value)),
+                              GRPC_SLICE_LENGTH(arr_.metadata[i].value));
+        }
+      }
     }
+    return grpc::string();
   }
 
-  std::multimap<grpc::string_ref, grpc::string_ref>* map() { return &map_; }
-  const std::multimap<grpc::string_ref, grpc::string_ref>* map() const {
+  std::multimap<grpc::string_ref, grpc::string_ref>* map() {
+    FillMap();
     return &map_;
   }
   grpc_metadata_array* arr() { return &arr_; }
 
  private:
+  bool filled_ = false;
   grpc_metadata_array arr_;
   std::multimap<grpc::string_ref, grpc::string_ref> map_;
+
+  void FillMap() {
+    if (filled_) return;
+    filled_ = true;
+    for (size_t i = 0; i < arr_.count; i++) {
+      // TODO(yangg) handle duplicates?
+      map_.insert(std::pair<grpc::string_ref, grpc::string_ref>(
+          StringRefFromSlice(&arr_.metadata[i].key),
+          StringRefFromSlice(&arr_.metadata[i].value)));
+    }
+  }
 };
 }  // namespace internal
 
diff --git a/include/grpcpp/impl/codegen/server_context.h b/include/grpcpp/impl/codegen/server_context.h
index 6314364db6..b58f029de9 100644
--- a/include/grpcpp/impl/codegen/server_context.h
+++ b/include/grpcpp/impl/codegen/server_context.h
@@ -294,7 +294,7 @@ class ServerContext {
   CompletionQueue* cq_;
   bool sent_initial_metadata_;
   mutable std::shared_ptr<const AuthContext> auth_context_;
-  internal::MetadataMap client_metadata_;
+  mutable internal::MetadataMap client_metadata_;
   std::multimap<grpc::string, grpc::string> initial_metadata_;
   std::multimap<grpc::string, grpc::string> trailing_metadata_;