Fix asan and tsan failures.

2 files changed, 34 insertions, 3 deletions

diff --git a/src/core/lib/iomgr/call_combiner.c b/src/core/lib/iomgr/call_combiner.c
index 899f98552d..48d8eaec18 100644
--- a/src/core/lib/iomgr/call_combiner.c
+++ b/src/core/lib/iomgr/call_combiner.c
@@ -140,11 +140,33 @@ void grpc_call_combiner_set_notify_on_cancel(grpc_exec_ctx* exec_ctx,
     // If error is set, invoke the cancellation closure immediately.
     // Otherwise, store the new closure.
     if (original_error != GRPC_ERROR_NONE) {
+      if (GRPC_TRACER_ON(grpc_call_combiner_trace)) {
+        gpr_log(GPR_DEBUG,
+                "call_combiner=%p: scheduling notify_on_cancel callback=%p "
+                "for pre-existing cancellation",
+                call_combiner, closure);
+      }
       GRPC_CLOSURE_SCHED(exec_ctx, closure, GRPC_ERROR_REF(original_error));
       break;
     } else {
       if (gpr_atm_full_cas(&call_combiner->cancel_state, original_state,
                            (gpr_atm)closure)) {
+        if (GRPC_TRACER_ON(grpc_call_combiner_trace)) {
+          gpr_log(GPR_DEBUG, "call_combiner=%p: setting notify_on_cancel=%p",
+                  call_combiner, closure);
+        }
+        // If we replaced an earlier closure, invoke the original
+        // closure with GRPC_ERROR_NONE.  This allows callers to clean
+        // up any resources they may be holding for the callback.
+        if (original_state != 0) {
+          closure = (grpc_closure*)original_state;
+          if (GRPC_TRACER_ON(grpc_call_combiner_trace)) {
+            gpr_log(GPR_DEBUG,
+                    "call_combiner=%p: scheduling old cancel callback=%p",
+                    call_combiner, closure);
+          }
+          GRPC_CLOSURE_SCHED(exec_ctx, closure, GRPC_ERROR_NONE);
+        }
         break;
       }
     }
diff --git a/src/core/lib/iomgr/call_combiner.h b/src/core/lib/iomgr/call_combiner.h
index 621e2c3669..11802b6eaa 100644
--- a/src/core/lib/iomgr/call_combiner.h
+++ b/src/core/lib/iomgr/call_combiner.h
@@ -87,11 +87,20 @@ void grpc_call_combiner_stop(grpc_exec_ctx* exec_ctx,
                              const char* reason);
 #endif
 
-/// Tells \a call_combiner to invoke \a closure when
-/// grpc_call_combiner_cancel() is called.  If grpc_call_combiner_cancel()
-/// was previously called, \a closure will be invoked immediately.
+/// Tells \a call_combiner to schedule \a closure when
+/// grpc_call_combiner_cancel() is called.
+///
+/// If grpc_call_combiner_cancel() was previously called, \a closure will be
+/// scheduled immediately.
+///
 /// 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.
 void grpc_call_combiner_set_notify_on_cancel(grpc_exec_ctx* exec_ctx,
                                              grpc_call_combiner* call_combiner,
                                              grpc_closure* closure);