From 8feb16171a3633339ec6d11b5aba3ebca2e4c0de Mon Sep 17 00:00:00 2001
From: Vijay Pai <vpai@google.com>
Date: Mon, 25 Feb 2019 00:48:11 -0800
Subject: [PATCH] Add an expectation and fix a ServerContext bug

---
 src/cpp/server/server_context.cc      | 15 ++++++++++-----
 test/cpp/end2end/test_service_impl.cc | 15 ++++++++++++---
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/src/cpp/server/server_context.cc b/src/cpp/server/server_context.cc
index 1b524bc3e83..e116cf6e7d2 100644
--- a/src/cpp/server/server_context.cc
+++ b/src/cpp/server/server_context.cc
@@ -138,7 +138,7 @@ class ServerContext::CompletionOp final : public internal::CallOpSetInterface {
   }
 
   internal::Call call_;
-  internal::ServerReactor* reactor_;
+  internal::ServerReactor* const reactor_;
   bool has_tag_;
   void* tag_;
   void* core_cq_tag_;
@@ -200,12 +200,17 @@ bool ServerContext::CompletionOp::FinalizeResult(void** tag, bool* status) {
     cancelled_ = 1;
   }
 
-  if (cancelled_ && (reactor_ != nullptr)) {
+  // Decide whether to call the cancel callback before releasing the lock
+  bool call_cancel = (cancelled_ != 0);
+
+  // Release the lock since we are going to be calling a callback and
+  // interceptors now
+  lock.unlock();
+
+  if (call_cancel && (reactor_ != nullptr)) {
     reactor_->OnCancel();
   }
-  /* Release the lock since we are going to be running through interceptors now
-   */
-  lock.unlock();
+
   /* Add interception point and run through interceptors */
   interceptor_methods_.AddInterceptionHookPoint(
       experimental::InterceptionHookPoints::POST_RECV_CLOSE);
diff --git a/test/cpp/end2end/test_service_impl.cc b/test/cpp/end2end/test_service_impl.cc
index 8c2df1acc33..baebdbc8091 100644
--- a/test/cpp/end2end/test_service_impl.cc
+++ b/test/cpp/end2end/test_service_impl.cc
@@ -583,7 +583,10 @@ CallbackTestServiceImpl::RequestStream() {
       StartRead(&request_);
     }
     void OnDone() override { delete this; }
-    void OnCancel() override { FinishOnce(Status::CANCELLED); }
+    void OnCancel() override {
+      EXPECT_TRUE(ctx_->IsCancelled());
+      FinishOnce(Status::CANCELLED);
+    }
     void OnReadDone(bool ok) override {
       if (ok) {
         response_->mutable_message()->append(request_.message());
@@ -666,7 +669,10 @@ CallbackTestServiceImpl::ResponseStream() {
       }
     }
     void OnDone() override { delete this; }
-    void OnCancel() override { FinishOnce(Status::CANCELLED); }
+    void OnCancel() override {
+      EXPECT_TRUE(ctx_->IsCancelled());
+      FinishOnce(Status::CANCELLED);
+    }
     void OnWriteDone(bool ok) override {
       if (num_msgs_sent_ < server_responses_to_send_) {
         NextWrite();
@@ -749,7 +755,10 @@ CallbackTestServiceImpl::BidiStream() {
       StartRead(&request_);
     }
     void OnDone() override { delete this; }
-    void OnCancel() override { FinishOnce(Status::CANCELLED); }
+    void OnCancel() override {
+      EXPECT_TRUE(ctx_->IsCancelled());
+      FinishOnce(Status::CANCELLED);
+    }
     void OnReadDone(bool ok) override {
       if (ok) {
         num_msgs_read_++;