Merge pull request #17536 from yashykt/fathom

Avoid taking refs on the stream by getting a copy of the context

3 files changed, 41 insertions, 39 deletions

diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc
index 9b6574b612..7f4627fa77 100644
--- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc
+++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc
@@ -170,7 +170,12 @@ grpc_chttp2_transport::~grpc_chttp2_transport() {
   grpc_slice_buffer_destroy_internal(&outbuf);
   grpc_chttp2_hpack_compressor_destroy(&hpack_compressor);
 
-  grpc_core::ContextList::Execute(cl, nullptr, GRPC_ERROR_NONE);
+  grpc_error* error =
+      GRPC_ERROR_CREATE_FROM_STATIC_STRING("Transport destroyed");
+  // ContextList::Execute follows semantics of a callback function and does not
+  // take a ref on error
+  grpc_core::ContextList::Execute(cl, nullptr, error);
+  GRPC_ERROR_UNREF(error);
   cl = nullptr;
 
   grpc_slice_buffer_destroy_internal(&read_buffer);
diff --git a/src/core/ext/transport/chttp2/transport/context_list.cc b/src/core/ext/transport/chttp2/transport/context_list.cc
index f30d41c332..df09809067 100644
--- a/src/core/ext/transport/chttp2/transport/context_list.cc
+++ b/src/core/ext/transport/chttp2/transport/context_list.cc
@@ -21,31 +21,47 @@
 #include "src/core/ext/transport/chttp2/transport/context_list.h"
 
 namespace {
-void (*write_timestamps_callback_g)(void*, grpc_core::Timestamps*) = nullptr;
-}
+void (*write_timestamps_callback_g)(void*, grpc_core::Timestamps*,
+                                    grpc_error* error) = nullptr;
+void* (*get_copied_context_fn_g)(void*) = nullptr;
+}  // namespace
 
 namespace grpc_core {
+void ContextList::Append(ContextList** head, grpc_chttp2_stream* s) {
+  if (get_copied_context_fn_g == nullptr ||
+      write_timestamps_callback_g == nullptr) {
+    return;
+  }
+  /* Create a new element in the list and add it at the front */
+  ContextList* elem = grpc_core::New<ContextList>();
+  elem->trace_context_ = get_copied_context_fn_g(s->context);
+  elem->byte_offset_ = s->byte_counter;
+  elem->next_ = *head;
+  *head = elem;
+}
+
 void ContextList::Execute(void* arg, grpc_core::Timestamps* ts,
                           grpc_error* error) {
   ContextList* head = static_cast<ContextList*>(arg);
   ContextList* to_be_freed;
   while (head != nullptr) {
-    if (error == GRPC_ERROR_NONE && ts != nullptr) {
-      if (write_timestamps_callback_g) {
-        ts->byte_offset = static_cast<uint32_t>(head->byte_offset_);
-        write_timestamps_callback_g(head->s_->context, ts);
-      }
+    if (write_timestamps_callback_g) {
+      ts->byte_offset = static_cast<uint32_t>(head->byte_offset_);
+      write_timestamps_callback_g(head->trace_context_, ts, error);
     }
-    GRPC_CHTTP2_STREAM_UNREF(static_cast<grpc_chttp2_stream*>(head->s_),
-                             "timestamp");
     to_be_freed = head;
     head = head->next_;
     grpc_core::Delete(to_be_freed);
   }
 }
 
-void grpc_http2_set_write_timestamps_callback(
-    void (*fn)(void*, grpc_core::Timestamps*)) {
+void grpc_http2_set_write_timestamps_callback(void (*fn)(void*,
+                                                         grpc_core::Timestamps*,
+                                                         grpc_error* error)) {
   write_timestamps_callback_g = fn;
 }
+
+void grpc_http2_set_fn_get_copied_context(void* (*fn)(void*)) {
+  get_copied_context_fn_g = fn;
+}
 } /* namespace grpc_core */
diff --git a/src/core/ext/transport/chttp2/transport/context_list.h b/src/core/ext/transport/chttp2/transport/context_list.h
index d870107749..5b9d2ab378 100644
--- a/src/core/ext/transport/chttp2/transport/context_list.h
+++ b/src/core/ext/transport/chttp2/transport/context_list.h
@@ -31,42 +31,23 @@ class ContextList {
  public:
   /* Creates a new element with \a context as the value and appends it to the
    * list. */
-  static void Append(ContextList** head, grpc_chttp2_stream* s) {
-    /* Make sure context is not already present */
-    GRPC_CHTTP2_STREAM_REF(s, "timestamp");
-
-#ifndef NDEBUG
-    ContextList* ptr = *head;
-    while (ptr != nullptr) {
-      if (ptr->s_ == s) {
-        GPR_ASSERT(
-            false &&
-            "Trying to append a stream that is already present in the list");
-      }
-      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;
-    elem->byte_offset_ = s->byte_counter;
-    elem->next_ = *head;
-    *head = elem;
-  }
+  static void Append(ContextList** head, grpc_chttp2_stream* s);
 
   /* Executes a function \a fn with each context in the list and \a ts. It also
-   * frees up the entire list after this operation. */
+   * frees up the entire list after this operation. It is intended as a callback
+   * and hence does not take a ref on \a error */
   static void Execute(void* arg, grpc_core::Timestamps* ts, grpc_error* error);
 
  private:
-  grpc_chttp2_stream* s_ = nullptr;
+  void* trace_context_ = nullptr;
   ContextList* next_ = nullptr;
   size_t byte_offset_ = 0;
 };
 
-void grpc_http2_set_write_timestamps_callback(
-    void (*fn)(void*, grpc_core::Timestamps*));
+void grpc_http2_set_write_timestamps_callback(void (*fn)(void*,
+                                                         grpc_core::Timestamps*,
+                                                         grpc_error* error));
+void grpc_http2_set_fn_get_copied_context(void* (*fn)(void*));
 } /* namespace grpc_core */
 
 #endif /* GRPC_CORE_EXT_TRANSPORT_CHTTP2_TRANSPORT_CONTEXT_LIST_H */