Reviewer comments

CMakeLists.txt

@@ -12738,6 +12738,8 @@ target_link_libraries(context_list_test
   ${_gRPC_ALLTARGETS_LIBRARIES}
   grpc_test_util
   grpc
+  gpr_test_util
+  gpr
   ${_gRPC_GFLAGS_LIBRARIES}
 )
 

Makefile

@@ -17582,16 +17582,16 @@ $(BINDIR)/$(CONFIG)/context_list_test: protobuf_dep_error
 
 else
 
-$(BINDIR)/$(CONFIG)/context_list_test: $(PROTOBUF_DEP) $(CONTEXT_LIST_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a
+$(BINDIR)/$(CONFIG)/context_list_test: $(PROTOBUF_DEP) $(CONTEXT_LIST_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a
 	$(E) "[LD]      Linking $@"
 	$(Q) mkdir -p `dirname $@`
-	$(Q) $(LDXX) $(LDFLAGS) $(CONTEXT_LIST_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LDLIBSXX) $(LDLIBS_PROTOBUF) $(LDLIBS) $(LDLIBS_SECURE) $(GTEST_LIB) -o $(BINDIR)/$(CONFIG)/context_list_test
+	$(Q) $(LDXX) $(LDFLAGS) $(CONTEXT_LIST_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a $(LDLIBSXX) $(LDLIBS_PROTOBUF) $(LDLIBS) $(LDLIBS_SECURE) $(GTEST_LIB) -o $(BINDIR)/$(CONFIG)/context_list_test
 
 endif
 
 endif
 
-$(OBJDIR)/$(CONFIG)/test/core/transport/chttp2/context_list_test.o:  $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a
+$(OBJDIR)/$(CONFIG)/test/core/transport/chttp2/context_list_test.o:  $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a
 
 deps_context_list_test: $(CONTEXT_LIST_TEST_OBJS:.o=.dep)
 

build.yaml

@@ -4605,6 +4605,7 @@ targets:
   - grpc++_codegen_base_src
   uses_polling: false
 - name: context_list_test
+  gtest: true
   build: test
   language: c++
   src:
@@ -4612,6 +4613,8 @@ targets:
   deps:
   - grpc_test_util
   - grpc
+  - gpr_test_util
+  - gpr
   uses_polling: false
 - name: credentials_test
   gtest: true

src/core/ext/transport/chttp2/transport/context_list.cc

@@ -21,30 +21,30 @@
 #include "src/core/ext/transport/chttp2/transport/context_list.h"
 
 namespace {
-void (*cb)(void*, grpc_core::Timestamps*) = nullptr;
+void (*write_timestamps_callback_g)(void*, grpc_core::Timestamps*) = nullptr;
 }
 
 namespace grpc_core {
 void ContextList::Execute(void* arg, grpc_core::Timestamps* ts,
                           grpc_error* error) {
   ContextList* head = static_cast<ContextList*>(arg);
-  ContextList* ptr;
+  ContextList* to_be_freed;
   while (head != nullptr) {
     if (error == GRPC_ERROR_NONE && ts != nullptr) {
-      if (cb) {
-        cb(head->s_->context, ts);
+      if (write_timestamps_callback_g) {
+        write_timestamps_callback_g(head->s_->context, ts);
       }
     }
     GRPC_CHTTP2_STREAM_UNREF(static_cast<grpc_chttp2_stream*>(head->s_),
                              "timestamp");
-    ptr = head;
+    to_be_freed = head;
     head = head->next_;
-    grpc_core::Delete(ptr);
+    grpc_core::Delete(to_be_freed);
   }
 }
 
 void grpc_http2_set_write_timestamps_callback(
     void (*fn)(void*, grpc_core::Timestamps*)) {
-  cb = fn;
+  write_timestamps_callback_g = fn;
 }
 } /* namespace grpc_core */

src/core/ext/transport/chttp2/transport/context_list.h

@@ -16,8 +16,8 @@
  *
  */
 
-#ifndef GRPC_CORE_EXT_TRANSPORT_CONTEXT_LIST_H
-#define GRPC_CORE_EXT_TRANSPORT_CONTEXT_LIST_H
+#ifndef GRPC_CORE_EXT_TRANSPORT_CHTTP2_TRANSPORT_CONTEXT_LIST_H
+#define GRPC_CORE_EXT_TRANSPORT_CHTTP2_TRANSPORT_CONTEXT_LIST_H
 
 #include <grpc/support/port_platform.h>
 
@@ -33,8 +33,10 @@ class ContextList {
    * list. */
   static void Append(ContextList** head, grpc_chttp2_stream* s) {
     /* Make sure context is not already present */
-    ContextList* ptr = *head;
     GRPC_CHTTP2_STREAM_REF(s, "timestamp");
+
+#ifndef NDEBUG
+    ContextList* ptr = *head;
     while (ptr != nullptr) {
       if (ptr->s_ == s) {
         GPR_ASSERT(
@@ -43,17 +45,13 @@ class ContextList {
       }
       ptr = ptr->next_;
     }
+#endif
+
+    /* Create a new element in the list and add it at the front */
     ContextList* elem = grpc_core::New<ContextList>();
     elem->s_ = s;
-    if (*head == nullptr) {
-      *head = elem;
-      return;
-    }
-    ptr = *head;
-    while (ptr->next_ != nullptr) {
-      ptr = ptr->next_;
-    }
-    ptr->next_ = elem;
+    elem->next_ = *head;
+    *head = elem;
   }
 
   /* Executes a function \a fn with each context in the list and \a ts. It also
@@ -69,4 +67,4 @@ void grpc_http2_set_write_timestamps_callback(
     void (*fn)(void*, grpc_core::Timestamps*));
 } /* namespace grpc_core */
 
-#endif /* GRPC_CORE_EXT_TRANSPORT_CONTEXT_LIST_H */
+#endif /* GRPC_CORE_EXT_TRANSPORT_CHTTP2_TRANSPORT_CONTEXT_LIST_H */

src/core/lib/iomgr/endpoint.cc

@@ -63,8 +63,5 @@ grpc_resource_user* grpc_endpoint_get_resource_user(grpc_endpoint* ep) {
 }
 
 bool grpc_endpoint_can_track_err(grpc_endpoint* ep) {
-  if (ep->vtable->can_track_err != nullptr) {
-    return ep->vtable->can_track_err(ep);
-  }
-  return false;
+  return ep->vtable->can_track_err(ep);
 }

test/core/transport/chttp2/BUILD

@@ -69,6 +69,9 @@ grpc_cc_test(
 grpc_cc_test(
     name = "context_list_test",
     srcs = ["context_list_test.cc"],
+    external_deps = [
+        "gtest",
+    ],
     language = "C++",
     deps = [
         "//:gpr",

test/core/transport/chttp2/context_list_test.cc

@@ -18,6 +18,7 @@
 
 #include "src/core/lib/iomgr/port.h"
 
+#include <gtest/gtest.h>
 #include <new>
 #include <vector>
 
@@ -29,25 +30,28 @@
 
 #include <grpc/grpc.h>
 
-static void TestExecuteFlushesListVerifier(void* arg,
-                                           grpc_core::Timestamps* ts) {
+namespace grpc_core {
+namespace testing {
+namespace {
+void TestExecuteFlushesListVerifier(void* arg, grpc_core::Timestamps* ts) {
   GPR_ASSERT(arg != nullptr);
   gpr_atm* done = reinterpret_cast<gpr_atm*>(arg);
   gpr_atm_rel_store(done, static_cast<gpr_atm>(1));
 }
 
-static void discard_write(grpc_slice slice) {}
+void discard_write(grpc_slice slice) {}
 
 /** Tests that all ContextList elements in the list are flushed out on
  * execute.
  * Also tests that arg is passed correctly.
  */
-static void TestExecuteFlushesList() {
+TEST(ContextList, ExecuteFlushesList) {
   grpc_core::ContextList* list = nullptr;
   grpc_http2_set_write_timestamps_callback(TestExecuteFlushesListVerifier);
-#define NUM_ELEM 5
+  const int kNumElems = 5;
   grpc_core::ExecCtx exec_ctx;
   grpc_stream_refcount ref;
+  GRPC_STREAM_REF_INIT(&ref, 1, nullptr, nullptr, "dummy ref");
   grpc_resource_quota* resource_quota =
       grpc_resource_quota_create("context_list_test");
   grpc_endpoint* mock_endpoint =
@@ -55,9 +59,9 @@ static void TestExecuteFlushesList() {
   grpc_transport* t =
       grpc_create_chttp2_transport(nullptr, mock_endpoint, true);
   std::vector<grpc_chttp2_stream*> s;
-  s.reserve(NUM_ELEM);
-  gpr_atm verifier_called[NUM_ELEM];
-  for (auto i = 0; i < NUM_ELEM; i++) {
+  s.reserve(kNumElems);
+  gpr_atm verifier_called[kNumElems];
+  for (auto i = 0; i < kNumElems; i++) {
     s.push_back(static_cast<grpc_chttp2_stream*>(
         gpr_malloc(grpc_transport_stream_size(t))));
     grpc_transport_init_stream(reinterpret_cast<grpc_transport*>(t),
@@ -69,7 +73,7 @@ static void TestExecuteFlushesList() {
   }
   grpc_core::Timestamps ts;
   grpc_core::ContextList::Execute(list, &ts, GRPC_ERROR_NONE);
-  for (auto i = 0; i < NUM_ELEM; i++) {
+  for (auto i = 0; i < kNumElems; i++) {
     GPR_ASSERT(gpr_atm_acq_load(&verifier_called[i]) ==
                static_cast<gpr_atm>(1));
     grpc_transport_destroy_stream(reinterpret_cast<grpc_transport*>(t),
@@ -82,12 +86,13 @@ static void TestExecuteFlushesList() {
   grpc_resource_quota_unref(resource_quota);
   exec_ctx.Flush();
 }
-
-static void TestContextList() { TestExecuteFlushesList(); }
+}  // namespace
+}  // namespace testing
+}  // namespace grpc_core
 
 int main(int argc, char** argv) {
+  grpc_test_init(argc, argv);
   grpc_init();
-  TestContextList();
-  grpc_shutdown();
-  return 0;
+  ::testing::InitGoogleTest(&argc, argv);
+  return RUN_ALL_TESTS();
 }

tools/run_tests/generated/sources_and_headers.json

@@ -3509,6 +3509,8 @@
   }, 
   {
     "deps": [
+      "gpr", 
+      "gpr_test_util", 
       "grpc", 
       "grpc_test_util"
     ], 

tools/run_tests/generated/tests.json

@@ -4136,7 +4136,7 @@
     "exclude_configs": [], 
     "exclude_iomgrs": [], 
     "flaky": false, 
-    "gtest": false, 
+    "gtest": true, 
     "language": "c++", 
     "name": "context_list_test", 
     "platforms": [