Merge pull request #14335 from carl-mastrangelo/ret

Check return values from ServiceReflection calls, and print a more helpful message
diff --git a/BUILD b/BUILD
index 1779575..78479ce 100644
--- a/BUILD
+++ b/BUILD
@@ -599,6 +599,7 @@
     public_hdrs = ["src/core/lib/gprpp/orphanable.h"],
     deps = [
         "debug_location",
+        "ref_counted_ptr",
         "gpr++_base",
         "grpc_trace",
     ],
@@ -610,6 +611,7 @@
     public_hdrs = ["src/core/lib/gprpp/ref_counted.h"],
     deps = [
         "debug_location",
+        "ref_counted_ptr",
         "gpr++_base",
         "grpc_trace",
     ],
diff --git a/include/grpc++/impl/codegen/client_context.h b/include/grpc++/impl/codegen/client_context.h
index ff6b107..38cce27 100644
--- a/include/grpc++/impl/codegen/client_context.h
+++ b/include/grpc++/impl/codegen/client_context.h
@@ -304,7 +304,10 @@
   /// Flag whether the initial metadata should be \a corked
   ///
   /// If \a corked is true, then the initial metadata will be coalesced with the
-  /// write of first message in the stream.
+  /// write of first message in the stream. As a result, any tag set for the
+  /// initial metadata operation (starting a client-streaming or bidi-streaming
+  /// RPC) will not actually be sent to the completion queue or delivered
+  /// via Next.
   ///
   /// \param corked The flag indicating whether the initial metadata is to be
   /// corked or not.
@@ -332,6 +335,10 @@
   /// already finished, it may still return success.
   ///
   /// There is no guarantee the call will be cancelled.
+  ///
+  /// Note that TryCancel() does not change any of the tags that are pending
+  /// on the completion queue. All pending tags will still be delivered
+  /// (though their ok result may reflect the effect of cancellation).
   void TryCancel();
 
   /// Global Callbacks
diff --git a/include/grpc++/impl/codegen/completion_queue.h b/include/grpc++/impl/codegen/completion_queue.h
index b8a7862..83477d0 100644
--- a/include/grpc++/impl/codegen/completion_queue.h
+++ b/include/grpc++/impl/codegen/completion_queue.h
@@ -111,12 +111,83 @@
 
   /// Tri-state return for AsyncNext: SHUTDOWN, GOT_EVENT, TIMEOUT.
   enum NextStatus {
-    SHUTDOWN,   ///< The completion queue has been shutdown.
+    SHUTDOWN,   ///< The completion queue has been shutdown and fully-drained
     GOT_EVENT,  ///< Got a new event; \a tag will be filled in with its
                 ///< associated value; \a ok indicating its success.
     TIMEOUT     ///< deadline was reached.
   };
 
