[BACKPORT 2.20][#23747] MetaCache: Callback should not be called whil…

…e holding the lock

Summary:
Original commit: c770d79 / D37706
Call callback in ScopeExit block only. Not while holding the lock.

Without this fix, it is possible that a thread can get into a deadlock, trying to request a shared_lock on a mutex, while already holding an exclusive lock on the same mutex:

This deadlock can be triggered if there are active read/write requests to a Table (from more than 1 thread)  right after the table had a tablet-split.

 If there is only 1 thread, it is unlikely to run into the deadlock, as the thread notices -- as part of the callback -- that the table's partition info is stale. Having a different thread refresh the partition version before the main thread checks if the table version is stale, is likely necessary to trigger the stack trace seen below.

e.g:
```
#0  pthread_cond_wait@@GLIBC_2.3.2 () at ../sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:185
#1  0x00005640c3eb441b in std::__1::shared_timed_mutex::lock_shared() ()
#2  0x00005640c3ffcbff in yb::client::internal::MetaCache::LookupTabletByKey(std::__1::shared_ptr<yb::client::YBTable> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::chrono::time_point<yb::CoarseMonoClock, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000000000l> > >, std::__1::function<void (yb::Result<scoped_refptr<yb::client::internal::RemoteTablet> > const&)>, yb::StronglyTypedBool<yb::client::internal::FailOnPartitionListRefreshed_Tag>) ()
#3  0x00005640c3f7549a in yb::client::internal::Batcher::LookupTabletFor(yb::client::internal::InFlightOp*) ()
#4  0x00005640c401855e in yb::client::(anonymous namespace)::FlushBatcherAsync(std::__1::shared_ptr<yb::client::internal::Batcher> const&, boost::function<void (yb::client::FlushStatus*)>, yb::client::YBSession::BatcherConfig, yb::StronglyTypedBool<yb::client::internal::IsWithinTransactionRetry_Tag>) ()
#5  0x00005640c401aa76 in yb::client::(anonymous namespace)::BatcherFlushDone(std::__1::shared_ptr<yb::client::internal::Batcher> const&, yb::Status const&, boost::function<void (yb::client::FlushStatus*)>, yb::client::YBSession::BatcherConfig) ()
#6  0x00005640c401b371 in boost::detail::function::void_function_obj_invoker1<std::__1::__bind<void (*)(std::__1::shared_ptr<yb::client::internal::Batcher> const&, yb::Status const&, boost::function<void (yb::client::FlushStatus*)>, yb::client::YBSession::BatcherConfig), std::__1::shared_ptr<yb::client::internal::Batcher> const&, std::__1::placeholders::__ph<1> const&, boost::function<void (yb::client::FlushStatus*)>, yb::client::YBSession::BatcherConfig&>, void, yb::Status const&>::invoke(boost::detail::function::function_buffer&, yb::Status const&) ()
#7  0x00005640c3f70398 in yb::client::internal::Batcher::Run() ()
#8  0x00005640c3f72656 in yb::client::internal::Batcher::FlushFinished() ()
#9  0x00005640c3f74a4d in yb::client::internal::Batcher::TabletLookupFinished(yb::client::internal::InFlightOp*, yb::Result<scoped_refptr<yb::client::internal::RemoteTablet> >) ()
#10 0x00005640c3f759bc in std::__1::__function::__func<yb::client::internal::Batcher::LookupTabletFor(yb::client::internal::InFlightOp*)::$_0, std::__1::allocator<yb::client::internal::Batcher::LookupTabletFor(yb::client::internal::InFlightOp*)::$_0>, void (yb::Result<scoped_refptr<yb::client::internal::RemoteTablet> > const&)>::operator()(yb::Result<scoped_refptr<yb::client::internal::RemoteTablet> > const&) ()

#11 0x00005640c3fff05d in yb::client::internal::MetaCache::LookupTabletByKey(std::__1::shared_ptr<yb::client::YBTable> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::chrono::time_point<yb::CoarseMonoClock, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000000000l> > >, std::__1::function<void (yb::Result<scoped_refptr<yb::client::internal::RemoteTablet> > const&)>,  yb::StronglyTypedBool<yb::client::internal::FailOnPartitionListRefreshed_Tag>) ()
** Is holding an exclusive lock in MetaCache::LookupTabletByKey/DoLookupTabletByKey **

#12 0x00005640c3f7549a in yb::client::internal::Batcher::LookupTabletFor(yb::client::internal::InFlightOp*) ()
#13 0x00005640c401855e in yb::client::(anonymous namespace)::FlushBatcherAsync(std::__1::shared_ptr<yb::client::internal::Batcher> const&, boost::function<void (yb::client::FlushStatus*)>, yb::client::YBSession::BatcherConfig, yb::StronglyTypedBool<yb::client::internal::IsWithinTransactionRetry_Tag>) ()
#14 0x00005640c4017130 in yb::client::YBSession::FlushAsync(boost::function<void (yb::client::FlushStatus*)>) ()
#15 0x00005640c5225a0c in yb::tserver::PgClientServiceImpl::Perform(yb::tserver::PgPerformRequestPB const*, yb::tserver::PgPerformResponsePB*, yb::rpc::RpcContext) ()
#16 0x00005640c51c4487 in std::__1::__function::__func<yb::tserver::PgClientServiceIf::InitMethods(scoped_refptr<yb::MetricEntity> const&)::$_20, std::__1::allocator<yb::tserver::PgClientServiceIf::InitMethods(scoped_refptr<yb::MetricEntity> const&)::$_20>, void (std::__1::shared_ptr<yb::rpc::InboundCall>)>::operator()(std::__1::shared_ptr<yb::rpc::InboundCall>&&) ()
#17 0x00005640c51d374f in yb::tserver::PgClientServiceIf::Handle(std::__1::shared_ptr<yb::rpc::InboundCall>) ()
#18 0x00005640c4f5f420 in yb::rpc::ServicePoolImpl::Handle(std::__1::shared_ptr<yb::rpc::InboundCall>) ()
#19 0x00005640c4e845af in yb::rpc::InboundCall::InboundCallTask::Run() ()
#20 0x00005640c4f6e243 in yb::rpc::(anonymous namespace)::Worker::Execute() ()
#21 0x00005640c570ecb4 in yb::Thread::SuperviseThread(void*) ()
#22 0x00007f808b7c6694 in start_thread (arg=0x7f76d8caf700) at pthread_create.c:333
#23 0x00007f808bac341d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
```
Jira: DB-12651

Test Plan:
Jenkins
yb_build.sh --cxx-test ql-stress-test QLStressTest.ReproMetaCacheDeadlock

Reviewers: rthallam, hsunder, qhu, timur

Reviewed By: rthallam

Subscribers: svc_phabricator, ybase

Differential Revision: https://phorge.dev.yugabyte.com/D37789

src/yb/client/meta_cache.cc

@@ -110,6 +110,8 @@ DEFINE_test_flag(bool, force_master_lookup_all_tablets, false,
                  "If set, force the client to go to the master for all tablet lookup "
                  "instead of reading from cache.");
 
+DEFINE_test_flag(int32, sleep_before_metacache_lookup_ms, 0,
+                 "If set, will sleep in LookupTabletByKey for a random amount up to this value.");
 DEFINE_test_flag(double, simulate_lookup_timeout_probability, 0,
                  "If set, mark an RPC as failed and force retry on the first attempt.");
 DEFINE_test_flag(double, simulate_lookup_partition_list_mismatch_probability, 0,
@@ -1883,9 +1885,12 @@ bool MetaCache::DoLookupTabletByKey(
     LookupTabletCallback* callback, PartitionGroupStartKeyPtr* partition_group_start) {
   DCHECK_ONLY_NOTNULL(partition_group_start);
   RemoteTabletPtr tablet;
-  auto scope_exit = ScopeExit([callback, &tablet] {
+  Status status = Status::OK();
+  auto scope_exit = ScopeExit([callback, &tablet, &status] {
     if (tablet) {
       (*callback)(tablet);
+    } else if (!status.ok()) {
+      (*callback)(status);
     }
   });
   int64_t request_no;
@@ -1912,13 +1917,13 @@ bool MetaCache::DoLookupTabletByKey(
         (PREDICT_FALSE(RandomActWithProbability(
             FLAGS_TEST_simulate_lookup_partition_list_mismatch_probability)) &&
          table->table_type() != YBTableType::TRANSACTION_STATUS_TABLE_TYPE)) {
-      (*callback)(STATUS(
+      status = STATUS(
           TryAgain,
           Format(
               "MetaCache's table $0 partitions version does not match, cached: $1, got: $2, "
               "refresh required",
               table->ToString(), table_data->partition_list->version, partitions->version),
-          ClientError(ClientErrorCode::kTablePartitionListIsStale)));
+          ClientError(ClientErrorCode::kTablePartitionListIsStale));
       return true;
     }
 
@@ -2025,6 +2030,12 @@ void MetaCache::LookupTabletByKey(const std::shared_ptr<YBTable>& table,
     return;
   }
 
+  if (FLAGS_TEST_sleep_before_metacache_lookup_ms > 0) {
+    MonoDelta sleep_time = MonoDelta::FromMilliseconds(1) *
+                           RandomUniformInt(1, FLAGS_TEST_sleep_before_metacache_lookup_ms);
+    SleepFor(sleep_time);
+    VLOG_WITH_FUNC(2) << "Slept for " << sleep_time;
+  }
   if (table->ArePartitionsStale()) {
     RefreshTablePartitions(
         table,
@@ -2118,9 +2129,12 @@ bool MetaCache::DoLookupTabletById(
     UseCache use_cache,
     LookupTabletCallback* callback) {
   std::optional<RemoteTabletPtr> tablet = std::nullopt;
-  auto scope_exit = ScopeExit([callback, &tablet] {
+  Status status = Status::OK();
+  auto scope_exit = ScopeExit([callback, &tablet, &status] {
     if (tablet) {
       (*callback)(*tablet);
+    } else if (!status.ok()) {
+      (*callback)(status);
     }
   });
   int64_t request_no;
@@ -2136,7 +2150,7 @@ bool MetaCache::DoLookupTabletById(
         if (use_cache) {
           if (!include_deleted) {
             tablet = std::nullopt;
-            (*callback)(STATUS(NotFound, "Tablet deleted"));
+            status = STATUS(NotFound, "Tablet deleted");
           }
           return true;
         }

src/yb/client/ql-stress-test.cc

@@ -74,8 +74,10 @@ DECLARE_bool(detect_duplicates_for_retryable_requests);
 DECLARE_bool(enable_ondisk_compression);
 DECLARE_bool(ycql_enable_packed_row);
 DECLARE_double(TEST_respond_write_failed_probability);
+DECLARE_double(TEST_simulate_lookup_partition_list_mismatch_probability);
 DECLARE_double(transaction_max_missed_heartbeat_periods);
 DECLARE_int32(TEST_max_write_waiters);
+DECLARE_int32(TEST_sleep_before_metacache_lookup_ms);
 DECLARE_int32(client_read_write_timeout_ms);
 DECLARE_int32(log_cache_size_limit_mb);
 DECLARE_int32(log_min_seconds_to_retain);
@@ -441,6 +443,12 @@ TEST_F(QLStressTest, RetryWritesWithRestarts) {
   TestRetryWrites(true /* restarts */);
 }
 
+TEST_F(QLStressTest, ReproMetaCacheDeadlock) {
+  ANNOTATE_UNPROTECTED_WRITE(FLAGS_TEST_simulate_lookup_partition_list_mismatch_probability) = 0.8;
+  ANNOTATE_UNPROTECTED_WRITE(FLAGS_TEST_sleep_before_metacache_lookup_ms) = 50;
+  TestRetryWrites(true /* restarts */);
+}
+
 void SetTransactional(YBSchemaBuilder* builder) {
   TableProperties table_properties;
   table_properties.SetTransactional(true);