Fix rename() sometimes failing if another process uses openFileForRead()

On Windows, fs::rename() could fail is another process was reading the
file at the same time using fs::openFileForRead().  In most cases the user
wouldn't notice as fs::rename() will continue to retry for 2000ms.  Typically
this is enough for the read to complete and a retry to succeed, but if the
disk is being it too hard then the response time might be longer than the
retry time and the rename would fail with a permission error.

Add FILE_SHARE_DELETE to the sharing flags for CreateFileW() in
fs::openFileForRead() and try ReplaceFileW() prior to MoveFileExW()
in fs::rename().

Based on an initial patch by Edd Dawson!

Differential Revision: http://reviews.llvm.org/D13647

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@250046 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/Support/Windows/Path.inc b/lib/Support/Windows/Path.inc
index 1ee45c6f362779d934181b88d22e03ff616eb2c2..2c111321c60ce89859cb8b6451376a7b3cd73ab6 100644 (file)
--- a/lib/Support/Windows/Path.inc
+++ b/lib/Support/Windows/Path.inc
@@ -253,17 +253,34 @@ std::error_code rename(const Twine &from, const Twine &to) {
     return ec;
 
   std::error_code ec = std::error_code();
+
+  // Retry while we see ERROR_ACCESS_DENIED.
+  // System scanners (eg. indexer) might open the source file when it is written
+  // and closed.
+
   for (int i = 0; i < 2000; i++) {
+    // Try ReplaceFile first, as it is able to associate a new data stream with
+    // the destination even if the destination file is currently open.
+    if (::ReplaceFileW(wide_to.begin(), wide_from.begin(), NULL, 0, NULL, NULL))
+      return std::error_code();
+
+    // We get ERROR_FILE_NOT_FOUND if the destination file is missing.
+    // MoveFileEx can handle this case.
+    DWORD ReplaceError = ::GetLastError();
+    ec = mapWindowsError(ReplaceError);
+    if (ReplaceError != ERROR_ACCESS_DENIED &&
+        ReplaceError != ERROR_FILE_NOT_FOUND &&
+        ReplaceError != ERROR_SHARING_VIOLATION)
+      break;
+
     if (::MoveFileExW(wide_from.begin(), wide_to.begin(),
                       MOVEFILE_COPY_ALLOWED | MOVEFILE_REPLACE_EXISTING))
       return std::error_code();
-    DWORD LastError = ::GetLastError();
-    ec = mapWindowsError(LastError);
-    if (LastError != ERROR_ACCESS_DENIED)
-      break;
-    // Retry MoveFile() at ACCESS_DENIED.
-    // System scanners (eg. indexer) might open the source file when
-    // It is written and closed.
+
+    DWORD MoveError = ::GetLastError();
+    ec = mapWindowsError(MoveError);
+    if (MoveError != ERROR_ACCESS_DENIED) break;
+
     ::Sleep(1);
   }
 
@@ -649,9 +666,10 @@ std::error_code openFileForRead(const Twine &Name, int &ResultFD) {
   if (std::error_code EC = widenPath(Name, PathUTF16))
     return EC;
 
-  HANDLE H = ::CreateFileW(PathUTF16.begin(), GENERIC_READ,
-                           FILE_SHARE_READ | FILE_SHARE_WRITE, NULL,
-                           OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
+  HANDLE H =
+      ::CreateFileW(PathUTF16.begin(), GENERIC_READ,
+                    FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
+                    NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
   if (H == INVALID_HANDLE_VALUE) {
     DWORD LastError = ::GetLastError();
     std::error_code EC = mapWindowsError(LastError);

diff --git a/unittests/Support/CMakeLists.txt b/unittests/Support/CMakeLists.txt
index 9c2aab43a6a3c2fd18e1dd1e741cbeb381a2becf..fd8324c836da92699f67c4a96ab468c3a948606e 100644 (file)
--- a/unittests/Support/CMakeLists.txt
+++ b/unittests/Support/CMakeLists.txt
@@ -32,6 +32,7 @@ add_llvm_unittest(SupportTests
   ProcessTest.cpp
   ProgramTest.cpp
   RegexTest.cpp
+  ReplaceFileTest.cpp
   ScaledNumberTest.cpp
   SourceMgrTest.cpp
   SpecialCaseListTest.cpp

diff --git a/unittests/Support/ReplaceFileTest.cpp b/unittests/Support/ReplaceFileTest.cpp
new file mode 100644 (file)
index 0000000..8b16daf
--- /dev/null
+++ b/unittests/Support/ReplaceFileTest.cpp
@@ -0,0 +1,113 @@
+//===- llvm/unittest/Support/ReplaceFileTest.cpp - unit tests -------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/Process.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace llvm::sys;
+
+#define ASSERT_NO_ERROR(x)                                                 \
+  do {                                                                     \
+    if (std::error_code ASSERT_NO_ERROR_ec = x) {                          \
+      errs() << #x ": did not return errc::success.\n"                     \
+             << "error number: " << ASSERT_NO_ERROR_ec.value() << "\n"     \
+             << "error message: " << ASSERT_NO_ERROR_ec.message() << "\n"; \
+    }                                                                      \
+  } while (false)
+
+namespace {
+std::error_code CreateFileWithContent(const SmallString<128> &FilePath,
+                                      const StringRef &content) {
+  int FD = 0;
+  if (std::error_code ec = fs::openFileForWrite(FilePath, FD, fs::F_None))
+    return ec;
+
+  const bool ShouldClose = true;
+  raw_fd_ostream OS(FD, ShouldClose);
+  OS << content;
+
+  return std::error_code();
+}
+
+class ScopedFD {
+  int FD;
+
+  ScopedFD(const ScopedFD &) = delete;
+  ScopedFD &operator=(const ScopedFD &) = delete;
+
+ public:
+  explicit ScopedFD(int Descriptor) : FD(Descriptor) {}
+  ~ScopedFD() { Process::SafelyCloseFileDescriptor(FD); }
+};
+
+TEST(rename, FileOpenedForReadingCanBeReplaced) {
+  // Create unique temporary directory for this test.
+  SmallString<128> TestDirectory;
+  ASSERT_NO_ERROR(fs::createUniqueDirectory(
+      "FileOpenedForReadingCanBeReplaced-test", TestDirectory));
+
+  // Add a couple of files to the test directory.
+  SmallString<128> SourceFileName(TestDirectory);
+  path::append(SourceFileName, "source");
+
+  SmallString<128> TargetFileName(TestDirectory);
+  path::append(TargetFileName, "target");
+
+  ASSERT_NO_ERROR(CreateFileWithContent(SourceFileName, "!!source!!"));
+  ASSERT_NO_ERROR(CreateFileWithContent(TargetFileName, "!!target!!"));
+
+  {
+    // Open the target file for reading.
+    int ReadFD = 0;
+    ASSERT_NO_ERROR(fs::openFileForRead(TargetFileName, ReadFD));
+    ScopedFD EventuallyCloseIt(ReadFD);
+
+    // Confirm we can replace the file while it is open.
+    EXPECT_TRUE(!fs::rename(SourceFileName, TargetFileName));
+
+    // We should still be able to read the old data through the existing
+    // descriptor.
+    auto Buffer = MemoryBuffer::getOpenFile(ReadFD, TargetFileName, -1);
+    ASSERT_TRUE(static_cast<bool>(Buffer));
+    EXPECT_EQ(Buffer.get()->getBuffer(), "!!target!!");
+
+    // The source file should no longer exist
+    EXPECT_FALSE(fs::exists(SourceFileName));
+  }
+
+  {
+    // If we obtain a new descriptor for the target file, we should find that it
+    // contains the content that was in the source file.
+    int ReadFD = 0;
+    ASSERT_NO_ERROR(fs::openFileForRead(TargetFileName, ReadFD));
+    ScopedFD EventuallyCloseIt(ReadFD);
+    auto Buffer = MemoryBuffer::getOpenFile(ReadFD, TargetFileName, -1);
+    ASSERT_TRUE(static_cast<bool>(Buffer));
+
+    EXPECT_EQ(Buffer.get()->getBuffer(), "!!source!!");
+  }
+
+  // Rename the target file back to the source file name to confirm that rename
+  // still works if the destination does not already exist.
+  EXPECT_TRUE(!fs::rename(TargetFileName, SourceFileName));
+  EXPECT_FALSE(fs::exists(TargetFileName));
+  ASSERT_TRUE(fs::exists(SourceFileName));
+
+  // Clean up.
+  ASSERT_NO_ERROR(fs::remove(SourceFileName));
+  ASSERT_NO_ERROR(fs::remove(TestDirectory.str()));
+}
+
+}  // anonymous namespace