Merge pull request #8979 from markdroth/handshaker_cleanup

Address comments from #8913.  Also make changes needed for import.

4 files changed, 58 insertions, 52 deletions

diff --git a/src/core/lib/security/transport/security_connector.c b/src/core/lib/security/transport/security_connector.c
index a2e0c7c7c7..5b088aa58d 100644
--- a/src/core/lib/security/transport/security_connector.c
+++ b/src/core/lib/security/transport/security_connector.c
@@ -43,6 +43,7 @@
 #include <grpc/support/string_util.h>
 
 #include "src/core/ext/transport/chttp2/alpn/alpn.h"
+#include "src/core/lib/channel/handshaker.h"
 #include "src/core/lib/iomgr/load_file.h"
 #include "src/core/lib/security/context/security_context.h"
 #include "src/core/lib/security/credentials/credentials.h"
@@ -111,19 +112,19 @@ const tsi_peer_property *tsi_peer_get_property_by_name(const tsi_peer *peer,
   return NULL;
 }
 
-void grpc_channel_security_connector_create_handshakers(
+void grpc_channel_security_connector_add_handshakers(
     grpc_exec_ctx *exec_ctx, grpc_channel_security_connector *connector,
     grpc_handshake_manager *handshake_mgr) {
   if (connector != NULL) {
-    connector->create_handshakers(exec_ctx, connector, handshake_mgr);
+    connector->add_handshakers(exec_ctx, connector, handshake_mgr);
   }
 }
 
-void grpc_server_security_connector_create_handshakers(
+void grpc_server_security_connector_add_handshakers(
     grpc_exec_ctx *exec_ctx, grpc_server_security_connector *connector,
     grpc_handshake_manager *handshake_mgr) {
   if (connector != NULL) {
-    connector->create_handshakers(exec_ctx, connector, handshake_mgr);
+    connector->add_handshakers(exec_ctx, connector, handshake_mgr);
   }
 }
 
@@ -285,20 +286,24 @@ static void fake_channel_check_call_host(grpc_exec_ctx *exec_ctx,
   cb(exec_ctx, user_data, GRPC_SECURITY_OK);
 }
 
-static void fake_channel_create_handshakers(
+static void fake_channel_add_handshakers(
     grpc_exec_ctx *exec_ctx, grpc_channel_security_connector *sc,
     grpc_handshake_manager *handshake_mgr) {
-  grpc_security_create_handshakers(
-      exec_ctx, tsi_create_fake_handshaker(true /* is_client */), &sc->base,
-      handshake_mgr);
+  grpc_handshake_manager_add(
+      handshake_mgr,
+      grpc_security_handshaker_create(
+          exec_ctx, tsi_create_fake_handshaker(true /* is_client */),
+          &sc->base));
 }
 
-static void fake_server_create_handshakers(
-    grpc_exec_ctx *exec_ctx, grpc_server_security_connector *sc,
-    grpc_handshake_manager *handshake_mgr) {
-  grpc_security_create_handshakers(
-      exec_ctx, tsi_create_fake_handshaker(false /* is_client */), &sc->base,
-      handshake_mgr);
+static void fake_server_add_handshakers(grpc_exec_ctx *exec_ctx,
+                                        grpc_server_security_connector *sc,
+                                        grpc_handshake_manager *handshake_mgr) {
+  grpc_handshake_manager_add(
+      handshake_mgr,
+      grpc_security_handshaker_create(
+          exec_ctx, tsi_create_fake_handshaker(false /* is_client */),
+          &sc->base));
 }
 
 static grpc_security_connector_vtable fake_channel_vtable = {
@@ -316,7 +321,7 @@ grpc_channel_security_connector *grpc_fake_channel_security_connector_create(
   c->base.vtable = &fake_channel_vtable;
   c->request_metadata_creds = grpc_call_credentials_ref(request_metadata_creds);
   c->check_call_host = fake_channel_check_call_host;
-  c->create_handshakers = fake_channel_create_handshakers;
+  c->add_handshakers = fake_channel_add_handshakers;
   return c;
 }
 
@@ -328,7 +333,7 @@ grpc_server_security_connector *grpc_fake_server_security_connector_create(
   gpr_ref_init(&c->base.refcount, 1);
   c->base.vtable = &fake_server_vtable;
   c->base.url_scheme = GRPC_FAKE_SECURITY_URL_SCHEME;
-  c->create_handshakers = fake_server_create_handshakers;
+  c->add_handshakers = fake_server_add_handshakers;
   return c;
 }
 
@@ -382,9 +387,9 @@ static grpc_security_status ssl_create_handshaker(
   return GRPC_SECURITY_OK;
 }
 
-static void ssl_channel_create_handshakers(
-    grpc_exec_ctx *exec_ctx, grpc_channel_security_connector *sc,
-    grpc_handshake_manager *handshake_mgr) {
+static void ssl_channel_add_handshakers(grpc_exec_ctx *exec_ctx,
+                                        grpc_channel_security_connector *sc,
+                                        grpc_handshake_manager *handshake_mgr) {
   grpc_ssl_channel_security_connector *c =
       (grpc_ssl_channel_security_connector *)sc;
   // Instantiate TSI handshaker.
@@ -395,12 +400,13 @@ static void ssl_channel_create_handshakers(
                             : c->target_name,
                         &tsi_hs);
   // Create handshakers.
-  grpc_security_create_handshakers(exec_ctx, tsi_hs, &sc->base, handshake_mgr);
+  grpc_handshake_manager_add(handshake_mgr, grpc_security_handshaker_create(
+                                                exec_ctx, tsi_hs, &sc->base));
 }
 
-static void ssl_server_create_handshakers(
-    grpc_exec_ctx *exec_ctx, grpc_server_security_connector *sc,
-    grpc_handshake_manager *handshake_mgr) {
+static void ssl_server_add_handshakers(grpc_exec_ctx *exec_ctx,
+                                       grpc_server_security_connector *sc,
+                                       grpc_handshake_manager *handshake_mgr) {
   grpc_ssl_server_security_connector *c =
       (grpc_ssl_server_security_connector *)sc;
   // Instantiate TSI handshaker.
@@ -408,7 +414,8 @@ static void ssl_server_create_handshakers(
   ssl_create_handshaker(c->handshaker_factory, false /* is_client */,
                         NULL /* peer_name */, &tsi_hs);
   // Create handshakers.
-  grpc_security_create_handshakers(exec_ctx, tsi_hs, &sc->base, handshake_mgr);
+  grpc_handshake_manager_add(handshake_mgr, grpc_security_handshaker_create(
+                                                exec_ctx, tsi_hs, &sc->base));
 }
 
 static int ssl_host_matches_name(const tsi_peer *peer, const char *peer_name) {
@@ -708,7 +715,7 @@ grpc_security_status grpc_ssl_channel_security_connector_create(
   c->base.request_metadata_creds =
       grpc_call_credentials_ref(request_metadata_creds);
   c->base.check_call_host = ssl_channel_check_call_host;
-  c->base.create_handshakers = ssl_channel_create_handshakers;
+  c->base.add_handshakers = ssl_channel_add_handshakers;
   gpr_split_host_port(target_name, &c->target_name, &port);
   gpr_free(port);
   if (overridden_target_name != NULL) {
@@ -783,7 +790,7 @@ grpc_security_status grpc_ssl_server_security_connector_create(
     *sc = NULL;
     goto error;
   }
-  c->base.create_handshakers = ssl_server_create_handshakers;
+  c->base.add_handshakers = ssl_server_add_handshakers;
   *sc = &c->base;
   gpr_free((void *)alpn_protocol_strings);
   gpr_free(alpn_protocol_string_lengths);
diff --git a/src/core/lib/security/transport/security_connector.h b/src/core/lib/security/transport/security_connector.h
index 696db0e02e..a84b359051 100644
--- a/src/core/lib/security/transport/security_connector.h
+++ b/src/core/lib/security/transport/security_connector.h
@@ -98,7 +98,7 @@ void grpc_security_connector_unref(grpc_security_connector *policy);
 #endif
 
 /* Check the peer. Callee takes ownership of the peer object.
-   Sets *auth_context and invokes on_peer_checked when done. */
+   When done, sets *auth_context and invokes on_peer_checked. */
 void grpc_security_connector_check_peer(grpc_exec_ctx *exec_ctx,
                                         grpc_security_connector *sc,
                                         tsi_peer peer,
@@ -133,9 +133,9 @@ struct grpc_channel_security_connector {
                           grpc_channel_security_connector *sc, const char *host,
                           grpc_auth_context *auth_context,
                           grpc_security_call_host_check_cb cb, void *user_data);
-  void (*create_handshakers)(grpc_exec_ctx *exec_ctx,
-                             grpc_channel_security_connector *sc,
-                             grpc_handshake_manager *handshake_mgr);
+  void (*add_handshakers)(grpc_exec_ctx *exec_ctx,
+                          grpc_channel_security_connector *sc,
+                          grpc_handshake_manager *handshake_mgr);
 };
 
 /* Checks that the host that will be set for a call is acceptable. */
@@ -145,7 +145,7 @@ void grpc_channel_security_connector_check_call_host(
     grpc_security_call_host_check_cb cb, void *user_data);
 
 /* Registers handshakers with \a handshake_mgr. */
-void grpc_channel_security_connector_create_handshakers(
+void grpc_channel_security_connector_add_handshakers(
     grpc_exec_ctx *exec_ctx, grpc_channel_security_connector *connector,
     grpc_handshake_manager *handshake_mgr);
 
@@ -158,12 +158,12 @@ typedef struct grpc_server_security_connector grpc_server_security_connector;
 
 struct grpc_server_security_connector {
   grpc_security_connector base;
-  void (*create_handshakers)(grpc_exec_ctx *exec_ctx,
-                             grpc_server_security_connector *sc,
-                             grpc_handshake_manager *handshake_mgr);
+  void (*add_handshakers)(grpc_exec_ctx *exec_ctx,
+                          grpc_server_security_connector *sc,
+                          grpc_handshake_manager *handshake_mgr);
 };
 
-void grpc_server_security_connector_create_handshakers(
+void grpc_server_security_connector_add_handshakers(
     grpc_exec_ctx *exec_ctx, grpc_server_security_connector *sc,
     grpc_handshake_manager *handshake_mgr);
 
diff --git a/src/core/lib/security/transport/security_handshaker.c b/src/core/lib/security/transport/security_handshaker.c
index fc01bec2f2..41a775db85 100644
--- a/src/core/lib/security/transport/security_handshaker.c
+++ b/src/core/lib/security/transport/security_handshaker.c
@@ -131,6 +131,9 @@ static void security_handshake_failed_locked(grpc_exec_ctx *exec_ctx,
     // Not shutting down, so the write failed.  Clean up before
     // invoking the callback.
     cleanup_args_for_failure_locked(h);
+    // Set shutdown to true so that subsequent calls to
+    // security_handshaker_shutdown() do nothing.
+    h->shutdown = true;
   }
   // Invoke callback.
   grpc_exec_ctx_sched(exec_ctx, h->on_handshake_done, error, NULL);
@@ -434,17 +437,14 @@ static grpc_handshaker *fail_handshaker_create() {
 // exported functions
 //
 
-void grpc_security_create_handshakers(grpc_exec_ctx *exec_ctx,
-                                      tsi_handshaker *handshaker,
-                                      grpc_security_connector *connector,
-                                      grpc_handshake_manager *handshake_mgr) {
-  // If no TSI handshaker was created, add a handshaker that always fails.
-  // Otherwise, add a real security handshaker.
+grpc_handshaker *grpc_security_handshaker_create(
+    grpc_exec_ctx *exec_ctx, tsi_handshaker *handshaker,
+    grpc_security_connector *connector) {
+  // If no TSI handshaker was created, return a handshaker that always fails.
+  // Otherwise, return a real security handshaker.
   if (handshaker == NULL) {
-    grpc_handshake_manager_add(handshake_mgr, fail_handshaker_create());
+    return fail_handshaker_create();
   } else {
-    grpc_handshake_manager_add(
-        handshake_mgr,
-        security_handshaker_create(exec_ctx, handshaker, connector));
+    return security_handshaker_create(exec_ctx, handshaker, connector);
   }
 }
diff --git a/src/core/lib/security/transport/security_handshaker.h b/src/core/lib/security/transport/security_handshaker.h
index f71f43a359..5ddbf4b451 100644
--- a/src/core/lib/security/transport/security_handshaker.h
+++ b/src/core/lib/security/transport/security_handshaker.h
@@ -34,14 +34,13 @@
 #ifndef GRPC_CORE_LIB_SECURITY_TRANSPORT_SECURITY_HANDSHAKER_H
 #define GRPC_CORE_LIB_SECURITY_TRANSPORT_SECURITY_HANDSHAKER_H
 
-#include "src/core/lib/iomgr/endpoint.h"
+#include "src/core/lib/channel/handshaker.h"
+#include "src/core/lib/iomgr/exec_ctx.h"
 #include "src/core/lib/security/transport/security_connector.h"
 
-/// Creates any necessary security handshakers and adds them to
-/// \a handshake_mgr.
-void grpc_security_create_handshakers(grpc_exec_ctx *exec_ctx,
-                                      tsi_handshaker *handshaker,
-                                      grpc_security_connector *connector,
-                                      grpc_handshake_manager *handshake_mgr);
+/// Creates a security handshaker using \a handshaker.
+grpc_handshaker *grpc_security_handshaker_create(
+    grpc_exec_ctx *exec_ctx, tsi_handshaker *handshaker,
+    grpc_security_connector *connector);
 
 #endif /* GRPC_CORE_LIB_SECURITY_TRANSPORT_SECURITY_HANDSHAKER_H */