Refcount grpc_alarm object so that grpc_alarm_destroy can safely be called before the alarm event is dequeued from the completion queue

17 files changed, 141 insertions, 8 deletions

diff --git a/BUILD b/BUILD
index fa42326787..b6f92c442e 100644
--- a/BUILD
+++ b/BUILD
@@ -645,6 +645,7 @@ grpc_cc_library(
         "src/core/lib/slice/slice_hash_table.h",
         "src/core/lib/slice/slice_internal.h",
         "src/core/lib/slice/slice_string_helpers.h",
+        "src/core/lib/surface/alarm_internal.h",
         "src/core/lib/surface/api_trace.h",
         "src/core/lib/surface/call.h",
         "src/core/lib/surface/call_test_only.h",
diff --git a/build.yaml b/build.yaml
index 940ca8902d..5156b95dae 100644
--- a/build.yaml
+++ b/build.yaml
@@ -261,6 +261,7 @@ filegroups:
   - src/core/lib/slice/slice_hash_table.h
   - src/core/lib/slice/slice_internal.h
   - src/core/lib/slice/slice_string_helpers.h
+  - src/core/lib/surface/alarm_internal.h
   - src/core/lib/surface/api_trace.h
   - src/core/lib/surface/call.h
   - src/core/lib/surface/call_test_only.h
diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec
index 782ebb8ae1..b33e08c5f5 100644
--- a/gRPC-Core.podspec
+++ b/gRPC-Core.podspec
@@ -325,6 +325,7 @@ Pod::Spec.new do |s|
                       'src/core/lib/slice/slice_hash_table.h',
                       'src/core/lib/slice/slice_internal.h',
                       'src/core/lib/slice/slice_string_helpers.h',
+                      'src/core/lib/surface/alarm_internal.h',
                       'src/core/lib/surface/api_trace.h',
                       'src/core/lib/surface/call.h',
                       'src/core/lib/surface/call_test_only.h',
@@ -808,6 +809,7 @@ Pod::Spec.new do |s|
                               'src/core/lib/slice/slice_hash_table.h',
                               'src/core/lib/slice/slice_internal.h',
                               'src/core/lib/slice/slice_string_helpers.h',
+                              'src/core/lib/surface/alarm_internal.h',
                               'src/core/lib/surface/api_trace.h',
                               'src/core/lib/surface/call.h',
                               'src/core/lib/surface/call_test_only.h',
diff --git a/grpc.gemspec b/grpc.gemspec
index 18ef37a077..ff01882e05 100755
--- a/grpc.gemspec
+++ b/grpc.gemspec
@@ -256,6 +256,7 @@ Gem::Specification.new do |s|
   s.files += %w( src/core/lib/slice/slice_hash_table.h )
   s.files += %w( src/core/lib/slice/slice_internal.h )
   s.files += %w( src/core/lib/slice/slice_string_helpers.h )
+  s.files += %w( src/core/lib/surface/alarm_internal.h )
   s.files += %w( src/core/lib/surface/api_trace.h )
   s.files += %w( src/core/lib/surface/call.h )
   s.files += %w( src/core/lib/surface/call_test_only.h )
diff --git a/package.xml b/package.xml
index 10cb2e63ff..a3b591ac75 100644
--- a/package.xml
+++ b/package.xml
@@ -270,6 +270,7 @@
     <file baseinstalldir="/" name="src/core/lib/slice/slice_hash_table.h" role="src" />
     <file baseinstalldir="/" name="src/core/lib/slice/slice_internal.h" role="src" />
     <file baseinstalldir="/" name="src/core/lib/slice/slice_string_helpers.h" role="src" />
+    <file baseinstalldir="/" name="src/core/lib/surface/alarm_internal.h" role="src" />
     <file baseinstalldir="/" name="src/core/lib/surface/api_trace.h" role="src" />
     <file baseinstalldir="/" name="src/core/lib/surface/call.h" role="src" />
     <file baseinstalldir="/" name="src/core/lib/surface/call_test_only.h" role="src" />
diff --git a/src/core/lib/surface/alarm.c b/src/core/lib/surface/alarm.c
index ef8405cca8..343e48c101 100644
--- a/src/core/lib/surface/alarm.c
+++ b/src/core/lib/surface/alarm.c
@@ -15,13 +15,19 @@
  * limitations under the License.
  *
  */
+#include "src/core/lib/surface/alarm_internal.h"
 
 #include <grpc/grpc.h>
 #include <grpc/support/alloc.h>
 #include "src/core/lib/iomgr/timer.h"
 #include "src/core/lib/surface/completion_queue.h"
 
+#ifndef NDEBUG
+grpc_tracer_flag grpc_trace_alarm_refcount = GRPC_TRACER_INITIALIZER(false);
+#endif
+
 struct grpc_alarm {
+  gpr_refcount refs;
   grpc_timer alarm;
   grpc_closure on_alarm;
   grpc_cq_completion completion;
@@ -31,13 +37,58 @@ struct grpc_alarm {
   void *tag;
 };
 
-static void do_nothing_end_completion(grpc_exec_ctx *exec_ctx, void *arg,
-                                      grpc_cq_completion *c) {}
+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");
+    grpc_exec_ctx_finish(&exec_ctx);
+    gpr_free(alarm);
+  }
+}
+
+#ifndef NDEBUG
+static void alarm_ref_dbg(grpc_alarm *alarm, const char *reason,
+                          const char *file, int line) {
+  if (GRPC_TRACER_ON(grpc_trace_alarm_refcount)) {
+    gpr_atm val = gpr_atm_no_barrier_load(&alarm->refs.count);
+    gpr_log(file, line, GPR_LOG_SEVERITY_DEBUG,
+            "Alarm:%p  ref %" PRIdPTR " -> %" PRIdPTR " %s", alarm, val,
+            val + 1, reason);
+  }
+
+  alarm_ref(alarm);
+}
+
+static void alarm_unref_dbg(grpc_alarm *alarm, const char *reason,
+                            const char *file, int line) {
+  if (GRPC_TRACER_ON(grpc_trace_alarm_refcount)) {
+    gpr_atm val = gpr_atm_no_barrier_load(&alarm->refs.count);
+    gpr_log(file, line, GPR_LOG_SEVERITY_DEBUG,
+            "Alarm:%p  Unref %" PRIdPTR " -> %" PRIdPTR " %s", alarm, val,
+            val - 1, reason);
+  }
+
+  alarm_unref(alarm);
+}
+#endif
+
+static void alarm_end_completion(grpc_exec_ctx *exec_ctx, void *arg,
+                                 grpc_cq_completion *c) {
+  grpc_alarm *alarm = arg;
+  GRPC_ALARM_UNREF(alarm, "dequeue-end-op");
+}
 
 static void alarm_cb(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) {
   grpc_alarm *alarm = arg;
-  grpc_cq_end_op(exec_ctx, alarm->cq, alarm->tag, error,
-                 do_nothing_end_completion, NULL, &alarm->completion);
+
+  /* We are queuing an op on completion queue. This means, the alarm's structure
+     cannot be destroyed until the op is dequeued. Adding an extra ref
+     here and unref'ing when the op is dequeued will achieve this */
+  GRPC_ALARM_REF(alarm, "queue-end-op");
+  grpc_cq_end_op(exec_ctx, alarm->cq, alarm->tag, error, alarm_end_completion,
+                 (void *)alarm, &alarm->completion);
 }
 
 grpc_alarm *grpc_alarm_create(grpc_completion_queue *cq, gpr_timespec deadline,
@@ -45,6 +96,14 @@ grpc_alarm *grpc_alarm_create(grpc_completion_queue *cq, gpr_timespec deadline,
   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)) {
+    gpr_log(GPR_DEBUG, "Alarm:%p created (ref: 1)", alarm);
+  }
+#endif
+
   GRPC_CQ_INTERNAL_REF(cq, "alarm");
   alarm->cq = cq;
   alarm->tag = tag;
