Make handshaker responsible for destroying endpoint on shutdown or failure.

2 files changed, 20 insertions, 18 deletions

diff --git a/src/core/lib/channel/handshaker.c b/src/core/lib/channel/handshaker.c
index 3c125a22f3..f3bd91284e 100644
--- a/src/core/lib/channel/handshaker.c
+++ b/src/core/lib/channel/handshaker.c
@@ -141,25 +141,20 @@ void grpc_handshake_manager_destroy(grpc_exec_ctx* exec_ctx,
 void grpc_handshake_manager_shutdown(grpc_exec_ctx* exec_ctx,
                                      grpc_handshake_manager* mgr) {
   gpr_mu_lock(&mgr->mu);
-  for (size_t i = 0; i < mgr->count; ++i) {
-    grpc_handshaker_shutdown(exec_ctx, mgr->handshakers[i]);
+  if (mgr->index > 0) {
+    grpc_handshaker_shutdown(exec_ctx, mgr->handshakers[mgr->index - 1]);
   }
   gpr_mu_unlock(&mgr->mu);
 }
 
-static void call_next_handshaker(grpc_exec_ctx* exec_ctx, void* arg,
-                                 grpc_error* error);
-
 // Helper function to call either the next handshaker or the
 // on_handshake_done callback.
 static void call_next_handshaker_locked(grpc_exec_ctx* exec_ctx,
                                         grpc_handshake_manager* mgr,
                                         grpc_error* error) {
   GPR_ASSERT(mgr->index <= mgr->count);
-  // If we got an error, skip all remaining handshakers and invoke the
-  // caller-supplied callback immediately.
-  // Otherwise, if this is the last handshaker, then call the on_handshake_done
-  // callback instead of chaining back to this function again.
+  // If we got an error or we've finished the last handshaker, invoke
+  // the on_handshake_done callback.  Otherwise, call the next handshaker.
   if (error != GRPC_ERROR_NONE || mgr->index == mgr->count) {
     // Cancel deadline timer, since we're invoking the on_handshake_done
     // callback now.
@@ -202,6 +197,8 @@ void grpc_handshake_manager_do_handshake(
     grpc_endpoint* endpoint, const grpc_channel_args* channel_args,
     gpr_timespec deadline, grpc_tcp_server_acceptor* acceptor,
     grpc_iomgr_cb_func on_handshake_done, void* user_data) {
+  gpr_mu_lock(&mgr->mu);
+  GPR_ASSERT(mgr->index == 0);
   // Construct handshaker args.  These will be passed through all
   // handshakers and eventually be freed by the on_handshake_done callback.
   mgr->args.endpoint = endpoint;
@@ -210,8 +207,6 @@ void grpc_handshake_manager_do_handshake(
   mgr->args.read_buffer = gpr_malloc(sizeof(*mgr->args.read_buffer));
   grpc_slice_buffer_init(mgr->args.read_buffer);
   // Initialize state needed for calling handshakers.
-  gpr_mu_lock(&mgr->mu);
-  GPR_ASSERT(mgr->index == 0);
   mgr->acceptor = acceptor;
   grpc_closure_init(&mgr->call_next_handshaker, call_next_handshaker, mgr);
   grpc_closure_init(&mgr->on_handshake_done, on_handshake_done, &mgr->args);
diff --git a/src/core/lib/channel/handshaker.h b/src/core/lib/channel/handshaker.h
index f0614c354b..2e1f543512 100644
--- a/src/core/lib/channel/handshaker.h
+++ b/src/core/lib/channel/handshaker.h
@@ -57,17 +57,24 @@ typedef struct grpc_handshaker grpc_handshaker;
 /// Arguments passed through handshakers and to the on_handshake_done callback.
 ///
 /// For handshakers, all members are input/output parameters; for
-/// example, a handshaker may read from \a endpoint and then later
-/// replace it with a wrapped endpoint.  Similarly, a handshaker may
-/// modify \a args.
+/// example, a handshaker may read from or write to \a endpoint and
+/// then later replace it with a wrapped endpoint.  Similarly, a
+/// handshaker may modify \a args.
+///
+/// A handshaker takes ownership of the members while a handshake is in
+/// progress.  Upon failure or shutdown of an in-progress handshaker,
+/// the handshaker is responsible for destroying the members and setting
+/// them to NULL before invoking the on_handshake_done callback.
 ///
 /// For the on_handshake_done callback, all members are input arguments,
 /// which the callback takes ownership of.
 typedef struct {
   grpc_endpoint* endpoint;
   grpc_channel_args* args;
-  void* user_data;
   grpc_slice_buffer* read_buffer;
+  // User data passed through the handshake manager.  Not used by
+  // individual handshakers.
+  void* user_data;
 } grpc_handshaker_args;
 
 typedef struct {
@@ -132,9 +139,9 @@ void grpc_handshake_manager_shutdown(grpc_exec_ctx* exec_ctx,
 ///
 /// When done, invokes \a on_handshake_done with a grpc_handshaker_args
 /// object as its argument.  If the callback is invoked with error !=
-/// GRPC_ERROR_NONE, then handshaking failed and the resulting endpoint
-/// will have already been shut down (although the caller will still be
-/// responsible for destroying it).
+/// GRPC_ERROR_NONE, then handshaking failed and the handshaker has done
+/// the necessary clean-up.  Otherwise, the callback takes ownership of
+/// the arguments.
 void grpc_handshake_manager_do_handshake(
     grpc_exec_ctx* exec_ctx, grpc_handshake_manager* mgr,
     grpc_endpoint* endpoint, const grpc_channel_args* channel_args,