Return different Status based on ObjectRegistry::NewObject calls (#9333)

Summary:
This fix addresses https://github.com/facebook/rocksdb/issues/9299.

If attempting to create a new object via the ObjectRegistry and a factory is not found, the ObjectRegistry will return a "NotSupported" status.  This is the same behavior as previously.

If the factory is found but could not successfully create the object, an "InvalidArgument" status is returned.  If the factory returned a reason why (in the errmsg), this message will be in the returned status.

In practice, there are two options in the ConfigOptions that control how these errors are propagated:
- If "ignore_unknown_options=true", then both InvalidArgument and NotSupported status codes will be swallowed internally.  Both cases will return success
- If "ignore_unsupported_options=true", then having no factory will return success but a failing factory will return an error
- If both options are false, both cases (no and failing factory) will return errors.

In practice this likely only changes Customizable that may be partially available.  For example, the JEMallocMemoryAllocator is a built-in allocator that is registered with the system but may not be compiled in.  In this case, the status code for this allocator changed from NotSupported("JEMalloc not available") to InvalidArgumen("JEMalloc not available").  Other Customizable builtins/plugins would have the same semantics.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/9333

Reviewed By: pdillinger

Differential Revision: D33517681

Pulled By: mrambacher

fbshipit-source-id: 8033052d4a4a7b88c2d9f90147b1b4467e51f6fd

HISTORY.md

@@ -58,6 +58,7 @@
 * Remove default implementation of Name() from FileSystemWrapper.
 * Rename `SizeApproximationOptions.include_memtabtles` to `SizeApproximationOptions.include_memtables`.
 * Remove deprecated option DBOptions::max_mem_compaction_level.
+* Return Status::InvalidArgument from ObjectRegistry::NewObject if a factory exists but the object ould not be created (returns NotFound if the factory is missing).
 * Remove deprecated overloads of API DB::GetApproximateSizes.
 * Remove deprecated option DBOptions::new_table_reader_for_compaction_inputs.
 

env/env.cc

@@ -687,12 +687,8 @@ Status Env::CreateFromString(const ConfigOptions& config_options,
   } else {
     RegisterSystemEnvs();
 #ifndef ROCKSDB_LITE
-    std::string errmsg;
+    // First, try to load the Env as a unique object.
-    env = config_options.registry->NewObject<Env>(id, &uniq, &errmsg);
+    status = config_options.registry->NewObject<Env>(id, &env, &uniq);
-    if (!env) {
-      status = Status::NotSupported(
-          std::string("Cannot load environment[") + id + "]: ", errmsg);
-    }
 #else
     status =
         Status::NotSupported("Cannot load environment in LITE mode", value);

include/rocksdb/utilities/object_registry.h

@@ -296,29 +296,31 @@ class ObjectRegistry {
     library->Register(registrar, arg);
   }
 
-  // Creates a new T using the factory function that was registered for this
+  // Finds the factory for target and instantiates a new T.
-  // target.  Searches through the libraries to find the first library where
+  // Returns NotSupported if no factory is found
-  // there is an entry that matches target (see PatternEntry for the matching
+  // Returns InvalidArgument if a factory is found but the factory failed.
-  // rules).
-  //
-  // If no registered functions match, returns nullptr. If multiple functions
-  // match, the factory function used is unspecified.
-  //
-  // Populates guard with result pointer if caller is granted ownership.
-  // Deprecated.  Use NewShared/Static/UniqueObject instead.
   template <typename T>
-  T* NewObject(const std::string& target, std::unique_ptr<T>* guard,
+  Status NewObject(const std::string& target, T** object,
-               std::string* errmsg) {
+                   std::unique_ptr<T>* guard) {
+    assert(guard != nullptr);
     guard->reset();
     auto factory = FindFactory<T>(target);
     if (factory != nullptr) {
-      return factory(target, guard, errmsg);
+      std::string errmsg;
+      *object = factory(target, guard, &errmsg);
+      if (*object != nullptr) {
+        return Status::OK();
+      } else if (errmsg.empty()) {
+        return Status::InvalidArgument(
+            std::string("Could not load ") + T::Type(), target);
       } else {
-      *errmsg = std::string("Could not load ") + T::Type();
+        return Status::InvalidArgument(errmsg, target);
-      return nullptr;
+      }
+    } else {
+      return Status::NotSupported(std::string("Could not load ") + T::Type(),
+                                  target);
     }
   }
-
   // Creates a new unique T using the input factory functions.
   // Returns OK if a new unique T was successfully created
   // Returns NotSupported if the type/target could not be created
@@ -327,11 +329,13 @@ class ObjectRegistry {
   template <typename T>
   Status NewUniqueObject(const std::string& target,
                          std::unique_ptr<T>* result) {
-    std::string errmsg;
+    T* ptr = nullptr;
-    T* ptr = NewObject(target, result, &errmsg);
+    std::unique_ptr<T> guard;
-    if (ptr == nullptr) {
+    Status s = NewObject(target, &ptr, &guard);
-      return Status::NotSupported(errmsg, target);
+    if (!s.ok()) {
-    } else if (*result) {
+      return s;
+    } else if (guard) {
+      result->reset(guard.release());
       return Status::OK();
     } else {
       return Status::InvalidArgument(std::string("Cannot make a unique ") +
@@ -348,11 +352,11 @@ class ObjectRegistry {
   template <typename T>
   Status NewSharedObject(const std::string& target,
                          std::shared_ptr<T>* result) {
-    std::string errmsg;
     std::unique_ptr<T> guard;
-    T* ptr = NewObject(target, &guard, &errmsg);
+    T* ptr = nullptr;
-    if (ptr == nullptr) {
+    Status s = NewObject(target, &ptr, &guard);
-      return Status::NotSupported(errmsg, target);
+    if (!s.ok()) {
+      return s;
     } else if (guard) {
       result->reset(guard.release());
       return Status::OK();
@@ -370,11 +374,11 @@ class ObjectRegistry {
   //                      (meaning it is managed by a unique ptr)
   template <typename T>
   Status NewStaticObject(const std::string& target, T** result) {
-    std::string errmsg;
     std::unique_ptr<T> guard;
-    T* ptr = NewObject(target, &guard, &errmsg);
+    T* ptr = nullptr;
-    if (ptr == nullptr) {
+    Status s = NewObject(target, &ptr, &guard);
-      return Status::NotSupported(errmsg, target);
+    if (!s.ok()) {
+      return s;
     } else if (guard.get()) {
       return Status::InvalidArgument(std::string("Cannot make a static ") +
                                          T::Type() + " from a guarded one ",

memory/memory_allocator_test.cc

@@ -30,11 +30,7 @@ class MemoryAllocatorTest
     std::tie(id_, supported_) = GetParam();
     Status s =
         MemoryAllocator::CreateFromString(ConfigOptions(), id_, &allocator_);
-    if (supported_) {
+    EXPECT_EQ(supported_, s.ok());
-      EXPECT_OK(s);
-    } else if (!s.ok()) {
-      EXPECT_TRUE(s.IsNotSupported());
-    }
   }
   bool IsSupported() { return supported_; }
 
@@ -140,7 +136,7 @@ TEST_F(CreateMemoryAllocatorTest, JemallocOptionsTest) {
   std::string id = std::string("id=") + JemallocNodumpAllocator::kClassName();
   Status s = MemoryAllocator::CreateFromString(config_options_, id, &allocator);
   if (!JemallocNodumpAllocator::IsSupported()) {
-    ASSERT_TRUE(s.IsNotSupported());
+    ASSERT_NOK(s);
     ROCKSDB_GTEST_BYPASS("JEMALLOC not supported");
     return;
   }
@@ -192,7 +188,7 @@ TEST_F(CreateMemoryAllocatorTest, NewJemallocNodumpAllocator) {
   Status s = NewJemallocNodumpAllocator(jopts, &allocator);
   std::string msg;
   if (!JemallocNodumpAllocator::IsSupported(&msg)) {
-    ASSERT_TRUE(s.IsNotSupported());
+    ASSERT_NOK(s);
     ROCKSDB_GTEST_BYPASS("JEMALLOC not supported");
     return;
   }

options/customizable_test.cc

@@ -497,6 +497,55 @@ TEST_F(CustomizableTest, BadOptionTest) {
   ASSERT_OK(c1->ConfigureFromString(ignore, "shared.id=A;A.string=s}"));
 }
 
+TEST_F(CustomizableTest, FailingFactoryTest) {
+  std::shared_ptr<ObjectRegistry> registry = ObjectRegistry::NewInstance();
+  std::unique_ptr<Configurable> c1(new SimpleConfigurable());
+  ConfigOptions ignore = config_options_;
+
+  Status s;
+  ignore.registry->AddLibrary("failing")->AddFactory<TestCustomizable>(
+      "failing",
+      [](const std::string& /*uri*/,
+         std::unique_ptr<TestCustomizable>* /*guard */, std::string* errmsg) {
+        *errmsg = "Bad Factory";
+        return nullptr;
+      });
+
+  // If we are ignoring unknown and unsupported options, will see
+  // different errors for failing versus missing
+  ignore.ignore_unknown_options = false;
+  ignore.ignore_unsupported_options = false;
+  s = c1->ConfigureFromString(ignore, "shared.id=failing");
+  ASSERT_TRUE(s.IsInvalidArgument());
+  s = c1->ConfigureFromString(ignore, "unique.id=failing");
+  ASSERT_TRUE(s.IsInvalidArgument());
+  s = c1->ConfigureFromString(ignore, "shared.id=missing");
+  ASSERT_TRUE(s.IsNotSupported());
+  s = c1->ConfigureFromString(ignore, "unique.id=missing");
+  ASSERT_TRUE(s.IsNotSupported());
+
+  // If we are ignoring unsupported options, will see
+  // errors for failing but not missing
+  ignore.ignore_unknown_options = false;
+  ignore.ignore_unsupported_options = true;
+  s = c1->ConfigureFromString(ignore, "shared.id=failing");
+  ASSERT_TRUE(s.IsInvalidArgument());
+  s = c1->ConfigureFromString(ignore, "unique.id=failing");
+  ASSERT_TRUE(s.IsInvalidArgument());
+
+  ASSERT_OK(c1->ConfigureFromString(ignore, "shared.id=missing"));
+  ASSERT_OK(c1->ConfigureFromString(ignore, "unique.id=missing"));
+
+  // If we are ignoring unknown options, will see no errors
+  // for failing or missing
+  ignore.ignore_unknown_options = true;
+  ignore.ignore_unsupported_options = false;
+  ASSERT_OK(c1->ConfigureFromString(ignore, "shared.id=failing"));
+  ASSERT_OK(c1->ConfigureFromString(ignore, "unique.id=failing"));
+  ASSERT_OK(c1->ConfigureFromString(ignore, "shared.id=missing"));
+  ASSERT_OK(c1->ConfigureFromString(ignore, "unique.id=missing"));
+}
+
 // Tests that different IDs lead to different objects
 TEST_F(CustomizableTest, UniqueIdTest) {
   std::unique_ptr<Configurable> base(new SimpleConfigurable());

util/comparator.cc

@@ -368,9 +368,10 @@ Status Comparator::CreateFromString(const ConfigOptions& config_options,
       } else {
         return status;
       }
-    } else if (!opt_map.empty()) {
+    } else {
       Comparator* comparator = const_cast<Comparator*>(*result);
-      status = comparator->ConfigureFromMap(config_options, opt_map);
+      status =
+          Customizable::ConfigureNewObject(config_options, comparator, opt_map);
     }
   }
   return status;

utilities/object_registry_test.cc

@@ -49,30 +49,48 @@ static FactoryFunc<Env> test_reg_b = ObjectLibrary::Default()->AddFactory<Env>(
 
 TEST_F(ObjRegistryTest, Basics) {
   std::string msg;
-  std::unique_ptr<Env> env_guard;
+  std::unique_ptr<Env> guard;
+  Env* a_env = nullptr;
+
   auto registry = ObjectRegistry::NewInstance();
-  auto res = registry->NewObject<Env>("a://test", &env_guard, &msg);
+  ASSERT_NOK(registry->NewStaticObject<Env>("c://test", &a_env));
-  ASSERT_NE(res, nullptr);
+  ASSERT_NOK(registry->NewUniqueObject<Env>("c://test", &guard));
-  ASSERT_EQ(env_guard, nullptr);
+  ASSERT_EQ(a_env, nullptr);
+  ASSERT_EQ(guard, nullptr);
+  ASSERT_EQ(0, num_a);
+  ASSERT_EQ(0, num_b);
+
+  ASSERT_OK(registry->NewStaticObject<Env>("a://test", &a_env));
+  ASSERT_NE(a_env, nullptr);
   ASSERT_EQ(1, num_a);
   ASSERT_EQ(0, num_b);
 
-  res = registry->NewObject<Env>("b://test", &env_guard, &msg);
+  ASSERT_OK(registry->NewUniqueObject<Env>("b://test", &guard));
-  ASSERT_NE(res, nullptr);
+  ASSERT_NE(guard, nullptr);
-  ASSERT_NE(env_guard, nullptr);
   ASSERT_EQ(1, num_a);
   ASSERT_EQ(1, num_b);
 
-  res = registry->NewObject<Env>("c://test", &env_guard, &msg);
+  Env* b_env = nullptr;
-  ASSERT_EQ(res, nullptr);
+  ASSERT_NOK(registry->NewStaticObject<Env>("b://test", &b_env));
-  ASSERT_EQ(env_guard, nullptr);
+  ASSERT_EQ(b_env, nullptr);
   ASSERT_EQ(1, num_a);
-  ASSERT_EQ(1, num_b);
+  ASSERT_EQ(2, num_b);  // Created but rejected as not static
+
+  b_env = a_env;
+  ASSERT_NOK(registry->NewStaticObject<Env>("b://test", &b_env));
+  ASSERT_EQ(b_env, a_env);
+  ASSERT_EQ(1, num_a);
+  ASSERT_EQ(3, num_b);
+
+  b_env = guard.get();
+  ASSERT_NOK(registry->NewUniqueObject<Env>("a://test", &guard));
+  ASSERT_EQ(guard.get(), b_env);  // Unchanged
+  ASSERT_EQ(2, num_a);            // Created one but rejected it as not unique
+  ASSERT_EQ(3, num_b);
 }
 
 TEST_F(ObjRegistryTest, LocalRegistry) {
-  std::string msg;
+  Env* env = nullptr;
-  std::unique_ptr<Env> guard;
   auto registry = ObjectRegistry::NewInstance();
   std::shared_ptr<ObjectLibrary> library =
       std::make_shared<ObjectLibrary>("local");
@@ -87,14 +105,16 @@ TEST_F(ObjRegistryTest, LocalRegistry) {
       [](const std::string& /*uri*/, std::unique_ptr<Env>* /*guard */,
          std::string* /* errmsg */) { return Env::Default(); });
 
-  ASSERT_EQ(
+  ASSERT_NOK(
-      ObjectRegistry::NewInstance()->NewObject<Env>("test-local", &guard, &msg),
+      ObjectRegistry::NewInstance()->NewStaticObject<Env>("test-local", &env));
-      nullptr);
+  ASSERT_EQ(env, nullptr);
-  ASSERT_NE(
+  ASSERT_OK(
-      ObjectRegistry::NewInstance()->NewObject("test-global", &guard, &msg),
+      ObjectRegistry::NewInstance()->NewStaticObject<Env>("test-global", &env));
-      nullptr);
+  ASSERT_NE(env, nullptr);
-  ASSERT_NE(registry->NewObject<Env>("test-local", &guard, &msg), nullptr);
+  ASSERT_OK(registry->NewStaticObject<Env>("test-local", &env));
-  ASSERT_NE(registry->NewObject<Env>("test-global", &guard, &msg), nullptr);
+  ASSERT_NE(env, nullptr);
+  ASSERT_OK(registry->NewStaticObject<Env>("test-global", &env));
+  ASSERT_NE(env, nullptr);
 }
 
 TEST_F(ObjRegistryTest, CheckShared) {
@@ -172,6 +192,36 @@ TEST_F(ObjRegistryTest, CheckUnique) {
   ASSERT_EQ(unique, nullptr);
 }
 
+TEST_F(ObjRegistryTest, FailingFactory) {
+  std::shared_ptr<ObjectRegistry> registry = ObjectRegistry::NewInstance();
+  std::shared_ptr<ObjectLibrary> library =
+      std::make_shared<ObjectLibrary>("failing");
+  registry->AddLibrary(library);
+  library->AddFactory<Env>(
+      "failing", [](const std::string& /*uri*/,
+                    std::unique_ptr<Env>* /*guard */, std::string* errmsg) {
+        *errmsg = "Bad Factory";
+        return nullptr;
+      });
+  std::unique_ptr<Env> unique;
+  std::shared_ptr<Env> shared;
+  Env* pointer = nullptr;
+  Status s;
+  s = registry->NewUniqueObject<Env>("failing", &unique);
+  ASSERT_TRUE(s.IsInvalidArgument());
+  s = registry->NewSharedObject<Env>("failing", &shared);
+  ASSERT_TRUE(s.IsInvalidArgument());
+  s = registry->NewStaticObject<Env>("failing", &pointer);
+  ASSERT_TRUE(s.IsInvalidArgument());
+
+  s = registry->NewUniqueObject<Env>("missing", &unique);
+  ASSERT_TRUE(s.IsNotSupported());
+  s = registry->NewSharedObject<Env>("missing", &shared);
+  ASSERT_TRUE(s.IsNotSupported());
+  s = registry->NewStaticObject<Env>("missing", &pointer);
+  ASSERT_TRUE(s.IsNotSupported());
+}
+
 TEST_F(ObjRegistryTest, TestRegistryParents) {
   auto grand = ObjectRegistry::Default();
   auto parent = ObjectRegistry::NewInstance();  // parent with a grandparent
@@ -194,14 +244,15 @@ TEST_F(ObjRegistryTest, TestRegistryParents) {
         return guard->get();
       });
 
+  Env* env = nullptr;
   std::unique_ptr<Env> guard;
   std::string msg;
 
-  // a:://* is registered in Default, so they should all workd
+  // a:://* is registered in Default, so they should all work
-  ASSERT_NE(parent->NewObject<Env>("a://test", &guard, &msg), nullptr);
+  ASSERT_OK(parent->NewStaticObject<Env>("a://test", &env));
-  ASSERT_NE(child->NewObject<Env>("a://test", &guard, &msg), nullptr);
+  ASSERT_OK(child->NewStaticObject<Env>("a://test", &env));
-  ASSERT_NE(uncle->NewObject<Env>("a://test", &guard, &msg), nullptr);
+  ASSERT_OK(uncle->NewStaticObject<Env>("a://test", &env));
-  ASSERT_NE(cousin->NewObject<Env>("a://test", &guard, &msg), nullptr);
+  ASSERT_OK(cousin->NewStaticObject<Env>("a://test", &env));
 
   // The parent env is only registered for parent, not uncle,
   // So parent and child should return success and uncle and cousin should fail