Fix an occasional failure in fs::rename() if another process uses fs::openFileForRead() on the same file

[gbedwell]

Summary

Hi,

While looking at the effectiveness of Clang C++ modules on Windows, we encountered some occasional but seemingly random failures. Upon investigation we found that it is actually a general issue affecting the Windows implementation of llvm::sys::fs::rename() and llvm::sys::fs::openFileForRead().

Within this function there is a retry loop that spins for a maximum of around 2000ms. In this loop, the Windows MoveFileExW() function is used to rename the file. If the move fails with ERROR_ACCESS_DENIED, it goes around the loop and tries again. A comment in the code suggests the reason for permitting this error condition is to allow for scanners (indexers, antivirus, etc) that open the source file immediately after it is written to complete their work. However, the ERROR_ACCESS_DENIED error code also appears if the destination file is opened for reading by another process using llvm::sys::fs::openFileForRead(). It is usually the case that the reading completes in less than 2000ms, so the issue doesn't often result in a failure that escapes the retry loop, however we've seen cases where during a build the disk is being hit so hard that disk response times can be longer than this causing llvm::sys::fs::rename() to fail.

The attached patch contains a unit test (ReplaceFileTest.cpp) that demonstrates the issue and fix. In the test we create a directory containg two files: 'source' and 'target'. 'target' is opened for reading with openFileForRead() and then replaced with 'source' using rename(source, target). Using the existing descriptor it verifies that the old data can still be read, but with the descriptor from a subsequent call to openFileForRead() the new data is read instead. Without the fixes to Path.inc, the call to rename() in the test will fail.

The fix is to instead use the Windows ReplaceFileW() function documented at https://msdn.microsoft.com/en-us/library/windows/desktop/aa365512(v=vs.85).aspx within the retry loop in rename() and to add the FILE_SHARE_DELETE flag to the share mode flags in the call to CreateFileW() within openFileForRead(). We still need to keep the old call to MoveFileExW() for the case where the target of the rename does not already exist, as ReplaceFileW() will fail with ERROR_FILE_NOT_FOUND in this situation. This case is also covered by the unit test for completeness.

This patch is based on an investigation and an initial version of the patch by my colleague Edd Dawson.

Thanks for taking a look.

Thanks,

-Greg

[aaron.ballman]

Thank you for looking into this! LGTM

~Aaron

lib/Support/Windows/Path.inc

std::error_code rename(const Twine &from, const Twine &to) {
   SmallVector<wchar_t, 128> wide_from;
   SmallVector<wchar_t, 128> wide_to;
   if (std::error_code ec = widenPath(from, wide_from))
     return ec;
   if (std::error_code ec = widenPath(to, wide_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);
   }
 
   return ec;
 }
 
 std::error_code resize_file(int FD, uint64_t Size) {
 #ifdef HAVE__CHSIZE_S

 }
 
 std::error_code openFileForRead(const Twine &Name, int &ResultFD) {
   SmallVector<wchar_t, 128> PathUTF16;
 
   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);
     // Provide a better error message when trying to open directories.
     // This only runs if we failed to open the file, so there is probably
     // no performances issues.
     if (LastError != ERROR_ACCESS_DENIED)
       return EC;

unittests/Support/CMakeLists.txt

add_llvm_unittest(SupportTests
   ManagedStatic.cpp
   MathExtrasTest.cpp
   MemoryBufferTest.cpp
   MemoryTest.cpp
   Path.cpp
   ProcessTest.cpp
   ProgramTest.cpp
   RegexTest.cpp
+  ReplaceFileTest.cpp
   ScaledNumberTest.cpp
   SourceMgrTest.cpp
   SpecialCaseListTest.cpp
   StreamingMemoryObject.cpp
   StringPool.cpp
   SwapByteOrderTest.cpp
   TargetRegistry.cpp
   ThreadLocalTest.cpp

unittests/Support/ReplaceFileTest.cpp

+//===- 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