+  /// Read from the queue, blocking until an event is available or the queue is
+  /// shutting down.
+  ///
+  /// \param tag[out] Updated to point to the read event's tag.
+  /// \param ok[out] true if read a successful event, false otherwise.
+  ///
+  /// Note that each tag sent to the completion queue (through RPC operations
+  /// or alarms) will be delivered out of the completion queue by a call to
+  /// Next (or a related method), regardless of whether the operation succeeded
+  /// or not. Success here means that this operation completed in the normal
+  /// valid manner.
+  ///
+  /// Server-side RPC request: \a ok indicates that the RPC has indeed
+  /// been started. If it is false, the server has been Shutdown
+  /// before this particular call got matched to an incoming RPC.
+  ///
+  /// Client-side StartCall/RPC invocation: \a ok indicates that the RPC is
+  /// going to go to the wire. If it is false, it not going to the wire. This
+  /// would happen if the channel is either permanently broken or
+  /// transiently broken but with the fail-fast option. (Note that async unary
+  /// RPCs don't post a CQ tag at this point, nor do client-streaming
+  /// or bidi-streaming RPCs that have the initial metadata corked option set.)
+  ///
+  /// Client-side Write, Client-side WritesDone, Server-side Write,
+  /// Server-side Finish, Server-side SendInitialMetadata (which is
+  /// typically included in Write or Finish when not done explicitly):
+  /// \a ok means that the data/metadata/status/etc is going to go to the
+  /// wire. If it is false, it not going to the wire because the call
+  /// is already dead (i.e., canceled, deadline expired, other side
+  /// dropped the channel, etc).
+  ///
+  /// Client-side Read, Server-side Read, Client-side
+  /// RecvInitialMetadata (which is typically included in Read if not
+  /// done explicitly): \a ok indicates whether there is a valid message
+  /// that got read. If not, you know that there are certainly no more
+  /// messages that can ever be read from this stream. For the client-side
+  /// operations, this only happens because the call is dead. For the
+  /// server-sider operation, though, this could happen because the client
+  /// has done a WritesDone already.
+  ///
+  /// Client-side Finish: \a ok should always be true
+  ///
+  /// Server-side AsyncNotifyWhenDone: \a ok should always be true
+  ///
+  /// Alarm: \a ok is true if it expired, false if it was canceled
+  ///
+  /// \return true if got an event, false if the queue is fully drained and
+  ///         shut down.
+  bool Next(void** tag, bool* ok) {
+    return (AsyncNextInternal(tag, ok,
+                              g_core_codegen_interface->gpr_inf_future(
+                                  GPR_CLOCK_REALTIME)) != SHUTDOWN);
+  }
+
+  /// Read from the queue, blocking up to \a deadline (or the queue's shutdown).
+  /// Both \a tag and \a ok are updated upon success (if an event is available
+  /// within the \a deadline).  A \a tag points to an arbitrary location usually
+  /// employed to uniquely identify an event.
+  ///
+  /// \param tag[out] Upon sucess, updated to point to the event's tag.
+  /// \param ok[out] Upon sucess, true if a successful event, false otherwise
+  ///        See documentation for CompletionQueue::Next for explanation of ok
+  /// \param deadline[in] How long to block in wait for an event.
+  ///
+  /// \return The type of event read.
+  template <typename T>
+  NextStatus AsyncNext(void** tag, bool* ok, const T& deadline) {
+    TimePoint<T> deadline_tp(deadline);
+    return AsyncNextInternal(tag, ok, deadline_tp.raw_time());
+  }
+
   /// EXPERIMENTAL
   /// First executes \a F, then reads from the queue, blocking up to
   /// \a deadline (or the queue's shutdown).
@@ -141,44 +212,16 @@
     }
   }
 
-  /// Read from the queue, blocking up to \a deadline (or the queue's shutdown).
-  /// Both \a tag and \a ok are updated upon success (if an event is available
-  /// within the \a deadline).  A \a tag points to an arbitrary location usually
-  /// employed to uniquely identify an event.
-  ///
-  /// \param tag[out] Upon sucess, updated to point to the event's tag.
-  /// \param ok[out] Upon sucess, true if read a regular event, false otherwise.
-  /// \param deadline[in] How long to block in wait for an event.
-  ///
-  /// \return The type of event read.
-  template <typename T>
-  NextStatus AsyncNext(void** tag, bool* ok, const T& deadline) {
-    TimePoint<T> deadline_tp(deadline);
-    return AsyncNextInternal(tag, ok, deadline_tp.raw_time());
-  }
-
-  /// Read from the queue, blocking until an event is available or the queue is
-  /// shutting down.
-  ///
-  /// \param tag[out] Updated to point to the read event's tag.
-  /// \param ok[out] true if read a regular event, false otherwise.
-  ///
-  /// \return true if read a regular event, false if the queue is shutting down.
-  bool Next(void** tag, bool* ok) {
-    return (AsyncNextInternal(tag, ok,
-                              g_core_codegen_interface->gpr_inf_future(
-                                  GPR_CLOCK_REALTIME)) != SHUTDOWN);
-  }
-
   /// Request the shutdown of the queue.
   ///
   /// \warning This method must be called at some point if this completion queue