@@ -66,9 +125,6 @@ void grpc_alarm_cancel(grpc_alarm *alarm) {
 }
 
 void grpc_alarm_destroy(grpc_alarm *alarm) {
-  grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT;
   grpc_alarm_cancel(alarm);
-  GRPC_CQ_INTERNAL_UNREF(&exec_ctx, alarm->cq, "alarm");
-  gpr_free(alarm);
-  grpc_exec_ctx_finish(&exec_ctx);
+  GRPC_ALARM_UNREF(alarm, "alarm_destroy");
 }
diff --git a/src/core/lib/surface/alarm_internal.h b/src/core/lib/surface/alarm_internal.h
new file mode 100644
index 0000000000..540ac692d5
--- /dev/null
+++ b/src/core/lib/surface/alarm_internal.h
@@ -0,0 +1,39 @@
+/*
+ *
+ * Copyright 2015-2017 gRPC authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+#ifndef GRPC_CORE_LIB_SURFACE_ALARM_INTERNAL_H
+#define GRPC_CORE_LIB_SURFACE_ALARM_INTERNAL_H
+
+#include <grpc/support/log.h>
+#include "src/core/lib/debug/trace.h"
+
+#ifndef NDEBUG
+
+extern grpc_tracer_flag grpc_trace_alarm_refcount;
+#define GRPC_ALARM_REF(a, reason) alarm_ref_dbg(a, reason, __FILE__, __LINE__)
+#define GRPC_ALARM_UNREF(a, reason) alarm_unref_dbg(a, reason, __FILE__, __LINE__)
+
+#else /* !defined(NDEBUG) */
+
+#define GRPC_ALARM_REF(a, reason) alarm_ref(a)
+#define GRPC_ALARM_UNREF(a, reason) alarm_unref(a)
+
+#endif /* defined(NDEBUG) */
+
+
+#endif // GRPC_CORE_LIB_SURFACE_ALARM_INTERNAL_H
diff --git a/src/core/lib/surface/init.c b/src/core/lib/surface/init.c
index 14a86bfa0a..7a5f398c96 100644
--- a/src/core/lib/surface/init.c
+++ b/src/core/lib/surface/init.c
@@ -36,6 +36,7 @@
 #include "src/core/lib/iomgr/resource_quota.h"
 #include "src/core/lib/profiling/timers.h"
 #include "src/core/lib/slice/slice_internal.h"
