Improve reliability of file renaming in Windows

[dyung]

Summary

Improve the reliability of file renaming in Windows by having the compiler retry the rename operation on 3 error conditions of ReplaceFileW() that it was previously bailing out on.

[vsk]

lib/Support/Windows/Path.inc

275

Does ReplaceError need to be declared a DWORD somewhere? I'm not familiar with this code, I could just be missing something.

[dyung]

Fixed missing declaration identified by vsk.

[rnk]

lib/Support/Windows/Path.inc

271

These begin() calls should be data() calls.

281

MSDN says this for ERROR_UNABLE_TO_MOVE_REPLACEMENT_2:
"The file to be replaced still exists with a different name."

Should we do anything about that?

[dyung]

Changed .begin() calls to .data() calls as suggested in review.

[dyung]

lib/Support/Windows/Path.inc

281

We felt that this situation was not possible in this case since we call ReplaceFileW with NULL as the value for lpBackupFileName, so we felt that no backup file would be made. Because of that, if this error occurs, we thought the result would be the same as the ERROR_UNABLE_TO_MOVE_REPLACEMENT, so we would handle it the same way.

[rnk]

lgtm

lib/Support/Windows/Path.inc

281

I can agree with that interpretation. :)

lib/Support/Windows/Path.inc

std::error_code rename(const Twine &from, const Twine &to) {
   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.
+  // Retry while we see recoverable errors.
   // System scanners (eg. indexer) might open the source file when it is written
   // and closed.
 
+  bool TryReplace = true;
+
   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))
+    if (i > 0)
+      ::Sleep(1);
+
+    if (TryReplace) {
+      // 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();
 
+      ReplaceError = ::GetLastError();
+      ec = mapWindowsError(ReplaceError);
+
+      // If ReplaceFileW returned ERROR_UNABLE_TO_MOVE_REPLACEMENT or
+      // ERROR_UNABLE_TO_MOVE_REPLACEMENT_2, retry but only use MoveFileExW().
+      if (ReplaceError == ERROR_UNABLE_TO_MOVE_REPLACEMENT ||
+          ReplaceError == ERROR_UNABLE_TO_MOVE_REPLACEMENT_2) {
+        TryReplace = false;
+        continue;
+      }
+      // If ReplaceFileW returned ERROR_UNABLE_TO_REMOVE_REPLACED, retry
+      // using ReplaceFileW().
+      if (ReplaceError == ERROR_UNABLE_TO_REMOVE_REPLACED)
+        continue;
       // 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 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
   errno_t error = ::_chsize_s(FD, Size);