From dafb568052fdcf711f9e352fe2c489bc191274d4 Mon Sep 17 00:00:00 2001 From: Cheng Chang Date: Mon, 10 Feb 2020 17:58:03 -0800 Subject: [PATCH] Add utility class Defer (#6382) Summary: Add a utility class `Defer` to defer the execution of a function until the Defer object goes out of scope. Used in VersionSet:: ProcessManifestWrites as an example. The inline comments for class `Defer` have more details. Pull Request resolved: https://github.com/facebook/rocksdb/pull/6382 Test Plan: `make defer_test version_set_test && ./defer_test && ./version_set_test` Differential Revision: D19797538 Pulled By: cheng-chang fbshipit-source-id: b1a9b7306e4fd4f48ec2ab55783caa561a315f0f --- CMakeLists.txt | 1 + Makefile | 4 ++++ TARGETS | 7 +++++++ db/version_set.cc | 25 ++++++++++------------ src.mk | 1 + util/defer.h | 52 ++++++++++++++++++++++++++++++++++++++++++++++ util/defer_test.cc | 39 ++++++++++++++++++++++++++++++++++ 7 files changed, 115 insertions(+), 14 deletions(-) create mode 100644 util/defer.h create mode 100644 util/defer_test.cc diff --git a/CMakeLists.txt b/CMakeLists.txt index af9f94318..aeef92e25 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1026,6 +1026,7 @@ if(WITH_TESTS) util/bloom_test.cc util/coding_test.cc util/crc32c_test.cc + util/defer_test.cc util/dynamic_bloom_test.cc util/file_reader_writer_test.cc util/filelock_test.cc diff --git a/Makefile b/Makefile index 24731770f..ce2e581cc 100644 --- a/Makefile +++ b/Makefile @@ -596,6 +596,7 @@ TESTS = \ db_secondary_test \ block_cache_tracer_test \ block_cache_trace_analyzer_test \ + defer_test \ ifeq ($(USE_FOLLY_DISTRIBUTED_MUTEX),1) TESTS += folly_synchronization_distributed_mutex_test @@ -1714,6 +1715,9 @@ block_cache_tracer_test: trace_replay/block_cache_tracer_test.o trace_replay/blo block_cache_trace_analyzer_test: tools/block_cache_analyzer/block_cache_trace_analyzer_test.o tools/block_cache_analyzer/block_cache_trace_analyzer.o $(LIBOBJECTS) $(TESTHARNESS) $(AM_LINK) +defer_test: util/defer_test.o $(LIBOBJECTS) $(TESTHARNESS) + $(AM_LINK) + #------------------------------------------------- # make install related stuff INSTALL_PATH ?= /usr/local diff --git a/TARGETS b/TARGETS index 1955eddfd..c2400ccf7 100644 --- a/TARGETS +++ b/TARGETS @@ -904,6 +904,13 @@ ROCKS_TESTS = [ [], [], ], + [ + "defer_test", + "util/defer_test.cc", + "serial", + [], + [], + ], [ "delete_scheduler_test", "file/delete_scheduler_test.cc", diff --git a/db/version_set.cc b/db/version_set.cc index 2427c51db..c0c62c129 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -50,6 +50,7 @@ #include "table/two_level_iterator.h" #include "test_util/sync_point.h" #include "util/coding.h" +#include "util/defer.h" #include "util/stop_watch.h" #include "util/string_util.h" #include "util/user_comparator_wrapper.h" @@ -3604,6 +3605,14 @@ Status VersionSet::ProcessManifestWrites( autovector versions; autovector mutable_cf_options_ptrs; std::vector> builder_guards; + Status s; + Defer defer([&s, &versions]() { + if (!s.ok()) { + for (auto v : versions) { + delete v; + } + } + }); if (first_writer.edit_list.front()->IsColumnFamilyManipulation()) { // No group commits for column family add or drop @@ -3689,12 +3698,8 @@ Status VersionSet::ProcessManifestWrites( } else if (group_start != std::numeric_limits::max()) { group_start = std::numeric_limits::max(); } - Status s = LogAndApplyHelper(last_writer->cfd, builder, e, mu); + s = LogAndApplyHelper(last_writer->cfd, builder, e, mu); if (!s.ok()) { - // free up the allocated memory - for (auto v : versions) { - delete v; - } return s; } batch_edits.push_back(e); @@ -3704,12 +3709,8 @@ Status VersionSet::ProcessManifestWrites( assert(!builder_guards.empty() && builder_guards.size() == versions.size()); auto* builder = builder_guards[i]->version_builder(); - Status s = builder->SaveTo(versions[i]->storage_info()); + s = builder->SaveTo(versions[i]->storage_info()); if (!s.ok()) { - // free up the allocated memory - for (auto v : versions) { - delete v; - } return s; } } @@ -3752,7 +3753,6 @@ Status VersionSet::ProcessManifestWrites( #endif // NDEBUG uint64_t new_manifest_file_size = 0; - Status s; assert(pending_manifest_file_number_ == 0); if (!descriptor_log_ || @@ -3959,9 +3959,6 @@ Status VersionSet::ProcessManifestWrites( ROCKS_LOG_ERROR(db_options_->info_log, "Error in committing version edit to MANIFEST: %s", version_edits.c_str()); - for (auto v : versions) { - delete v; - } // If manifest append failed for whatever reason, the file could be // corrupted. So we need to force the next version update to start a // new manifest file. diff --git a/src.mk b/src.mk index 94b8cfd35..5db947a21 100644 --- a/src.mk +++ b/src.mk @@ -419,6 +419,7 @@ MAIN_SOURCES = \ util/bloom_test.cc \ util/coding_test.cc \ util/crc32c_test.cc \ + util/defer_test.cc \ util/dynamic_bloom_test.cc \ util/filelock_test.cc \ util/log_write_bench.cc \ diff --git a/util/defer.h b/util/defer.h new file mode 100644 index 000000000..dcf02ca55 --- /dev/null +++ b/util/defer.h @@ -0,0 +1,52 @@ +// Copyright (c) 2011-present, Facebook, Inc. All rights reserved. +// This source code is licensed under both the GPLv2 (found in the +// COPYING file in the root directory) and Apache 2.0 License +// (found in the LICENSE.Apache file in the root directory). + +#pragma once + +#include + +namespace rocksdb { + +// Defers the execution of the provided function until the Defer +// object goes out of scope. +// +// Usage example: +// +// Status DeferTest() { +// Status s; +// Defer defer([&s]() { +// if (!s.ok()) { +// // do cleanups ... +// } +// }); +// // do something ... +// if (!s.ok()) return; +// // do some other things ... +// return s; +// } +// +// The above code ensures that cleanups will always happen on returning. +// +// Without the help of Defer, you can +// 1. every time when !s.ok(), do the cleanup; +// 2. instead of returning when !s.ok(), continue the work only when s.ok(), +// but sometimes, this might lead to nested blocks of "if (s.ok()) {...}". +// +// With the help of Defer, you can centralize the cleanup logic inside the +// lambda passed to Defer, and you can return immediately on failure when necessary. +class Defer final { + public: + Defer(std::function&& fn) : fn_(std::move(fn)) {} + ~Defer() { fn_(); } + + // Disallow copy. + Defer(const Defer&) = delete; + Defer& operator=(const Defer&) = delete; + + private: + std::function fn_; +}; + +} // namespace rocksdb diff --git a/util/defer_test.cc b/util/defer_test.cc new file mode 100644 index 000000000..cd7d242d1 --- /dev/null +++ b/util/defer_test.cc @@ -0,0 +1,39 @@ +// Copyright (c) 2011-present, Facebook, Inc. All rights reserved. +// This source code is licensed under both the GPLv2 (found in the +// COPYING file in the root directory) and Apache 2.0 License +// (found in the LICENSE.Apache file in the root directory). + +#include "port/port.h" +#include "port/stack_trace.h" +#include "test_util/testharness.h" +#include "util/defer.h" + +namespace rocksdb { + +class DeferTest {}; + +TEST(DeferTest, BlockScope) { + int v = 1; + { + Defer defer([&v]() { v *= 2; }); + } + ASSERT_EQ(2, v); +} + +TEST(DeferTest, FunctionScope) { + int v = 1; + auto f = [&v]() { + Defer defer([&v]() { v *= 2; }); + v = 2; + }; + f(); + ASSERT_EQ(4, v); +} + +} // namespace rocksdb + +int main(int argc, char** argv) { + rocksdb::port::InstallStackTraceHandler(); + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +}