Merge pull request #12372 from vjpai/armalarmadingdong

Core API change: Decouple alarm construction from setting to avoid races in MT code

5 files changed, 28 insertions, 13 deletions

diff --git a/src/core/lib/iomgr/timer.h b/src/core/lib/iomgr/timer.h
index b92b8fb8b8..ac392f87fe 100644
--- a/src/core/lib/iomgr/timer.h
+++ b/src/core/lib/iomgr/timer.h
@@ -44,6 +44,10 @@ void grpc_timer_init(grpc_exec_ctx *exec_ctx, grpc_timer *timer,
                      gpr_timespec deadline, grpc_closure *closure,
                      gpr_timespec now);
 
+/* Initialize *timer without setting it. This can later be passed through
+   the regular init or cancel */
+void grpc_timer_init_unset(grpc_timer *timer);
+
 /* Note that there is no timer destroy function. This is because the
    timer is a one-time occurrence with a guarantee that the callback will
    be called exactly once, either at expiration or cancellation. Thus, all
diff --git a/src/core/lib/iomgr/timer_generic.c b/src/core/lib/iomgr/timer_generic.c
index 12efce241f..c08bb525b7 100644
--- a/src/core/lib/iomgr/timer_generic.c
+++ b/src/core/lib/iomgr/timer_generic.c
@@ -234,6 +234,8 @@ static void note_deadline_change(timer_shard *shard) {
   }
 }
 
+void grpc_timer_init_unset(grpc_timer *timer) { timer->pending = false; }
+
 void grpc_timer_init(grpc_exec_ctx *exec_ctx, grpc_timer *timer,
                      gpr_timespec deadline, grpc_closure *closure,
                      gpr_timespec now) {
diff --git a/src/core/lib/iomgr/timer_uv.c b/src/core/lib/iomgr/timer_uv.c
index 70f49bcbe8..adced41f53 100644
--- a/src/core/lib/iomgr/timer_uv.c
+++ b/src/core/lib/iomgr/timer_uv.c
@@ -77,6 +77,8 @@ void grpc_timer_init(grpc_exec_ctx *exec_ctx, grpc_timer *timer,
   uv_unref((uv_handle_t *)uv_timer);
 }
 
+void grpc_timer_init_unset(grpc_timer *timer) { timer->pending = 0; }
+
 void grpc_timer_cancel(grpc_exec_ctx *exec_ctx, grpc_timer *timer) {
   GRPC_UV_ASSERT_SAME_THREAD();
   if (timer->pending) {
diff --git a/src/core/lib/surface/alarm.c b/src/core/lib/surface/alarm.c
index 7d60b1de17..5dbfaa2d43 100644
--- a/src/core/lib/surface/alarm.c
+++ b/src/core/lib/surface/alarm.c
@@ -44,7 +44,9 @@ static void alarm_ref(grpc_alarm *alarm) { gpr_ref(&alarm->refs); }
 static void alarm_unref(grpc_alarm *alarm) {
   if (gpr_unref(&alarm->refs)) {
     grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT;
-    GRPC_CQ_INTERNAL_UNREF(&exec_ctx, alarm->cq, "alarm");
+    if (alarm->cq != NULL) {
+      GRPC_CQ_INTERNAL_UNREF(&exec_ctx, alarm->cq, "alarm");
+    }
     grpc_exec_ctx_finish(&exec_ctx);
     gpr_free(alarm);
   }
@@ -93,12 +95,8 @@ static void alarm_cb(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) {
                  (void *)alarm, &alarm->completion);
 }
 
-grpc_alarm *grpc_alarm_create(grpc_completion_queue *cq, gpr_timespec deadline,
-                              void *tag) {
+grpc_alarm *grpc_alarm_create(void *reserved) {
   grpc_alarm *alarm = gpr_malloc(sizeof(grpc_alarm));
-  grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT;
-
-  gpr_ref_init(&alarm->refs, 1);
 
 #ifndef NDEBUG
   if (GRPC_TRACER_ON(grpc_trace_alarm_refcount)) {
@@ -106,27 +104,36 @@ grpc_alarm *grpc_alarm_create(grpc_completion_queue *cq, gpr_timespec deadline,
   }
 #endif
 
+  gpr_ref_init(&alarm->refs, 1);
+  grpc_timer_init_unset(&alarm->alarm);
+  alarm->cq = NULL;
+  GRPC_CLOSURE_INIT(&alarm->on_alarm, alarm_cb, alarm,
+                    grpc_schedule_on_exec_ctx);
+  return alarm;
+}
+
+void grpc_alarm_set(grpc_alarm *alarm, grpc_completion_queue *cq,
+                    gpr_timespec deadline, void *tag, void *reserved) {
+  grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT;
+
   GRPC_CQ_INTERNAL_REF(cq, "alarm");
   alarm->cq = cq;
   alarm->tag = tag;
 
   GPR_ASSERT(grpc_cq_begin_op(cq, tag));
-  GRPC_CLOSURE_INIT(&alarm->on_alarm, alarm_cb, alarm,
-                    grpc_schedule_on_exec_ctx);
   grpc_timer_init(&exec_ctx, &alarm->alarm,
                   gpr_convert_clock_type(deadline, GPR_CLOCK_MONOTONIC),
                   &alarm->on_alarm, gpr_now(GPR_CLOCK_MONOTONIC));
   grpc_exec_ctx_finish(&exec_ctx);
-  return alarm;
 }
 
-void grpc_alarm_cancel(grpc_alarm *alarm) {
+void grpc_alarm_cancel(grpc_alarm *alarm, void *reserved) {
   grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT;
   grpc_timer_cancel(&exec_ctx, &alarm->alarm);
   grpc_exec_ctx_finish(&exec_ctx);
 }
 
-void grpc_alarm_destroy(grpc_alarm *alarm) {
-  grpc_alarm_cancel(alarm);
+void grpc_alarm_destroy(grpc_alarm *alarm, void *reserved) {
+  grpc_alarm_cancel(alarm, reserved);
   GRPC_ALARM_UNREF(alarm, "alarm_destroy");
 }
diff --git a/src/core/lib/surface/version.c b/src/core/lib/surface/version.c
index 96c16105e7..fd6ea4daa9 100644
--- a/src/core/lib/surface/version.c
+++ b/src/core/lib/surface/version.c
@@ -21,6 +21,6 @@
 
 #include <grpc/grpc.h>
 
-const char *grpc_version_string(void) { return "4.0.0-dev"; }
+const char *grpc_version_string(void) { return "5.0.0-dev"; }
 
 const char *grpc_g_stands_for(void) { return "gambit"; }