Back out "Split setting up decoratedProcessorFactory from initializat…

…ion of ServerModule" Summary: This caused S487919 since it enabled SAP authorization on a service. Backing out so we can roll this out more safely Differential Revision: D69152403 fbshipit-source-id: c16d690437e690d24f96dd0897cb391141281227
facebook · Feb 5, 2025 · 27f22aa · 27f22aa
1 parent 9ec1d33
commit 27f22aa
Show file tree

Hide file tree

Showing 3 changed files with 20 additions and 53 deletions.
diff --git a/third-party/thrift/src/thrift/lib/cpp2/server/ThriftServer.cpp b/third-party/thrift/src/thrift/lib/cpp2/server/ThriftServer.cpp
@@ -1122,10 +1122,11 @@ bool ThriftServer::runtimeResourcePoolsChecks() {
     }
     runtimeDisableResourcePoolsDeprecated();
   } else {
-    // Check whether there are any wildcard services.
     // Need to set this up now to check.
-    auto methodMetadata =
-        ensureDecoratedProcessorFactoryInitialized().createMethodMetadata();
+    ensureProcessedServiceDescriptionInitialized();
+
+    // Check whether there are any wildcard services.
+    auto methodMetadata = getDecoratedProcessorFactory().createMethodMetadata();
 
     if (auto* methodMetadataMap =
             std::get_if<AsyncProcessorFactory::MethodMetadataMap>(
@@ -1497,7 +1498,6 @@ void ThriftServer::cleanUp() {
   // Clear the service description so that it's re-created if the server
   // is restarted.
   processedServiceDescription_.reset();
-  decoratedProcessorFactory_.reset();
 }
 
 uint64_t ThriftServer::getNumDroppedConnections() const {
@@ -1679,30 +1679,24 @@ void ThriftServer::stopAcceptingAndJoinOutstandingRequests() {
   internalStatus_.store(ServerStatus::NOT_RUNNING, std::memory_order_release);
 }
 
-AsyncProcessorFactory&
-ThriftServer::ensureDecoratedProcessorFactoryInitialized() {
+void ThriftServer::ensureProcessedServiceDescriptionInitialized() {
   DCHECK(getProcessorFactory().get());
-  if (decoratedProcessorFactory_ == nullptr) {
-    decoratedProcessorFactory_ = createDecoratedProcessorFactory(
+  if (processedServiceDescription_ == nullptr) {
+    auto modules = processModulesSpecification(
+        std::exchange(unprocessedModulesSpecification_, {}));
+    auto decoratedProcessorFactory = createDecoratedProcessorFactory(
         getProcessorFactory(),
         getStatusInterface(),
         getMonitoringInterface(),
         getControlInterface(),
         getSecurityInterface(),
         isCheckUnimplementedExtraInterfacesAllowed() &&
             THRIFT_FLAG(server_check_unimplemented_extra_interfaces));
-  }
-  return *decoratedProcessorFactory_;
-}
-
-void ThriftServer::ensureProcessedServiceDescriptionInitialized() {
-  ensureDecoratedProcessorFactoryInitialized();
-  if (processedServiceDescription_ == nullptr) {
-    auto modules = processModulesSpecification(
-        std::exchange(unprocessedModulesSpecification_, {}));
     processedServiceDescription_ =
         ProcessedServiceDescription::createAndActivate(
-            *this, ProcessedServiceDescription{std::move(modules)});
+            *this,
+            ProcessedServiceDescription{
+                std::move(modules), std::move(decoratedProcessorFactory)});
   }
 }
 

diff --git a/third-party/thrift/src/thrift/lib/cpp2/server/ThriftServer.h b/third-party/thrift/src/thrift/lib/cpp2/server/ThriftServer.h
@@ -1899,7 +1899,6 @@ class ThriftServer : public apache::thrift::concurrency::Runnable,
   void callOnStartServing();
   void callOnStopRequested();
 
-  AsyncProcessorFactory& ensureDecoratedProcessorFactoryInitialized();
   void ensureProcessedServiceDescriptionInitialized();
 
   bool serverRanWithDCHECK();
@@ -2071,9 +2070,13 @@ class ThriftServer : public apache::thrift::concurrency::Runnable,
 
   struct ProcessedServiceDescription {
     ProcessedModuleSet modules;
+    std::unique_ptr<AsyncProcessorFactory> decoratedProcessorFactory;
 
-    ProcessedServiceDescription(ProcessedModuleSet moduleSet)
-        : modules(std::move(moduleSet)) {}
+    ProcessedServiceDescription(
+        ProcessedModuleSet moduleSet,
+        std::unique_ptr<AsyncProcessorFactory> processorFactory)
+        : modules(std::move(moduleSet)),
+          decoratedProcessorFactory(std::move(processorFactory)) {}
 
     ProcessedServiceDescription(ProcessedServiceDescription&& modules) =
         default;
@@ -2120,7 +2123,6 @@ class ThriftServer : public apache::thrift::concurrency::Runnable,
       return ptr;
     }
   };
-  std::unique_ptr<AsyncProcessorFactory> decoratedProcessorFactory_;
   ProcessedServiceDescription::UniquePtr processedServiceDescription_{nullptr};
 
  public:
@@ -2135,18 +2137,6 @@ class ThriftServer : public apache::thrift::concurrency::Runnable,
         ModulesSpecification::Info{std::move(module), std::move(name)});
   }
 
-  bool hasModule(const std::string_view name) const noexcept {
-    CHECK(processedServiceDescription_)
-        << "Server must be set up before calling this method";
-    for (const auto& moduleInfo :
-         processedServiceDescription_->modules.modules) {
-      if (moduleInfo.name == name) {
-        return true;
-      }
-    }
-    return false;
-  }
-
  private:
   /**
    * Collects service handlers of the current service of a specific type.
@@ -2815,9 +2805,9 @@ class ThriftServer : public apache::thrift::concurrency::Runnable,
    *    └────────────────────────┘
    */
   AsyncProcessorFactory& getDecoratedProcessorFactory() const {
-    CHECK(decoratedProcessorFactory_)
+    CHECK(processedServiceDescription_)
         << "Server must be set up before calling this method";
-    return *decoratedProcessorFactory_;
+    return *processedServiceDescription_->decoratedProcessorFactory;
   }
 
   /**

diff --git a/third-party/thrift/src/thrift/lib/cpp2/test/server/ThriftServerTest.cpp b/third-party/thrift/src/thrift/lib/cpp2/test/server/ThriftServerTest.cpp
@@ -3872,23 +3872,6 @@ TEST(ThriftServer, SetupThreadManager) {
       [](auto& ts) { ts.setupThreadManager(); });
 }
 
-TEST(ThriftServer, AddModuleAfterSetupThreadManager) {
-  ScopedServerInterfaceThread runner(
-      std::make_shared<apache::thrift::ServiceHandler<TestService>>(),
-      "::1",
-      0,
-      [](auto& ts) {
-        class TestModule : public apache::thrift::ServerModule {
-         public:
-          std::string getName() const override { return "TestModule"; }
-        };
-        ts.setupThreadManager();
-        ts.addModule(std::make_unique<TestModule>());
-      });
-
-  EXPECT_EQ(runner.getThriftServer().hasModule("TestModule"), true);
-}
-
 TEST(ThriftServer, GetSetMaxRequests) {
   for (auto target : std::array<uint32_t, 2>{1000, 0}) {
     {