-  /// is accessed with Next or AsyncNext. Once invoked, \a Next
-  /// will start to return false and \a AsyncNext will return \a
-  /// NextStatus::SHUTDOWN. Only once either one of these methods does that
-  /// (that is, once the queue has been \em drained) can an instance of this
-  /// class be destroyed. Also note that applications must ensure that
-  /// no work is enqueued on this completion queue after this method is called.
+  /// is accessed with Next or AsyncNext. \a Next will not return false
+  /// until this method has been called and all pending tags have been drained.
+  /// (Likewise for \a AsyncNext returning \a NextStatus::SHUTDOWN .)
+  /// Only once either one of these methods does that (that is, once the queue
+  /// has been \em drained) can an instance of this class be destroyed.
+  /// Also note that applications must ensure that no work is enqueued on this
+  /// completion queue after this method is called.
   void Shutdown();
 
   /// Returns a \em raw pointer to the underlying \a grpc_completion_queue
diff --git a/include/grpc++/impl/codegen/server_context.h b/include/grpc++/impl/codegen/server_context.h
index c369758..57347f4 100644
--- a/include/grpc++/impl/codegen/server_context.h
+++ b/include/grpc++/impl/codegen/server_context.h
@@ -151,6 +151,10 @@
   /// The only exception is that if the serverhandler is already returning an
   /// error status code, it is ok to not return Status::CANCELLED even if
   /// TryCancel() was called.
+  ///
+  /// Note that TryCancel() does not change any of the tags that are pending
+  /// on the completion queue. All pending tags will still be delivered
+  /// (though their ok result may reflect the effect of cancellation).
   void TryCancel() const;
 
   /// Return a collection of initial metadata key-value pairs sent from the
diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc
index dc1beee..6bf710c 100644
--- a/src/core/ext/filters/client_channel/subchannel.cc
+++ b/src/core/ext/filters/client_channel/subchannel.cc
@@ -738,8 +738,9 @@
 }
 
 namespace grpc_core {
+
 ConnectedSubchannel::ConnectedSubchannel(grpc_channel_stack* channel_stack)
-    : grpc_core::RefCountedWithTracing(&grpc_trace_stream_refcount),
+    : RefCountedWithTracing<ConnectedSubchannel>(&grpc_trace_stream_refcount),
       channel_stack_(channel_stack) {}
 
 ConnectedSubchannel::~ConnectedSubchannel() {
@@ -774,7 +775,9 @@
       args.arena,
       sizeof(grpc_subchannel_call) + channel_stack_->call_stack_size);
   grpc_call_stack* callstk = SUBCHANNEL_CALL_TO_CALL_STACK(*call);
-  Ref(DEBUG_LOCATION, "subchannel_call");
+  RefCountedPtr<ConnectedSubchannel> connection =
+      Ref(DEBUG_LOCATION, "subchannel_call");
+  connection.release();  // Ref is passed to the grpc_subchannel_call object.
   (*call)->connection = this;
   const grpc_call_element_args call_args = {
       callstk,           /* call_stack */
@@ -796,4 +799,5 @@
   grpc_call_stack_set_pollset_or_pollset_set(callstk, args.pollent);
   return GRPC_ERROR_NONE;
 }
+
 }  // namespace grpc_core