+#include "src/core/lib/surface/alarm_internal.h"
 #include "src/core/lib/surface/api_trace.h"
 #include "src/core/lib/surface/call.h"
 #include "src/core/lib/surface/channel_init.h"
@@ -137,6 +138,7 @@ void grpc_init(void) {
     grpc_register_tracer("call_error", &grpc_call_error_trace);
 #ifndef NDEBUG
     grpc_register_tracer("pending_tags", &grpc_trace_pending_tags);
+    grpc_register_tracer("alarm_refcount", &grpc_trace_alarm_refcount);
     grpc_register_tracer("queue_refcount", &grpc_trace_cq_refcount);
     grpc_register_tracer("closure", &grpc_trace_closure);
     grpc_register_tracer("error_refcount", &grpc_trace_error_refcount);
diff --git a/test/core/surface/alarm_test.c b/test/core/surface/alarm_test.c
index baaa05928e..6971d92074 100644
--- a/test/core/surface/alarm_test.c
+++ b/test/core/surface/alarm_test.c
@@ -73,6 +73,21 @@ static void test_alarm(void) {
     GPR_ASSERT(ev.success == 0);
     grpc_alarm_destroy(alarm);
   }
+  {
+    /* alarm_destroy before cq_next */
+    grpc_event ev;
+    void *tag = create_test_tag();
+    grpc_alarm *alarm =
+        grpc_alarm_create(cc, grpc_timeout_seconds_to_deadline(2), tag);
+
+    grpc_alarm_destroy(alarm);
+    ev = grpc_completion_queue_next(cc, grpc_timeout_seconds_to_deadline(1),
+                                    NULL);
+    GPR_ASSERT(ev.type == GRPC_OP_COMPLETE);
+    GPR_ASSERT(ev.tag == tag);
+    GPR_ASSERT(ev.success == 0);
+  }
+
   shutdown_and_destroy(cc);
 }
 
diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal
index 63067b3081..fcc8104488 100644
--- a/tools/doxygen/Doxyfile.core.internal
+++ b/tools/doxygen/Doxyfile.core.internal
@@ -1332,6 +1332,7 @@ src/core/lib/support/tmpfile_windows.c \
 src/core/lib/support/wrap_memcpy.c \
 src/core/lib/surface/README.md \
 src/core/lib/surface/alarm.c \
+src/core/lib/surface/alarm_internal.h \
 src/core/lib/surface/api_trace.c \
 src/core/lib/surface/api_trace.h \
 src/core/lib/surface/byte_buffer.c \
diff --git a/tools/run_tests/generated/sources_and_headers.json b/tools/run_tests/generated/sources_and_headers.json
index 0c4e1fae1d..bf3e00d3c3 100644
--- a/tools/run_tests/generated/sources_and_headers.json
+++ b/tools/run_tests/generated/sources_and_headers.json
@@ -7684,6 +7684,7 @@
       "src/core/lib/slice/slice_hash_table.h", 
       "src/core/lib/slice/slice_internal.h", 
       "src/core/lib/slice/slice_string_helpers.h", 
