Reset cancellation closure when unreffing the call to avoid race conditions.

5 files changed, 26 insertions, 27 deletions

diff --git a/src/core/ext/filters/client_channel/client_channel.c b/src/core/ext/filters/client_channel/client_channel.c
index 8b0d2ee61f..c24e52ec1d 100644
--- a/src/core/ext/filters/client_channel/client_channel.c
+++ b/src/core/ext/filters/client_channel/client_channel.c
@@ -1083,10 +1083,9 @@ static void pick_after_resolver_result_cancel_locked(grpc_exec_ctx *exec_ctx,
   // it's safe to call subchannel_ready_locked() here -- we are
   // essentially calling it here instead of calling it in
   // pick_after_resolver_result_done_locked().
-  subchannel_ready_locked(
-      exec_ctx, elem,
-      GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING("Pick cancelled",
-                                                       &error, 1));
+  subchannel_ready_locked(exec_ctx, elem,
+                          GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING(
+                              "Pick cancelled", &error, 1));
 }
 
 static void pick_after_resolver_result_done_locked(grpc_exec_ctx *exec_ctx,
@@ -1105,8 +1104,6 @@ static void pick_after_resolver_result_done_locked(grpc_exec_ctx *exec_ctx,
   grpc_call_element *elem = args->elem;
   channel_data *chand = elem->channel_data;
   call_data *calld = elem->call_data;
-  grpc_call_combiner_set_notify_on_cancel(exec_ctx, calld->call_combiner,
-                                          NULL);
   if (error != GRPC_ERROR_NONE) {
     if (GRPC_TRACER_ON(grpc_client_channel_trace)) {
       gpr_log(GPR_DEBUG, "chand=%p calld=%p: resolver failed to return data",
@@ -1177,7 +1174,6 @@ static void pick_callback_done_locked(grpc_exec_ctx *exec_ctx, void *arg,
     gpr_log(GPR_DEBUG, "chand=%p calld=%p: pick completed asynchronously",
             chand, calld);
   }
-  grpc_call_combiner_set_notify_on_cancel(exec_ctx, calld->call_combiner, NULL);
   GPR_ASSERT(calld->lb_policy != NULL);
   GRPC_LB_POLICY_UNREF(exec_ctx, calld->lb_policy, "pick_subchannel");
   calld->lb_policy = NULL;
diff --git a/src/core/lib/iomgr/call_combiner.h b/src/core/lib/iomgr/call_combiner.h
index 11802b6eaa..5cfb3f0c07 100644
--- a/src/core/lib/iomgr/call_combiner.h
+++ b/src/core/lib/iomgr/call_combiner.h
@@ -87,20 +87,28 @@ void grpc_call_combiner_stop(grpc_exec_ctx* exec_ctx,
                              const char* reason);
 #endif
 
-/// Tells \a call_combiner to schedule \a closure when
+/// Registers \a closure to be invoked by \a call_combiner when
 /// grpc_call_combiner_cancel() is called.
 ///
-/// If grpc_call_combiner_cancel() was previously called, \a closure will be
-/// scheduled immediately.
+/// Once a closure is registered, it will always be scheduled exactly
+/// once; this allows the closure to hold references that will be freed
+/// regardless of whether or not the call was cancelled.  If a cancellation
+/// does occur, the closure will be scheduled with the cancellation error;
+/// otherwise, it will be scheduled with GRPC_ERROR_NONE.
+///
+/// The closure will be scheduled in the following cases:
+/// - If grpc_call_combiner_cancel() was called prior to registering the
+///   closure, it will be scheduled immediately with the cancelation error.
+/// - If grpc_call_combiner_cancel() is called after registering the
+///   closure, the closure will be scheduled with the cancellation error.
+/// - If grpc_call_combiner_set_notify_on_cancel() is called again to
+///   register a new cancellation closure, the previous cancellation
+///   closure will be scheduled with GRPC_ERROR_NONE.
 ///
 /// If \a closure is NULL, then no closure will be invoked on
 /// cancellation; this effectively unregisters the previously set closure.
-///
-/// If a closure was set via a previous call to
-/// grpc_call_combiner_set_notify_on_cancel(), the previous closure will be
-/// scheduled immediately with GRPC_ERROR_NONE.  This ensures that
-/// \a closure will be scheduled exactly once, which allows callers to clean
-/// up resources they may be holding for the callback.
+/// However, most filters will not need to explicitly unregister their
+/// callbacks, as this is done automatically when the call is destroyed.
 void grpc_call_combiner_set_notify_on_cancel(grpc_exec_ctx* exec_ctx,
                                              grpc_call_combiner* call_combiner,
                                              grpc_closure* closure);
diff --git a/src/core/lib/security/transport/client_auth_filter.c b/src/core/lib/security/transport/client_auth_filter.c
index 69eb612eff..dd7dd44e79 100644
--- a/src/core/lib/security/transport/client_auth_filter.c
+++ b/src/core/lib/security/transport/client_auth_filter.c
@@ -95,7 +95,6 @@ static void on_credentials_metadata(grpc_exec_ctx *exec_ctx, void *arg,
   grpc_transport_stream_op_batch *batch = (grpc_transport_stream_op_batch *)arg;
   grpc_call_element *elem = batch->handler_private.extra_arg;
   call_data *calld = elem->call_data;
-  grpc_call_combiner_set_notify_on_cancel(exec_ctx, calld->call_combiner, NULL);
   reset_auth_metadata_context(&calld->auth_md_context);
   grpc_error *error = GRPC_ERROR_REF(input_error);
   if (error == GRPC_ERROR_NONE) {
@@ -228,7 +227,6 @@ static void on_host_checked(grpc_exec_ctx *exec_ctx, void *arg,
   grpc_transport_stream_op_batch *batch = (grpc_transport_stream_op_batch *)arg;
   grpc_call_element *elem = batch->handler_private.extra_arg;
   call_data *calld = elem->call_data;
-  grpc_call_combiner_set_notify_on_cancel(exec_ctx, calld->call_combiner, NULL);
   if (error == GRPC_ERROR_NONE) {
     send_security_metadata(exec_ctx, elem, batch);
   } else {
@@ -318,14 +316,6 @@ static void auth_start_transport_stream_op_batch(
         on_host_checked(exec_ctx, batch, error);
         GRPC_ERROR_UNREF(error);
       } else {
-// FIXME: if grpc_channel_security_connector_check_call_host() invokes
-// the callback in this thread before returning, then we'll call
-// grpc_call_combiner_set_notify_on_cancel() to set it "back" to NULL
-// *before* we call this to set it to the cancel function.
-// Can't just do this before calling
-// grpc_channel_security_connector_check_call_host(), because then the
-// cancellation might be invoked before we actually send the request.
-// May need to fix the credentials plugin API to deal with this.
         // Async return; register cancellation closure with call combiner.
         GRPC_CALL_STACK_REF(calld->owning_call, "cancel_check_call_host");
         grpc_call_combiner_set_notify_on_cancel(
diff --git a/src/core/lib/security/transport/server_auth_filter.c b/src/core/lib/security/transport/server_auth_filter.c
index 6cf61c03a2..7f523c0883 100644
--- a/src/core/lib/security/transport/server_auth_filter.c
+++ b/src/core/lib/security/transport/server_auth_filter.c
@@ -97,7 +97,6 @@ static void on_md_processing_done_inner(grpc_exec_ctx *exec_ctx,
                                         grpc_error *error) {
   call_data *calld = elem->call_data;
   grpc_transport_stream_op_batch *batch = calld->recv_initial_metadata_batch;
-  grpc_call_combiner_set_notify_on_cancel(exec_ctx, calld->call_combiner, NULL);
   /* TODO(jboeuf): Implement support for response_md. */
   if (response_md != NULL && num_response_md > 0) {
     gpr_log(GPR_INFO,
diff --git a/src/core/lib/surface/call.c b/src/core/lib/surface/call.c
index 03eaaf99ac..3aa20ffcd7 100644
--- a/src/core/lib/surface/call.c
+++ b/src/core/lib/surface/call.c
@@ -598,6 +598,12 @@ void grpc_call_unref(grpc_call *c) {
   if (cancel) {
     cancel_with_error(&exec_ctx, c, STATUS_FROM_API_OVERRIDE,
                       GRPC_ERROR_CANCELLED);
+  } else {
+    // Unset the call combiner cancellation closure.  This has the
+    // effect of scheduling the previously set cancellation closure, if
+    // any, so that it can release any internal references it may be
+    // holding to the call stack.
+    grpc_call_combiner_set_notify_on_cancel(&exec_ctx, &c->call_combiner, NULL);
   }
   GRPC_CALL_INTERNAL_UNREF(&exec_ctx, c, "destroy");
   grpc_exec_ctx_finish(&exec_ctx);