diff --git a/src/core/ext/filters/client_channel/subchannel.h b/src/core/ext/filters/client_channel/subchannel.h
index b7593ec..d2b45ae 100644
--- a/src/core/ext/filters/client_channel/subchannel.h
+++ b/src/core/ext/filters/client_channel/subchannel.h
@@ -68,7 +68,8 @@
 #endif
 
 namespace grpc_core {
-class ConnectedSubchannel : public grpc_core::RefCountedWithTracing {
+
+class ConnectedSubchannel : public RefCountedWithTracing<ConnectedSubchannel> {
  public:
   struct CallArgs {
     grpc_polling_entity* pollent;
@@ -93,6 +94,7 @@
  private:
   grpc_channel_stack* channel_stack_;
 };
+
 }  // namespace grpc_core
 
 grpc_subchannel* grpc_subchannel_ref(
diff --git a/src/core/lib/gprpp/orphanable.h b/src/core/lib/gprpp/orphanable.h
index 5019973..6e127c2 100644
--- a/src/core/lib/gprpp/orphanable.h
+++ b/src/core/lib/gprpp/orphanable.h
@@ -28,6 +28,7 @@
 #include "src/core/lib/gprpp/abstract.h"
 #include "src/core/lib/gprpp/debug_location.h"
 #include "src/core/lib/gprpp/memory.h"
+#include "src/core/lib/gprpp/ref_counted_ptr.h"
 
 namespace grpc_core {
 
@@ -69,6 +70,7 @@
 }
 
 // A type of Orphanable with internal ref-counting.
+template <typename Child>
 class InternallyRefCounted : public Orphanable {
  public:
   // Not copyable nor movable.
@@ -78,10 +80,20 @@
   GRPC_ABSTRACT_BASE_CLASS
 
  protected:
+  // Allow Delete() to access destructor.
+  template <typename T>
+  friend void Delete(T*);
+
+  // Allow RefCountedPtr<> to access Unref() and IncrementRefCount().
+  friend class RefCountedPtr<Child>;
+
   InternallyRefCounted() { gpr_ref_init(&refs_, 1); }
   virtual ~InternallyRefCounted() {}
 
-  void Ref() { gpr_ref(&refs_); }
+  RefCountedPtr<Child> Ref() GRPC_MUST_USE_RESULT {
+    IncrementRefCount();
+    return RefCountedPtr<Child>(reinterpret_cast<Child*>(this));
+  }
 
   void Unref() {
     if (gpr_unref(&refs_)) {
@@ -89,11 +101,9 @@
     }
   }
 
-  // Allow Delete() to access destructor.
-  template <typename T>
-  friend void Delete(T*);
-
  private:
+  void IncrementRefCount() { gpr_ref(&refs_); }
+
   gpr_refcount refs_;
 };
 
@@ -103,6 +113,7 @@
 // pointers and legacy code that is manually calling Ref() and Unref().
 // Once all of our code is converted to idiomatic C++, we may be able to
 // eliminate this class.
+template <typename Child>
 class InternallyRefCountedWithTracing : public Orphanable {
  public:
   // Not copyable nor movable.
@@ -118,6 +129,9 @@
   template <typename T>
   friend void Delete(T*);
 
+  // Allow RefCountedPtr<> to access Unref() and IncrementRefCount().
+  friend class RefCountedPtr<Child>;
+
   InternallyRefCountedWithTracing()
       : InternallyRefCountedWithTracing(static_cast<TraceFlag*>(nullptr)) {}
 
@@ -133,18 +147,27 @@
 
   virtual ~InternallyRefCountedWithTracing() {}
 
-  void Ref() { gpr_ref(&refs_); }
+  RefCountedPtr<Child> Ref() GRPC_MUST_USE_RESULT {
+    IncrementRefCount();
+    return RefCountedPtr<Child>(reinterpret_cast<Child*>(this));
+  }
 
-  void Ref(const DebugLocation& location, const char* reason) {
+  RefCountedPtr<Child> Ref(const DebugLocation& location,
+                           const char* reason) GRPC_MUST_USE_RESULT {
     if (location.Log() && trace_flag_ != nullptr && trace_flag_->enabled()) {
       gpr_atm old_refs = gpr_atm_no_barrier_load(&refs_.count);
       gpr_log(GPR_DEBUG, "%s:%p %s:%d ref %" PRIdPTR " -> %" PRIdPTR " %s",
               trace_flag_->name(), this, location.file(), location.line(),
               old_refs, old_refs + 1, reason);
     }
-    Ref();
+    return Ref();
   }
 
+  // TODO(roth): Once all of our code is converted to C++ and can use
+  // RefCountedPtr<> instead of manual ref-counting, make the Unref() methods
+  // private, since they will only be used by RefCountedPtr<>, which is a
+  // friend of this class.
+
   void Unref() {
     if (gpr_unref(&refs_)) {
       Delete(this);
@@ -162,6 +185,8 @@
   }
 
  private:
+  void IncrementRefCount() { gpr_ref(&refs_); }
+
   TraceFlag* trace_flag_ = nullptr;
   gpr_refcount refs_;
 };
diff --git a/src/core/lib/gprpp/ref_counted.h b/src/core/lib/gprpp/ref_counted.h
index c68118a..16c7912 100644
--- a/src/core/lib/gprpp/ref_counted.h
+++ b/src/core/lib/gprpp/ref_counted.h
@@ -26,16 +26,28 @@
 #include "src/core/lib/gprpp/abstract.h"
 #include "src/core/lib/gprpp/debug_location.h"
 #include "src/core/lib/gprpp/memory.h"
+#include "src/core/lib/gprpp/ref_counted_ptr.h"
 
 namespace grpc_core {
 
 // A base class for reference-counted objects.
 // New objects should be created via New() and start with a refcount of 1.
 // When the refcount reaches 0, the object will be deleted via Delete().
+//
+// This will commonly be used by CRTP (curiously-recurring template pattern)
+// e.g., class MyClass : public RefCounted<MyClass>
+template <typename Child>
 class RefCounted {
  public:
-  void Ref() { gpr_ref(&refs_); }
+  RefCountedPtr<Child> Ref() GRPC_MUST_USE_RESULT {
+    IncrementRefCount();
+    return RefCountedPtr<Child>(reinterpret_cast<Child*>(this));
+  }
 
+  // TODO(roth): Once all of our code is converted to C++ and can use
+  // RefCountedPtr<> instead of manual ref-counting, make this method
+  // private, since it will only be used by RefCountedPtr<>, which is a
+  // friend of this class.
   void Unref() {
     if (gpr_unref(&refs_)) {
       Delete(this);
@@ -58,6 +70,11 @@
   virtual ~RefCounted() {}
 
  private:
+  // Allow RefCountedPtr<> to access IncrementRefCount().
+  friend class RefCountedPtr<Child>;
+
+  void IncrementRefCount() { gpr_ref(&refs_); }
+
   gpr_refcount refs_;
 };
 
@@ -67,20 +84,30 @@
 // pointers and legacy code that is manually calling Ref() and Unref().
 // Once all of our code is converted to idiomatic C++, we may be able to
 // eliminate this class.
+template <typename Child>
 class RefCountedWithTracing {
  public:
-  void Ref() { gpr_ref(&refs_); }
+  RefCountedPtr<Child> Ref() GRPC_MUST_USE_RESULT {
+    IncrementRefCount();
+    return RefCountedPtr<Child>(reinterpret_cast<Child*>(this));
+  }
 
-  void Ref(const DebugLocation& location, const char* reason) {
+  RefCountedPtr<Child> Ref(const DebugLocation& location,
+                           const char* reason) GRPC_MUST_USE_RESULT {
     if (location.Log() && trace_flag_ != nullptr && trace_flag_->enabled()) {
       gpr_atm old_refs = gpr_atm_no_barrier_load(&refs_.count);
       gpr_log(GPR_DEBUG, "%s:%p %s:%d ref %" PRIdPTR " -> %" PRIdPTR " %s",
               trace_flag_->name(), this, location.file(), location.line(),
               old_refs, old_refs + 1, reason);
     }
-    Ref();
+    return Ref();
   }
 
+  // TODO(roth): Once all of our code is converted to C++ and can use
+  // RefCountedPtr<> instead of manual ref-counting, make the Unref() methods
+  // private, since they will only be used by RefCountedPtr<>, which is a
+  // friend of this class.
+
   void Unref() {
     if (gpr_unref(&refs_)) {
       Delete(this);
@@ -124,6 +151,11 @@
   virtual ~RefCountedWithTracing() {}
 
  private:
+  // Allow RefCountedPtr<> to access IncrementRefCount().
+  friend class RefCountedPtr<Child>;
+
+  void IncrementRefCount() { gpr_ref(&refs_); }
+
   TraceFlag* trace_flag_ = nullptr;
   gpr_refcount refs_;
 };
diff --git a/src/core/lib/gprpp/ref_counted_ptr.h b/src/core/lib/gprpp/ref_counted_ptr.h
index dda0f00..f82ba50 100644
--- a/src/core/lib/gprpp/ref_counted_ptr.h
+++ b/src/core/lib/gprpp/ref_counted_ptr.h
@@ -25,8 +25,8 @@
 
 namespace grpc_core {
 
-// A smart pointer class for objects that provide Ref() and Unref() methods,
-// such as those provided by the RefCounted base class.
+// A smart pointer class for objects that provide IncrementRefCount() and
+// Unref() methods, such as those provided by the RefCounted base class.
 template <typename T>
 class RefCountedPtr {
  public:
@@ -49,13 +49,13 @@
 
   // Copy support.
   RefCountedPtr(const RefCountedPtr& other) {
-    if (other.value_ != nullptr) other.value_->Ref();
+    if (other.value_ != nullptr) other.value_->IncrementRefCount();
     value_ = other.value_;
   }
   RefCountedPtr& operator=(const RefCountedPtr& other) {
     // Note: Order of reffing and unreffing is important here in case value_
     // and other.value_ are the same object.
-    if (other.value_ != nullptr) other.value_->Ref();
+    if (other.value_ != nullptr) other.value_->IncrementRefCount();
     if (value_ != nullptr) value_->Unref();
     value_ = other.value_;
     return *this;
@@ -71,6 +71,16 @@
     value_ = value;
   }
 
+  // TODO(roth): This method exists solely as a transition mechanism to allow
+  // us to pass a ref to idiomatic C code that does not use RefCountedPtr<>.
+  // Once all of our code has been converted to idiomatic C++, this
+  // method should go away.
+  T* release() {
+    T* value = value_;
+    value_ = nullptr;
+    return value;
+  }
+
   T* get() const { return value_; }
 
   T& operator*() const { return *value_; }
diff --git a/test/core/gprpp/orphanable_test.cc b/test/core/gprpp/orphanable_test.cc
index ff2f6d8..ad6b9ac 100644
--- a/test/core/gprpp/orphanable_test.cc
+++ b/test/core/gprpp/orphanable_test.cc
@@ -58,18 +58,19 @@
   EXPECT_EQ(5, foo->value());
 }
 
-class Bar : public InternallyRefCounted {
+class Bar : public InternallyRefCounted<Bar> {
  public:
   Bar() : Bar(0) {}
   explicit Bar(int value) : value_(value) {}
   void Orphan() override { Unref(); }
   int value() const { return value_; }
 
-  void StartWork() { Ref(); }
-  void FinishWork() { Unref(); }
+  void StartWork() { self_ref_ = Ref(); }
+  void FinishWork() { self_ref_.reset(); }
 
  private:
   int value_;
+  RefCountedPtr<Bar> self_ref_;
 };
 
 TEST(OrphanablePtr, InternallyRefCounted) {
@@ -82,19 +83,24 @@
 // things build properly in both debug and non-debug cases.
 DebugOnlyTraceFlag baz_tracer(true, "baz");
 
-class Baz : public InternallyRefCountedWithTracing {
+class Baz : public InternallyRefCountedWithTracing<Baz> {
  public:
   Baz() : Baz(0) {}
   explicit Baz(int value)
-      : InternallyRefCountedWithTracing(&baz_tracer), value_(value) {}
+      : InternallyRefCountedWithTracing<Baz>(&baz_tracer), value_(value) {}
   void Orphan() override { Unref(); }
   int value() const { return value_; }
 
-  void StartWork() { Ref(DEBUG_LOCATION, "work"); }
-  void FinishWork() { Unref(DEBUG_LOCATION, "work"); }
+  void StartWork() { self_ref_ = Ref(DEBUG_LOCATION, "work"); }
+  void FinishWork() {
+    // This is a little ugly, but it makes the logged ref and unref match up.
+    self_ref_.release();
+    Unref(DEBUG_LOCATION, "work");
+  }
 
  private:
   int value_;
+  RefCountedPtr<Baz> self_ref_;
 };
 
 TEST(OrphanablePtr, InternallyRefCountedWithTracing) {
diff --git a/test/core/gprpp/ref_counted_ptr_test.cc b/test/core/gprpp/ref_counted_ptr_test.cc
index f1f13f3..2e398a7 100644
--- a/test/core/gprpp/ref_counted_ptr_test.cc
+++ b/test/core/gprpp/ref_counted_ptr_test.cc
@@ -30,7 +30,7 @@
 namespace testing {
 namespace {
 
-class Foo : public RefCounted {
+class Foo : public RefCounted<Foo> {
  public:
   Foo() : value_(0) {}
 
@@ -163,14 +163,15 @@
 
 TraceFlag foo_tracer(true, "foo");
 
-class FooWithTracing : public RefCountedWithTracing {
+class FooWithTracing : public RefCountedWithTracing<FooWithTracing> {
  public:
   FooWithTracing() : RefCountedWithTracing(&foo_tracer) {}
 };
 
 TEST(RefCountedPtr, RefCountedWithTracing) {
   RefCountedPtr<FooWithTracing> foo(New<FooWithTracing>());
-  foo->Ref(DEBUG_LOCATION, "foo");
+  RefCountedPtr<FooWithTracing> foo2 = foo->Ref(DEBUG_LOCATION, "foo");
+  foo2.release();
   foo->Unref(DEBUG_LOCATION, "foo");
 }
 
diff --git a/test/core/gprpp/ref_counted_test.cc b/test/core/gprpp/ref_counted_test.cc
index b1b0fee..f85a2e4 100644
--- a/test/core/gprpp/ref_counted_test.cc
+++ b/test/core/gprpp/ref_counted_test.cc
@@ -27,7 +27,7 @@
 namespace testing {
 namespace {
 
-class Foo : public RefCounted {
+class Foo : public RefCounted<Foo> {
  public:
   Foo() {}
 };
@@ -39,7 +39,8 @@
 
 TEST(RefCounted, ExtraRef) {
   Foo* foo = New<Foo>();
-  foo->Ref();
+  RefCountedPtr<Foo> foop = foo->Ref();
+  foop.release();
   foo->Unref();
   foo->Unref();
 }
@@ -48,17 +49,19 @@
 // things build properly in both debug and non-debug cases.
 DebugOnlyTraceFlag foo_tracer(true, "foo");
 
-class FooWithTracing : public RefCountedWithTracing {
+class FooWithTracing : public RefCountedWithTracing<FooWithTracing> {
  public:
   FooWithTracing() : RefCountedWithTracing(&foo_tracer) {}
 };
 
 TEST(RefCountedWithTracing, Basic) {
   FooWithTracing* foo = New<FooWithTracing>();
-  foo->Ref(DEBUG_LOCATION, "extra_ref");
+  RefCountedPtr<FooWithTracing> foop = foo->Ref(DEBUG_LOCATION, "extra_ref");
+  foop.release();
   foo->Unref(DEBUG_LOCATION, "extra_ref");
   // Can use the no-argument methods, too.
-  foo->Ref();
+  foop = foo->Ref();
+  foop.release();
   foo->Unref();
   foo->Unref(DEBUG_LOCATION, "original_ref");
 }