Change the File System File Wrappers to std::unique_ptr (#8618)

Summary:
This allows the wrapper classes to own the wrapped object and eliminates confusion as to ownership.  Previously, many classes implemented their own ownership solutions.  Fixes https://github.com/facebook/rocksdb/issues/8606

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

Reviewed By: pdillinger

Differential Revision: D30136064

Pulled By: mrambacher

fbshipit-source-id: d0bf471df8818dbb1770a86335fe98f761cca193

diff --git a/db/db_basic_test.cc b/db/db_basic_test.cc
index a9e6922f9e54ed1043e71bb25a2d747ae95f4e4e..9ebc0d7084480a5549340e52a76af137d17dfe5b 100644 (file)
--- a/db/db_basic_test.cc
+++ b/db/db_basic_test.cc
@@ -3235,13 +3235,11 @@ INSTANTIATE_TEST_CASE_P(ParallelIO, DBBasicTestWithParallelIO,
 // Forward declaration
 class DeadlineFS;
 
-class DeadlineRandomAccessFile : public FSRandomAccessFileWrapper {
+class DeadlineRandomAccessFile : public FSRandomAccessFileOwnerWrapper {
  public:
   DeadlineRandomAccessFile(DeadlineFS& fs,
                            std::unique_ptr<FSRandomAccessFile>& file)
-      : FSRandomAccessFileWrapper(file.get()),
-        fs_(fs),
-        file_(std::move(file)) {}
+      : FSRandomAccessFileOwnerWrapper(std::move(file)), fs_(fs) {}
 
   IOStatus Read(uint64_t offset, size_t len, const IOOptions& opts,
                 Slice* result, char* scratch,

diff --git a/env/file_system_tracer.h b/env/file_system_tracer.h
index da87797d30b6da32a71ca0cd1e1404a762a609b0..0c0b918c81b63406a6760fdadcc1fca566395b1e 100644 (file)
--- a/env/file_system_tracer.h
+++ b/env/file_system_tracer.h
@@ -131,12 +131,12 @@ class FileSystemPtr {
 // for tracing. It overrides methods we are interested in tracing and extends
 // FSSequentialFileWrapper, which forwards all methods that are not explicitly
 // overridden.
-class FSSequentialFileTracingWrapper : public FSSequentialFileWrapper {
+class FSSequentialFileTracingWrapper : public FSSequentialFileOwnerWrapper {
  public:
-  FSSequentialFileTracingWrapper(FSSequentialFile* t,
+  FSSequentialFileTracingWrapper(std::unique_ptr<FSSequentialFile>&& t,
                                  std::shared_ptr<IOTracer> io_tracer,
                                  const std::string& file_name)
-      : FSSequentialFileWrapper(t),
+      : FSSequentialFileOwnerWrapper(std::move(t)),
         io_tracer_(io_tracer),
         clock_(SystemClock::Default().get()),
         file_name_(file_name) {}
@@ -169,9 +169,8 @@ class FSSequentialFilePtr {
   FSSequentialFilePtr(std::unique_ptr<FSSequentialFile>&& fs,
                       const std::shared_ptr<IOTracer>& io_tracer,
                       const std::string& file_name)
-      : fs_(std::move(fs)),
-        io_tracer_(io_tracer),
-        fs_tracer_(fs_.get(), io_tracer_,
+      : io_tracer_(io_tracer),
+        fs_tracer_(std::move(fs), io_tracer_,
                    file_name.substr(file_name.find_last_of("/\\") +
                                     1) /* pass file name */) {}
 
@@ -179,7 +178,7 @@ class FSSequentialFilePtr {
     if (io_tracer_ && io_tracer_->is_tracing_enabled()) {
       return const_cast<FSSequentialFileTracingWrapper*>(&fs_tracer_);
     } else {
-      return fs_.get();
+      return fs_tracer_.target();
     }
   }
 
@@ -187,12 +186,11 @@ class FSSequentialFilePtr {
     if (io_tracer_ && io_tracer_->is_tracing_enabled()) {
       return const_cast<FSSequentialFileTracingWrapper*>(&fs_tracer_);
     } else {
-      return fs_.get();
+      return fs_tracer_.target();
     }
   }
 
  private:
-  std::unique_ptr<FSSequentialFile> fs_;
   std::shared_ptr<IOTracer> io_tracer_;
   FSSequentialFileTracingWrapper fs_tracer_;
 };
@@ -203,12 +201,12 @@ class FSSequentialFilePtr {
 // binary format for tracing. It overrides methods we are interested in tracing
 // and extends FSRandomAccessFileWrapper, which forwards all methods that are
 // not explicitly overridden.
-class FSRandomAccessFileTracingWrapper : public FSRandomAccessFileWrapper {
+class FSRandomAccessFileTracingWrapper : public FSRandomAccessFileOwnerWrapper {
  public:
-  FSRandomAccessFileTracingWrapper(FSRandomAccessFile* t,
+  FSRandomAccessFileTracingWrapper(std::unique_ptr<FSRandomAccessFile>&& t,
                                    std::shared_ptr<IOTracer> io_tracer,
                                    const std::string& file_name)
-      : FSRandomAccessFileWrapper(t),
+      : FSRandomAccessFileOwnerWrapper(std::move(t)),
         io_tracer_(io_tracer),
         clock_(SystemClock::Default().get()),
         file_name_(file_name) {}
@@ -244,9 +242,8 @@ class FSRandomAccessFilePtr {
   FSRandomAccessFilePtr(std::unique_ptr<FSRandomAccessFile>&& fs,
                         const std::shared_ptr<IOTracer>& io_tracer,
                         const std::string& file_name)
-      : fs_(std::move(fs)),
-        io_tracer_(io_tracer),
-        fs_tracer_(fs_.get(), io_tracer_,
+      : io_tracer_(io_tracer),
+        fs_tracer_(std::move(fs), io_tracer_,
                    file_name.substr(file_name.find_last_of("/\\") +
                                     1) /* pass file name */) {}
 
@@ -254,7 +251,7 @@ class FSRandomAccessFilePtr {
     if (io_tracer_ && io_tracer_->is_tracing_enabled()) {
       return const_cast<FSRandomAccessFileTracingWrapper*>(&fs_tracer_);
     } else {
-      return fs_.get();
+      return fs_tracer_.target();
     }
   }
 
@@ -262,12 +259,11 @@ class FSRandomAccessFilePtr {
     if (io_tracer_ && io_tracer_->is_tracing_enabled()) {
       return const_cast<FSRandomAccessFileTracingWrapper*>(&fs_tracer_);
     } else {
-      return fs_.get();
+      return fs_tracer_.target();
     }
   }
 
  private:
-  std::unique_ptr<FSRandomAccessFile> fs_;
   std::shared_ptr<IOTracer> io_tracer_;
   FSRandomAccessFileTracingWrapper fs_tracer_;
 };
@@ -278,12 +274,12 @@ class FSRandomAccessFilePtr {
 // for tracing. It overrides methods we are interested in tracing and extends
 // FSWritableFileWrapper, which forwards all methods that are not explicitly
 // overridden.
-class FSWritableFileTracingWrapper : public FSWritableFileWrapper {
+class FSWritableFileTracingWrapper : public FSWritableFileOwnerWrapper {
  public:
-  FSWritableFileTracingWrapper(FSWritableFile* t,
+  FSWritableFileTracingWrapper(std::unique_ptr<FSWritableFile>&& t,
                                std::shared_ptr<IOTracer> io_tracer,
                                const std::string& file_name)
-      : FSWritableFileWrapper(t),
+      : FSWritableFileOwnerWrapper(std::move(t)),
         io_tracer_(io_tracer),
         clock_(SystemClock::Default().get()),
         file_name_(file_name) {}
@@ -334,9 +330,9 @@ class FSWritableFilePtr {
   FSWritableFilePtr(std::unique_ptr<FSWritableFile>&& fs,
                     const std::shared_ptr<IOTracer>& io_tracer,
                     const std::string& file_name)
-      : fs_(std::move(fs)), io_tracer_(io_tracer) {
+      : io_tracer_(io_tracer) {
     fs_tracer_.reset(new FSWritableFileTracingWrapper(
-        fs_.get(), io_tracer_,
+        std::move(fs), io_tracer_,
         file_name.substr(file_name.find_last_of("/\\") +
                          1) /* pass file name */));
   }
@@ -345,26 +341,26 @@ class FSWritableFilePtr {
     if (io_tracer_ && io_tracer_->is_tracing_enabled()) {
       return fs_tracer_.get();
     } else {
-      return fs_.get();
+      return fs_tracer_->target();
     }
   }
 
   FSWritableFile* get() const {
     if (io_tracer_ && io_tracer_->is_tracing_enabled()) {
       return fs_tracer_.get();
+    } else if (fs_tracer_) {
+      return fs_tracer_->target();
     } else {
-      return fs_.get();
+      return nullptr;
     }
   }
 
   void reset() {
-    fs_.reset();
     fs_tracer_.reset();
     io_tracer_ = nullptr;
   }
 
  private:
-  std::unique_ptr<FSWritableFile> fs_;
   std::shared_ptr<IOTracer> io_tracer_;
   std::unique_ptr<FSWritableFileTracingWrapper> fs_tracer_;
 };
@@ -375,12 +371,12 @@ class FSWritableFilePtr {
 // for tracing. It overrides methods we are interested in tracing and extends
 // FSRandomRWFileWrapper, which forwards all methods that are not explicitly
 // overridden.
-class FSRandomRWFileTracingWrapper : public FSRandomRWFileWrapper {
+class FSRandomRWFileTracingWrapper : public FSRandomRWFileOwnerWrapper {
  public:
-  FSRandomRWFileTracingWrapper(FSRandomRWFile* t,
+  FSRandomRWFileTracingWrapper(std::unique_ptr<FSRandomRWFile>&& t,
                                std::shared_ptr<IOTracer> io_tracer,
                                const std::string& file_name)
-      : FSRandomRWFileWrapper(t),
+      : FSRandomRWFileOwnerWrapper(std::move(t)),
         io_tracer_(io_tracer),
         clock_(SystemClock::Default().get()),
         file_name_(file_name) {}
@@ -419,9 +415,8 @@ class FSRandomRWFilePtr {
   FSRandomRWFilePtr(std::unique_ptr<FSRandomRWFile>&& fs,
                     std::shared_ptr<IOTracer> io_tracer,
                     const std::string& file_name)
-      : fs_(std::move(fs)),
-        io_tracer_(io_tracer),
-        fs_tracer_(fs_.get(), io_tracer_,
+      : io_tracer_(io_tracer),
+        fs_tracer_(std::move(fs), io_tracer_,
                    file_name.substr(file_name.find_last_of("/\\") +
                                     1) /* pass file name */) {}
 
@@ -429,7 +424,7 @@ class FSRandomRWFilePtr {
     if (io_tracer_ && io_tracer_->is_tracing_enabled()) {
       return const_cast<FSRandomRWFileTracingWrapper*>(&fs_tracer_);
     } else {
-      return fs_.get();
+      return fs_tracer_.target();
     }
   }
 
@@ -437,12 +432,11 @@ class FSRandomRWFilePtr {
     if (io_tracer_ && io_tracer_->is_tracing_enabled()) {
       return const_cast<FSRandomRWFileTracingWrapper*>(&fs_tracer_);
     } else {
-      return fs_.get();
+      return fs_tracer_.target();
     }
   }
 
  private:
-  std::unique_ptr<FSRandomRWFile> fs_;
   std::shared_ptr<IOTracer> io_tracer_;
   FSRandomRWFileTracingWrapper fs_tracer_;
 };

diff --git a/file/prefetch_test.cc b/file/prefetch_test.cc
index ce4ac37aeb8f20c2cc5a29e39125fe23b1e1a236..739e0e37bb5db26b250b677e4a1e7157321f1f41 100644 (file)
--- a/file/prefetch_test.cc
+++ b/file/prefetch_test.cc
@@ -10,12 +10,11 @@ namespace ROCKSDB_NAMESPACE {
 
 class MockFS;
 
-class MockRandomAccessFile : public FSRandomAccessFileWrapper {
+class MockRandomAccessFile : public FSRandomAccessFileOwnerWrapper {
  public:
   MockRandomAccessFile(std::unique_ptr<FSRandomAccessFile>& file,
                        bool support_prefetch, std::atomic_int& prefetch_count)
-      : FSRandomAccessFileWrapper(file.get()),
-        file_(std::move(file)),
+      : FSRandomAccessFileOwnerWrapper(std::move(file)),
         support_prefetch_(support_prefetch),
         prefetch_count_(prefetch_count) {}
 
@@ -30,7 +29,6 @@ class MockRandomAccessFile : public FSRandomAccessFileWrapper {
   }
 
  private:
-  std::unique_ptr<FSRandomAccessFile> file_;
   const bool support_prefetch_;
   std::atomic_int& prefetch_count_;
 };

diff --git a/include/rocksdb/file_system.h b/include/rocksdb/file_system.h
index 025908e4f976acbc564ccc9b44533aebbd6ceec5..2c73f720ad93fa2d6795c2722976555b973ac7ff 100644 (file)
--- a/include/rocksdb/file_system.h
+++ b/include/rocksdb/file_system.h
@@ -1340,6 +1340,8 @@ class FileSystemWrapper : public FileSystem {
 
 class FSSequentialFileWrapper : public FSSequentialFile {
  public:
+  // Creates a FileWrapper around the input File object and without
+  // taking ownership of the object
   explicit FSSequentialFileWrapper(FSSequentialFile* t) : target_(t) {}
 
   FSSequentialFile* target() const { return target_; }
@@ -1366,8 +1368,21 @@ class FSSequentialFileWrapper : public FSSequentialFile {
   FSSequentialFile* target_;
 };
 
+class FSSequentialFileOwnerWrapper : public FSSequentialFileWrapper {
+ public:
+  // Creates a FileWrapper around the input File object and takes
+  // ownership of the object
+  explicit FSSequentialFileOwnerWrapper(std::unique_ptr<FSSequentialFile>&& t)
+      : FSSequentialFileWrapper(t.get()), guard_(std::move(t)) {}
+
+ private:
+  std::unique_ptr<FSSequentialFile> guard_;
+};
+
 class FSRandomAccessFileWrapper : public FSRandomAccessFile {
  public:
+  // Creates a FileWrapper around the input File object and without
+  // taking ownership of the object
   explicit FSRandomAccessFileWrapper(FSRandomAccessFile* t) : target_(t) {}
 
   FSRandomAccessFile* target() const { return target_; }
@@ -1398,11 +1413,26 @@ class FSRandomAccessFileWrapper : public FSRandomAccessFile {
   }
 
  private:
+  std::unique_ptr<FSRandomAccessFile> guard_;
   FSRandomAccessFile* target_;
 };
 
+class FSRandomAccessFileOwnerWrapper : public FSRandomAccessFileWrapper {
+ public:
+  // Creates a FileWrapper around the input File object and takes
+  // ownership of the object
+  explicit FSRandomAccessFileOwnerWrapper(
+      std::unique_ptr<FSRandomAccessFile>&& t)
+      : FSRandomAccessFileWrapper(t.get()), guard_(std::move(t)) {}
+
+ private:
+  std::unique_ptr<FSRandomAccessFile> guard_;
+};
+
 class FSWritableFileWrapper : public FSWritableFile {
  public:
+  // Creates a FileWrapper around the input File object and without
+  // taking ownership of the object
   explicit FSWritableFileWrapper(FSWritableFile* t) : target_(t) {}
 
   FSWritableFile* target() const { return target_; }
@@ -1500,8 +1530,21 @@ class FSWritableFileWrapper : public FSWritableFile {
   FSWritableFile* target_;
 };
 
+class FSWritableFileOwnerWrapper : public FSWritableFileWrapper {
+ public:
+  // Creates a FileWrapper around the input File object and takes
+  // ownership of the object
+  explicit FSWritableFileOwnerWrapper(std::unique_ptr<FSWritableFile>&& t)
+      : FSWritableFileWrapper(t.get()), guard_(std::move(t)) {}
+
+ private:
+  std::unique_ptr<FSWritableFile> guard_;
+};
+
 class FSRandomRWFileWrapper : public FSRandomRWFile {
  public:
+  // Creates a FileWrapper around the input File object and without
+  // taking ownership of the object
   explicit FSRandomRWFileWrapper(FSRandomRWFile* t) : target_(t) {}
 
   FSRandomRWFile* target() const { return target_; }
@@ -1536,8 +1579,28 @@ class FSRandomRWFileWrapper : public FSRandomRWFile {
   FSRandomRWFile* target_;
 };
 
+class FSRandomRWFileOwnerWrapper : public FSRandomRWFileWrapper {
+ public:
+  // Creates a FileWrapper around the input File object and takes
+  // ownership of the object
+  explicit FSRandomRWFileOwnerWrapper(std::unique_ptr<FSRandomRWFile>&& t)
+      : FSRandomRWFileWrapper(t.get()), guard_(std::move(t)) {}
+
+ private:
+  std::unique_ptr<FSRandomRWFile> guard_;
+};
+
 class FSDirectoryWrapper : public FSDirectory {
  public:
+  // Creates a FileWrapper around the input File object and takes
+  // ownership of the object
+  explicit FSDirectoryWrapper(std::unique_ptr<FSDirectory>&& t)
+      : guard_(std::move(t)) {
+    target_ = guard_.get();
+  }
+
+  // Creates a FileWrapper around the input File object and without
+  // taking ownership of the object
   explicit FSDirectoryWrapper(FSDirectory* t) : target_(t) {}
 
   IOStatus Fsync(const IOOptions& options, IODebugContext* dbg) override {
@@ -1548,6 +1611,7 @@ class FSDirectoryWrapper : public FSDirectory {
   }
 
  private:
+  std::unique_ptr<FSDirectory> guard_;
   FSDirectory* target_;
 };
 

diff --git a/tools/simulated_hybrid_file_system.cc b/tools/simulated_hybrid_file_system.cc
index 59b4654be5ea7c516eb73dae407ecb6a269cf541..7cb992152225b8fddb08ef6593fe37fefb65381b 100644 (file)
--- a/tools/simulated_hybrid_file_system.cc
+++ b/tools/simulated_hybrid_file_system.cc
@@ -81,7 +81,7 @@ IOStatus SimulatedHybridFileSystem::NewRandomAccessFile(
   }
   IOStatus s = target()->NewRandomAccessFile(fname, file_opts, result, dbg);
   result->reset(
-      new SimulatedHybridRaf(result->release(), rate_limiter_, temperature));
+      new SimulatedHybridRaf(std::move(*result), rate_limiter_, temperature));
   return s;
 }
 

diff --git a/tools/simulated_hybrid_file_system.h b/tools/simulated_hybrid_file_system.h
index a3d16c4e71cb98451c7d926303af5927f63cea83..7b101b57ab565932c43ed2bc21e9b5abfa8f01e9 100644 (file)
--- a/tools/simulated_hybrid_file_system.h
+++ b/tools/simulated_hybrid_file_system.h
@@ -59,12 +59,12 @@ class SimulatedHybridFileSystem : public FileSystemWrapper {
 
 // Simulated random access file that can control IOPs and latency to simulate
 // specific storage media
-class SimulatedHybridRaf : public FSRandomAccessFileWrapper {
+class SimulatedHybridRaf : public FSRandomAccessFileOwnerWrapper {
  public:
-  SimulatedHybridRaf(FSRandomAccessFile* t,
+  SimulatedHybridRaf(std::unique_ptr<FSRandomAccessFile>&& t,
                      std::shared_ptr<RateLimiter> rate_limiter,
                      Temperature temperature)
-      : FSRandomAccessFileWrapper(t),
+      : FSRandomAccessFileOwnerWrapper(std::move(t)),
         rate_limiter_(rate_limiter),
         temperature_(temperature) {}
 

diff --git a/utilities/fault_injection_fs.cc b/utilities/fault_injection_fs.cc
index 68a0397092ce6c87a5e98cda0dc2dbd7c2a2c15b..d0d2260c1cf10e6d2ff8eabf801f474508ffcf9d 100644 (file)
--- a/utilities/fault_injection_fs.cc
+++ b/utilities/fault_injection_fs.cc
@@ -544,7 +544,7 @@ IOStatus FaultInjectionTestFS::NewSequentialFile(
   }
   IOStatus io_s = target()->NewSequentialFile(fname, file_opts, result, dbg);
   if (io_s.ok()) {
-    result->reset(new TestFSSequentialFile(result->release(), this));
+    result->reset(new TestFSSequentialFile(std::move(*result), this));
   }
   return io_s;
 }

diff --git a/utilities/fault_injection_fs.h b/utilities/fault_injection_fs.h
index 62c938ca0f9988fa9fe408ce1faef1baefb9096b..3238f1e2351d918efe13a84171befd727b91ba67 100644 (file)
--- a/utilities/fault_injection_fs.h
+++ b/utilities/fault_injection_fs.h
@@ -150,10 +150,11 @@ class TestFSRandomAccessFile : public FSRandomAccessFile {
   FaultInjectionTestFS* fs_;
 };
 
-class TestFSSequentialFile : public FSSequentialFileWrapper {
+class TestFSSequentialFile : public FSSequentialFileOwnerWrapper {
  public:
-  explicit TestFSSequentialFile(FSSequentialFile* f, FaultInjectionTestFS* fs)
-      : FSSequentialFileWrapper(f), target_guard_(f), fs_(fs) {}
+  explicit TestFSSequentialFile(std::unique_ptr<FSSequentialFile>&& f,
+                                FaultInjectionTestFS* fs)
+      : FSSequentialFileOwnerWrapper(std::move(f)), fs_(fs) {}
   IOStatus Read(size_t n, const IOOptions& options, Slice* result,
                 char* scratch, IODebugContext* dbg) override;
   IOStatus PositionedRead(uint64_t offset, size_t n, const IOOptions& options,
@@ -161,7 +162,6 @@ class TestFSSequentialFile : public FSSequentialFileWrapper {
                           IODebugContext* dbg) override;
 
  private:
-  std::unique_ptr<FSSequentialFile> target_guard_;
   FaultInjectionTestFS* fs_;
 };