+      "src/core/lib/surface/alarm_internal.h", 
       "src/core/lib/surface/api_trace.h", 
       "src/core/lib/surface/call.h", 
       "src/core/lib/surface/call_test_only.h", 
@@ -7902,6 +7903,7 @@
       "src/core/lib/slice/slice_string_helpers.c", 
       "src/core/lib/slice/slice_string_helpers.h", 
       "src/core/lib/surface/alarm.c", 
+      "src/core/lib/surface/alarm_internal.h", 
       "src/core/lib/surface/api_trace.c", 
       "src/core/lib/surface/api_trace.h", 
       "src/core/lib/surface/byte_buffer.c", 
diff --git a/vsprojects/vcxproj/grpc/grpc.vcxproj b/vsprojects/vcxproj/grpc/grpc.vcxproj
index f9badbbd35..da02f14ec5 100644
--- a/vsprojects/vcxproj/grpc/grpc.vcxproj
+++ b/vsprojects/vcxproj/grpc/grpc.vcxproj
@@ -383,6 +383,7 @@
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\slice\slice_hash_table.h" />
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\slice\slice_internal.h" />
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\slice\slice_string_helpers.h" />
+    <ClInclude Include="$(SolutionDir)\..\src\core\lib\surface\alarm_internal.h" />
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\surface\api_trace.h" />
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\surface\call.h" />
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\surface\call_test_only.h" />
diff --git a/vsprojects/vcxproj/grpc/grpc.vcxproj.filters b/vsprojects/vcxproj/grpc/grpc.vcxproj.filters
index f3e997e4e0..3cc1fcf511 100644
--- a/vsprojects/vcxproj/grpc/grpc.vcxproj.filters
+++ b/vsprojects/vcxproj/grpc/grpc.vcxproj.filters
@@ -1094,6 +1094,9 @@
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\slice\slice_string_helpers.h">
       <Filter>src\core\lib\slice</Filter>
     </ClInclude>
+    <ClInclude Include="$(SolutionDir)\..\src\core\lib\surface\alarm_internal.h">
+      <Filter>src\core\lib\surface</Filter>
+    </ClInclude>
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\surface\api_trace.h">
       <Filter>src\core\lib\surface</Filter>
     </ClInclude>
diff --git a/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj b/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj
index 1899be91db..edcbc15565 100644
--- a/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj
+++ b/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj
@@ -278,6 +278,7 @@
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\slice\slice_hash_table.h" />
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\slice\slice_internal.h" />
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\slice\slice_string_helpers.h" />
+    <ClInclude Include="$(SolutionDir)\..\src\core\lib\surface\alarm_internal.h" />
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\surface\api_trace.h" />
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\surface\call.h" />
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\surface\call_test_only.h" />
diff --git a/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj.filters b/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj.filters
index 9c81fc5bf5..73bfc23131 100644
--- a/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj.filters
+++ b/vsprojects/vcxproj/grpc_test_util/grpc_test_util.vcxproj.filters
@@ -827,6 +827,9 @@
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\slice\slice_string_helpers.h">
       <Filter>src\core\lib\slice</Filter>
     </ClInclude>
+    <ClInclude Include="$(SolutionDir)\..\src\core\lib\surface\alarm_internal.h">
+      <Filter>src\core\lib\surface</Filter>
+    </ClInclude>
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\surface\api_trace.h">
       <Filter>src\core\lib\surface</Filter>
     </ClInclude>
diff --git a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj
index 3a865f3e81..7c6f72971c 100644
--- a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj
+++ b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj
@@ -373,6 +373,7 @@
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\slice\slice_hash_table.h" />
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\slice\slice_internal.h" />
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\slice\slice_string_helpers.h" />
+    <ClInclude Include="$(SolutionDir)\..\src\core\lib\surface\alarm_internal.h" />
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\surface\api_trace.h" />
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\surface\call.h" />
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\surface\call_test_only.h" />
diff --git a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters
index 0310ebc012..a737429d40 100644
--- a/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters
+++ b/vsprojects/vcxproj/grpc_unsecure/grpc_unsecure.vcxproj.filters
@@ -1004,6 +1004,9 @@
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\slice\slice_string_helpers.h">
       <Filter>src\core\lib\slice</Filter>
     </ClInclude>
+    <ClInclude Include="$(SolutionDir)\..\src\core\lib\surface\alarm_internal.h">
+      <Filter>src\core\lib\surface</Filter>
+    </ClInclude>
     <ClInclude Include="$(SolutionDir)\..\src\core\lib\surface\api_trace.h">
       <Filter>src\core\lib\surface</Filter>
     </ClInclude>