Clear old fs::path when moving a TemporaryDirectory

Summary:
If a TemporaryDirectory with scope DELETE_ON_DESTRUCTION gets moved
and then goes out of scope, the directory will be deleted while the moved
object still holds that path; we just clear the path from the old object to
prevent this

Reviewed By: yfeldblum

Differential Revision: D3331079

fbshipit-source-id: 9c99413661e2436ccec937d3fa652da810133b34

diff --git a/folly/experimental/TestUtil.cpp b/folly/experimental/TestUtil.cpp
index e1f2010bc21d77d10de8769c812613ac7451cde6..362ed1580d6135c5ad8a0a1bf0d3e7f4ca6fe2d2 100644 (file)
--- a/folly/experimental/TestUtil.cpp
+++ b/folly/experimental/TestUtil.cpp
@@ -95,18 +95,20 @@ TemporaryFile::~TemporaryFile() {
   }
 }
 
-TemporaryDirectory::TemporaryDirectory(StringPiece namePrefix,
-                                       fs::path dir,
-                                       Scope scope)
-  : scope_(scope),
-    path_(generateUniquePath(std::move(dir), namePrefix)) {
-  fs::create_directory(path_);
+TemporaryDirectory::TemporaryDirectory(
+    StringPiece namePrefix,
+    fs::path dir,
+    Scope scope)
+    : scope_(scope),
+      path_(std::make_unique<fs::path>(
+          generateUniquePath(std::move(dir), namePrefix))) {
+  fs::create_directory(path());
 }
 
 TemporaryDirectory::~TemporaryDirectory() {
-  if (scope_ == Scope::DELETE_ON_DESTRUCTION) {
+  if (scope_ == Scope::DELETE_ON_DESTRUCTION && path_ != nullptr) {
     boost::system::error_code ec;
-    fs::remove_all(path_, ec);
+    fs::remove_all(path(), ec);
     if (ec) {
       LOG(WARNING) << "recursive delete on destruction failed: " << ec;
     }

diff --git a/folly/experimental/TestUtil.h b/folly/experimental/TestUtil.h
index f5fb1638dc2b994c2ef32e5cc0a67e81d55b6515..5b4112febb6440d555a04e644f32e9c1e1336bcf 100644 (file)
--- a/folly/experimental/TestUtil.h
+++ b/folly/experimental/TestUtil.h
@@ -89,11 +89,13 @@ class TemporaryDirectory {
   TemporaryDirectory(TemporaryDirectory&&) = default;
   TemporaryDirectory& operator=(TemporaryDirectory&&) = default;
 
-  const fs::path& path() const { return path_; }
+  const fs::path& path() const {
+    return *path_;
+  }
 
  private:
   Scope scope_;
-  fs::path path_;
+  std::unique_ptr<fs::path> path_;
 };
 
 /**

diff --git a/folly/experimental/test/TestUtilTest.cpp b/folly/experimental/test/TestUtilTest.cpp
index 8e69da9c1bafe5ff75db452005c0f321105da74b..d8480b1d731dc1f0e181bc1e8b59c9415478a0fb 100644 (file)
--- a/folly/experimental/test/TestUtilTest.cpp
+++ b/folly/experimental/test/TestUtilTest.cpp
@@ -101,6 +101,30 @@ TEST(TemporaryDirectory, DeleteOnDestruction) {
   testTemporaryDirectory(TemporaryDirectory::Scope::DELETE_ON_DESTRUCTION);
 }
 
+void expectTempdirExists(const TemporaryDirectory& d) {
+  EXPECT_FALSE(d.path().empty());
+  EXPECT_TRUE(fs::exists(d.path()));
+  EXPECT_TRUE(fs::is_directory(d.path()));
+}
+
+TEST(TemporaryDirectory, SafelyMove) {
+  std::unique_ptr<TemporaryDirectory> dir;
+  TemporaryDirectory dir2;
+  {
+    auto scope = TemporaryDirectory::Scope::DELETE_ON_DESTRUCTION;
+    TemporaryDirectory d("", "", scope);
+    TemporaryDirectory d2("", "", scope);
+    expectTempdirExists(d);
+    expectTempdirExists(d2);
+
+    dir = std::make_unique<TemporaryDirectory>(std::move(d));
+    dir2 = std::move(d2);
+  }
+
+  expectTempdirExists(*dir);
+  expectTempdirExists(dir2);
+}
+
 TEST(ChangeToTempDir, ChangeDir) {
   auto pwd1 = fs::current_path();
   {