Back out "Revert D68910844"

Summary:
This is an unland of an unland.

Meta:

I did my original unland due to it being plausible that the diff was triggering a latent bug in the THRIFT_SERVER_EVENT handler and crashing in tsp_global/di_capacity/quota_enforcement_manager. It was worth attempting the unland as a potential mitigation to unblock the release of that service's binaries.
The service continues to crash without this diff being in prod, so mitigation of that problem continues. Meanwhile, I would like to re-land this diff since it there is no longer any reason to suspect it causes problems.

Reviewed By: sazonovkirill

Differential Revision: D71045479

fbshipit-source-id: 39eea82404423df24324b66732a5f35cb28d0c2a

Author: Cameron Evans

thrift/lib/cpp2/server/ThriftServer.cpp

@@ -1973,6 +1973,40 @@ folly::Optional<OverloadResult> ThriftServer::checkOverload(
         LoadShedder::CUSTOM};
   }
 
+  // Log services that might break if ConcurrencyController's executionLimit is
+  // unsynced from maxRequests and is synced with only concurrencyLimit instead.
+  if (folly::test_once(cancelSetMaxRequestsCallbackHandleFlag_) ||
+      folly::test_once(serviceMightRelyOnSyncedMaxRequestsFlag_)) {
+    // This is okay. Either maxRequests syncing was cancelled, or we already
+    // logged that the service might rely on the syncing.
+  } else if (
+      thriftConfig_.getMaxRequests().get() ==
+      thriftConfig_.getConcurrencyLimit().get()) {
+    // This is okay. When ConcurrencyController's executionLimit is synced with
+    // concurrencyLimit instead of maxRequests, there will be no difference
+    // since the values are the same.
+  } else if (
+      !isActiveRequestsTrackingDisabled() &&
+      !THRIFT_FLAG(enforce_queue_concurrency_resource_pools) &&
+      !getMethodsBypassMaxRequestsLimit().contains(method)) {
+    // This is okay. No bypass method is enabled. ThriftServer is strictly
+    // rejecting requests once there are maxRequests requests on the server.
+    // ConcurrencyController's will never enforce its executionLimit (synced
+    // from maxRequests) since ThriftServer will never pass enough requests into
+    // the resource pool. After ConcurrencyController's executionLimit is synced
+    // from concurrencyLimit instead, the new default value (uint32_t max) will
+    // continue to be greater than the number of requests that can be passed
+    // into the resource pool.
+  } else {
+    // This is not okay. When ConcurrencyController's executionLimit is unsynced
+    // from maxRequests and synced to concurrencyLimit instead, the service will
+    // encounter a behavioral change.
+    folly::call_once(serviceMightRelyOnSyncedMaxRequestsFlag_, [this]() {
+      LOG(WARNING) << "Service might rely on synced max requests.";
+      THRIFT_SERVER_EVENT(serviceMightRelyOnSyncedMaxRequests).log(*this);
+    });
+  }
+
   // If active request tracking is disabled or we are using resource pools,
   // skip max requests enforcement here. Resource pools has its own separate
   // concurrency limiting mechanism.

thrift/lib/cpp2/server/ThriftServer.h

@@ -1854,6 +1854,10 @@ class ThriftServer : public apache::thrift::concurrency::Runnable,
   // from resource pools, when setConcurrencyLimit is explicitly called.
   folly::once_flag cancelSetMaxRequestsCallbackHandleFlag_;
 
+  // If the service might rely on the synced maxRequests, then we need to log.
+  // Logging only needs to happen once.
+  folly::once_flag serviceMightRelyOnSyncedMaxRequestsFlag_;
+
   struct IdleServerAction : public folly::HHWheelTimer::Callback {
     IdleServerAction(
         ThriftServer& server,