From fe9d4951126d2c21a2672b7ebb22bda014fa82ea Mon Sep 17 00:00:00 2001 From: mrambacher Date: Fri, 11 Feb 2022 05:10:10 -0800 Subject: [PATCH] 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 | 1 + env/env.cc | 8 +- include/rocksdb/utilities/object_registry.h | 62 ++++++------ memory/memory_allocator_test.cc | 10 +- options/customizable_test.cc | 49 ++++++++++ util/comparator.cc | 5 +- utilities/object_registry_test.cc | 103 +++++++++++++++----- 7 files changed, 168 insertions(+), 70 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index fb27f95917..5e510ee2e1 100644 --- a/HISTORY.md +++ b/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. diff --git a/env/env.cc b/env/env.cc index 186429c157..dd214f655e 100644 --- a/env/env.cc +++ b/env/env.cc @@ -687,12 +687,8 @@ Status Env::CreateFromString(const ConfigOptions& config_options, } else { RegisterSystemEnvs(); #ifndef ROCKSDB_LITE - std::string errmsg; - env = config_options.registry->NewObject(id, &uniq, &errmsg); - if (!env) { - status = Status::NotSupported( - std::string("Cannot load environment[") + id + "]: ", errmsg); - } + // First, try to load the Env as a unique object. + status = config_options.registry->NewObject(id, &env, &uniq); #else status = Status::NotSupported("Cannot load environment in LITE mode", value); diff --git a/include/rocksdb/utilities/object_registry.h b/include/rocksdb/utilities/object_registry.h index 928676b2e6..9730ed1b13 100644 --- a/include/rocksdb/utilities/object_registry.h +++ b/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 - // target. Searches through the libraries to find the first library where - // there is an entry that matches target (see PatternEntry for the matching - // 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. + // Finds the factory for target and instantiates a new T. + // Returns NotSupported if no factory is found + // Returns InvalidArgument if a factory is found but the factory failed. template - T* NewObject(const std::string& target, std::unique_ptr* guard, - std::string* errmsg) { + Status NewObject(const std::string& target, T** object, + std::unique_ptr* guard) { + assert(guard != nullptr); guard->reset(); auto factory = FindFactory(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 { + return Status::InvalidArgument(errmsg, target); + } } else { - *errmsg = std::string("Could not load ") + T::Type(); - return nullptr; + 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 Status NewUniqueObject(const std::string& target, std::unique_ptr* result) { - std::string errmsg; - T* ptr = NewObject(target, result, &errmsg); - if (ptr == nullptr) { - return Status::NotSupported(errmsg, target); - } else if (*result) { + T* ptr = nullptr; + std::unique_ptr guard; + Status s = NewObject(target, &ptr, &guard); + if (!s.ok()) { + 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 Status NewSharedObject(const std::string& target, std::shared_ptr* result) { - std::string errmsg; std::unique_ptr guard; - T* ptr = NewObject(target, &guard, &errmsg); - if (ptr == nullptr) { - return Status::NotSupported(errmsg, target); + T* ptr = nullptr; + Status s = NewObject(target, &ptr, &guard); + 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 Status NewStaticObject(const std::string& target, T** result) { - std::string errmsg; std::unique_ptr guard; - T* ptr = NewObject(target, &guard, &errmsg); - if (ptr == nullptr) { - return Status::NotSupported(errmsg, target); + T* ptr = nullptr; + Status s = NewObject(target, &ptr, &guard); + if (!s.ok()) { + return s; } else if (guard.get()) { return Status::InvalidArgument(std::string("Cannot make a static ") + T::Type() + " from a guarded one ", diff --git a/memory/memory_allocator_test.cc b/memory/memory_allocator_test.cc index 90aed448b6..1e96c44ee9 100644 --- a/memory/memory_allocator_test.cc +++ b/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_OK(s); - } else if (!s.ok()) { - EXPECT_TRUE(s.IsNotSupported()); - } + EXPECT_EQ(supported_, s.ok()); } 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; } diff --git a/options/customizable_test.cc b/options/customizable_test.cc index c61a512434..92fe527f2c 100644 --- a/options/customizable_test.cc +++ b/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 registry = ObjectRegistry::NewInstance(); + std::unique_ptr c1(new SimpleConfigurable()); + ConfigOptions ignore = config_options_; + + Status s; + ignore.registry->AddLibrary("failing")->AddFactory( + "failing", + [](const std::string& /*uri*/, + std::unique_ptr* /*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 base(new SimpleConfigurable()); diff --git a/util/comparator.cc b/util/comparator.cc index a3c9a8c451..9fbc111b48 100644 --- a/util/comparator.cc +++ b/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(*result); - status = comparator->ConfigureFromMap(config_options, opt_map); + status = + Customizable::ConfigureNewObject(config_options, comparator, opt_map); } } return status; diff --git a/utilities/object_registry_test.cc b/utilities/object_registry_test.cc index 7880703f05..3ab2df6b9b 100644 --- a/utilities/object_registry_test.cc +++ b/utilities/object_registry_test.cc @@ -49,30 +49,48 @@ static FactoryFunc test_reg_b = ObjectLibrary::Default()->AddFactory( TEST_F(ObjRegistryTest, Basics) { std::string msg; - std::unique_ptr env_guard; + std::unique_ptr guard; + Env* a_env = nullptr; + auto registry = ObjectRegistry::NewInstance(); - auto res = registry->NewObject("a://test", &env_guard, &msg); - ASSERT_NE(res, nullptr); - ASSERT_EQ(env_guard, nullptr); + ASSERT_NOK(registry->NewStaticObject("c://test", &a_env)); + ASSERT_NOK(registry->NewUniqueObject("c://test", &guard)); + ASSERT_EQ(a_env, nullptr); + ASSERT_EQ(guard, nullptr); + ASSERT_EQ(0, num_a); + ASSERT_EQ(0, num_b); + + ASSERT_OK(registry->NewStaticObject("a://test", &a_env)); + ASSERT_NE(a_env, nullptr); ASSERT_EQ(1, num_a); ASSERT_EQ(0, num_b); - res = registry->NewObject("b://test", &env_guard, &msg); - ASSERT_NE(res, nullptr); - ASSERT_NE(env_guard, nullptr); + ASSERT_OK(registry->NewUniqueObject("b://test", &guard)); + ASSERT_NE(guard, nullptr); ASSERT_EQ(1, num_a); ASSERT_EQ(1, num_b); - res = registry->NewObject("c://test", &env_guard, &msg); - ASSERT_EQ(res, nullptr); - ASSERT_EQ(env_guard, nullptr); + Env* b_env = nullptr; + ASSERT_NOK(registry->NewStaticObject("b://test", &b_env)); + 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("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("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; - std::unique_ptr guard; + Env* env = nullptr; auto registry = ObjectRegistry::NewInstance(); std::shared_ptr library = std::make_shared("local"); @@ -87,14 +105,16 @@ TEST_F(ObjRegistryTest, LocalRegistry) { [](const std::string& /*uri*/, std::unique_ptr* /*guard */, std::string* /* errmsg */) { return Env::Default(); }); - ASSERT_EQ( - ObjectRegistry::NewInstance()->NewObject("test-local", &guard, &msg), - nullptr); - ASSERT_NE( - ObjectRegistry::NewInstance()->NewObject("test-global", &guard, &msg), - nullptr); - ASSERT_NE(registry->NewObject("test-local", &guard, &msg), nullptr); - ASSERT_NE(registry->NewObject("test-global", &guard, &msg), nullptr); + ASSERT_NOK( + ObjectRegistry::NewInstance()->NewStaticObject("test-local", &env)); + ASSERT_EQ(env, nullptr); + ASSERT_OK( + ObjectRegistry::NewInstance()->NewStaticObject("test-global", &env)); + ASSERT_NE(env, nullptr); + ASSERT_OK(registry->NewStaticObject("test-local", &env)); + ASSERT_NE(env, nullptr); + ASSERT_OK(registry->NewStaticObject("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 registry = ObjectRegistry::NewInstance(); + std::shared_ptr library = + std::make_shared("failing"); + registry->AddLibrary(library); + library->AddFactory( + "failing", [](const std::string& /*uri*/, + std::unique_ptr* /*guard */, std::string* errmsg) { + *errmsg = "Bad Factory"; + return nullptr; + }); + std::unique_ptr unique; + std::shared_ptr shared; + Env* pointer = nullptr; + Status s; + s = registry->NewUniqueObject("failing", &unique); + ASSERT_TRUE(s.IsInvalidArgument()); + s = registry->NewSharedObject("failing", &shared); + ASSERT_TRUE(s.IsInvalidArgument()); + s = registry->NewStaticObject("failing", &pointer); + ASSERT_TRUE(s.IsInvalidArgument()); + + s = registry->NewUniqueObject("missing", &unique); + ASSERT_TRUE(s.IsNotSupported()); + s = registry->NewSharedObject("missing", &shared); + ASSERT_TRUE(s.IsNotSupported()); + s = registry->NewStaticObject("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 guard; std::string msg; - // a:://* is registered in Default, so they should all workd - ASSERT_NE(parent->NewObject("a://test", &guard, &msg), nullptr); - ASSERT_NE(child->NewObject("a://test", &guard, &msg), nullptr); - ASSERT_NE(uncle->NewObject("a://test", &guard, &msg), nullptr); - ASSERT_NE(cousin->NewObject("a://test", &guard, &msg), nullptr); + // a:://* is registered in Default, so they should all work + ASSERT_OK(parent->NewStaticObject("a://test", &env)); + ASSERT_OK(child->NewStaticObject("a://test", &env)); + ASSERT_OK(uncle->NewStaticObject("a://test", &env)); + ASSERT_OK(cousin->NewStaticObject("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