Address reviewer comments

include/grpcpp/server.h

@@ -248,11 +248,11 @@ class Server : public ServerInterface, private GrpcLibraryCodegen {
   /// the \a sync_server_cqs)
   std::vector<std::unique_ptr<SyncRequestThreadManager>> sync_req_mgrs_;
 
-  // Outstanding callback requests. The vector is indexed by method with a
+  // Outstanding callback requests. The vector is indexed by method with a list
-  // list per method. Each element should store its own iterator
+  // per method. Each element should store its own iterator in the list and
-  // in the list and should erase it when the request is actually bound to
+  // should erase it when the request is actually bound to an RPC. Synchronize
-  // an RPC. Synchronize this list with its own mu_ (not the server mu_) since
+  // this list with its own mu_ (not the server mu_) since these must be active
-  // these must be active at Shutdown when the server mu_ is locked
+  // at Shutdown when the server mu_ is locked.
   // TODO(vjpai): Merge with the core request matcher to avoid duplicate work
   struct MethodReqList {
     std::mutex reqs_mu;
@@ -274,13 +274,12 @@ class Server : public ServerInterface, private GrpcLibraryCodegen {
   std::condition_variable shutdown_cv_;
 
   // It is ok (but not required) to nest callback_reqs_mu_ under mu_ .
-  // Incrementing callback_reqs_outstanding_ is ok without a lock
+  // Incrementing callback_reqs_outstanding_ is ok without a lock but it must be
-  // but it should only be decremented under the lock in case it is the
+  // decremented under the lock in case it is the last request and enables the
-  // last request and enables the server shutdown. The increment is
+  // server shutdown. The increment is performance-critical since it happens
-  // performance-critical since it happens during periods of increasing
+  // during periods of increasing load; the decrement happens only when memory
-  // load; the decrement happens only when memory is maxed out, during server
+  // is maxed out, during server shutdown, or (possibly in a future version)
-  // shutdown, or (possibly in a future version) during decreasing load, so
+  // during decreasing load, so it is less performance-critical.
-  // it is less performance-critical.
   std::mutex callback_reqs_mu_;
   std::condition_variable callback_reqs_done_cv_;
   std::atomic_int callback_reqs_outstanding_{0};

src/core/lib/iomgr/executor.cc

@@ -111,11 +111,11 @@ size_t Executor::RunClosures(const char* executor_name,
                              grpc_closure_list list) {
   size_t n = 0;
 
-  // In the executor, the ExecCtx for the thread is declared
+  // In the executor, the ExecCtx for the thread is declared in the executor
-  // in the executor thread itself, but this is the point where we
+  // thread itself, but this is the point where we could start seeing
-  // could start seeing application-level callbacks. No need to
+  // application-level callbacks. No need to create a new ExecCtx, though,
-  // create a new ExecCtx, though, since there already is one and it is
+  // since there already is one and it is flushed (but not destructed) in this
-  // flushed (but not destructed) in this function itself
+  // function itself.
   grpc_core::ApplicationCallbackExecCtx callback_exec_ctx;
 
   grpc_closure* c = list.head;

src/cpp/server/server_cc.cc

@@ -62,11 +62,11 @@ namespace {
 #define DEFAULT_CALLBACK_REQS_PER_METHOD 512
 
 // What is the (soft) limit for outstanding requests in the server
-#define MAXIMUM_CALLBACK_REQS_OUTSTANDING 30000
+#define SOFT_MAXIMUM_CALLBACK_REQS_OUTSTANDING 30000
 
-// If the number of unmatched requests for a method drops below this amount,
+// If the number of unmatched requests for a method drops below this amount, try
-// try to allocate extra unless it pushes the total number of callbacks above
+// to allocate extra unless it pushes the total number of callbacks above the
-// the soft maximum
+// soft maximum
 #define SOFT_MINIMUM_SPARE_CALLBACK_REQS_PER_METHOD 128
 
 class DefaultGlobalCallbacks final : public Server::GlobalCallbacks {
@@ -185,11 +185,10 @@ class Server::SyncRequest final : public internal::CompletionQueueTag {
     GPR_ASSERT(cq_ && !in_flight_);
     in_flight_ = true;
     if (method_tag_) {
-      if (GRPC_CALL_OK !=
+      if (grpc_server_request_registered_call(
-          grpc_server_request_registered_call(
               server, method_tag_, &call_, &deadline_, &request_metadata_,
               has_request_payload_ ? &request_payload_ : nullptr, cq_,
-              notify_cq, this)) {
+              notify_cq, this) != GRPC_CALL_OK) {
         TeardownRequest();
         return;
       }
@@ -452,7 +451,7 @@ class Server::CallbackRequest final : public internal::CompletionQueueTag {
             (req_->req_list_->reqs_list_sz <
                  SOFT_MINIMUM_SPARE_CALLBACK_REQS_PER_METHOD &&
              req_->server_->callback_reqs_outstanding_ <
-                 MAXIMUM_CALLBACK_REQS_OUTSTANDING)) {
+                 SOFT_MAXIMUM_CALLBACK_REQS_OUTSTANDING)) {
           spawn_new = true;
         }
       }
@@ -528,7 +527,7 @@ class Server::CallbackRequest final : public internal::CompletionQueueTag {
                 //              load no longer justifies it. Consider measuring
                 //              dynamic load and setting a target accordingly.
                 if (req_->server_->callback_reqs_outstanding_ <
-                    MAXIMUM_CALLBACK_REQS_OUTSTANDING) {
+                    SOFT_MAXIMUM_CALLBACK_REQS_OUTSTANDING) {
                   req_->Clear();
                   req_->Setup();
                 } else {