kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From a...@apache.org
Subject [kudu] branch master updated: KUDU-2946: avoid ServicePool inc/dec in Messenger::QueueInboundCall
Date Thu, 19 Sep 2019 00:49:18 GMT
This is an automated email from the ASF dual-hosted git repository.

adar pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git

The following commit(s) were added to refs/heads/master by this push:
     new 49308a1  KUDU-2946: avoid ServicePool inc/dec in Messenger::QueueInboundCall
49308a1 is described below

commit 49308a199bfc39b080d8d65190fb63b2a287cb7b
Author: Adar Dembo <adar@cloudera.com>
AuthorDate: Wed Sep 18 16:09:29 2019 -0700

    KUDU-2946: avoid ServicePool inc/dec in Messenger::QueueInboundCall
    Before commit 0ecc2c771, this function took the Messenger lock for what
    seemed like longer than necessary. To reduce the size of the critical
    section, we had to explicitly take a ServicePool ref, then release it at the
    end of the function.
    This turned out to have unintended side effects: it's possible for the
    release to drop the last ref and destroy the ServicePool. The problem with
    this is that QueueInboundCall is called by the reactor thread and destroying
    the ServicePool is a blocking operation.
    Rather than untangle the morass of _how_ the reactor thread ended up with
    the last ServicePool ref, let's just revert this portion of that commit.
    Change-Id: Id759617dcb13a2533e9ce071880c43678b700d25
    Reviewed-on: http://gerrit.cloudera.org:8080/14259
    Reviewed-by: Andrew Wong <awong@cloudera.com>
    Tested-by: Kudu Jenkins
 src/kudu/rpc/messenger.cc | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/kudu/rpc/messenger.cc b/src/kudu/rpc/messenger.cc
index 871192a..fd9d939 100644
--- a/src/kudu/rpc/messenger.cc
+++ b/src/kudu/rpc/messenger.cc
@@ -372,7 +372,15 @@ void Messenger::QueueOutboundCall(const shared_ptr<OutboundCall>
&call) {
 void Messenger::QueueInboundCall(gscoped_ptr<InboundCall> call) {
-  const auto service = rpc_service(call->remote_method().service_name());
+  // This lock acquisition spans the entirety of the function to avoid having to
+  // take a ref on the RpcService. In doing so, we guarantee that the service
+  // isn't shut down here, which would be problematic because shutdown is a
+  // blocking operation and QueueInboundCall is called by the reactor thread.
+  //
+  // See KUDU-2946 for more details.
+  shared_lock<rw_spinlock> guard(lock_.get_lock());
+  scoped_refptr<RpcService>* service = FindOrNull(rpc_services_,
+                                                  call->remote_method().service_name());
   if (PREDICT_FALSE(!service)) {
     Status s =  Status::ServiceUnavailable(Substitute("service $0 not registered on $1",
@@ -381,10 +389,10 @@ void Messenger::QueueInboundCall(gscoped_ptr<InboundCall> call)
-  call->set_method_info(service->LookupMethod(call->remote_method()));
+  call->set_method_info((*service)->LookupMethod(call->remote_method()));
   // The RpcService will respond to the client on success or failure.
-  WARN_NOT_OK(service->QueueInboundCall(std::move(call)), "Unable to handle RPC call");
+  WARN_NOT_OK((*service)->QueueInboundCall(std::move(call)), "Unable to handle RPC call");
 void Messenger::QueueCancellation(const shared_ptr<OutboundCall> &call) {

View raw message