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 <[email protected]>
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 <[email protected]>
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",
call->remote_method().service_name(), name_));
@@ -381,10 +389,10 @@ void Messenger::QueueInboundCall(gscoped_ptr<InboundCall>
call) {
return;
}
- 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) {
