Index: llvm/trunk/include/llvm/Support/FileSystem.h =================================================================== --- llvm/trunk/include/llvm/Support/FileSystem.h +++ llvm/trunk/include/llvm/Support/FileSystem.h @@ -343,7 +343,11 @@ /// platform-specific error code. std::error_code remove_directories(const Twine &path, bool IgnoreErrors = true); -/// @brief Rename \a from to \a to. Files are renamed as if by POSIX rename(). +/// @brief Rename \a from to \a to. +/// +/// Files are renamed as if by POSIX rename(), except that on Windows there may +/// be a short interval of time during which the destination file does not +/// exist. /// /// @param from The path to rename from. /// @param to The path to rename to. This is created. Index: llvm/trunk/lib/Support/Windows/Path.inc =================================================================== --- llvm/trunk/lib/Support/Windows/Path.inc +++ llvm/trunk/lib/Support/Windows/Path.inc @@ -359,65 +359,126 @@ return is_local_internal(FinalPath, Result); } -std::error_code rename(const Twine &from, const Twine &to) { +static std::error_code rename_internal(HANDLE FromHandle, const Twine &To, + bool ReplaceIfExists) { + SmallVector ToWide; + if (auto EC = widenPath(To, ToWide)) + return EC; + + std::vector RenameInfoBuf(sizeof(FILE_RENAME_INFO) - sizeof(wchar_t) + + (ToWide.size() * sizeof(wchar_t))); + FILE_RENAME_INFO &RenameInfo = + *reinterpret_cast(RenameInfoBuf.data()); + RenameInfo.ReplaceIfExists = ReplaceIfExists; + RenameInfo.RootDirectory = 0; + RenameInfo.FileNameLength = ToWide.size(); + std::copy(ToWide.begin(), ToWide.end(), RenameInfo.FileName); + + if (!SetFileInformationByHandle(FromHandle, FileRenameInfo, &RenameInfo, + RenameInfoBuf.size())) + return mapWindowsError(GetLastError()); + + return std::error_code(); +} + +std::error_code rename(const Twine &From, const Twine &To) { // Convert to utf-16. - SmallVector wide_from; - SmallVector wide_to; - if (std::error_code ec = widenPath(from, wide_from)) - return ec; - if (std::error_code ec = widenPath(to, wide_to)) - return ec; + SmallVector WideFrom; + SmallVector WideTo; + if (std::error_code EC = widenPath(From, WideFrom)) + return EC; + if (std::error_code EC = widenPath(To, WideTo)) + return EC; - std::error_code ec = std::error_code(); + ScopedFileHandle FromHandle; + // Retry this a few times to defeat badly behaved file system scanners. + for (unsigned Retry = 0; Retry != 200; ++Retry) { + if (Retry != 0) + ::Sleep(10); + FromHandle = + ::CreateFileW(WideFrom.begin(), GENERIC_READ | DELETE, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); + if (FromHandle) + break; + } + if (!FromHandle) + return mapWindowsError(GetLastError()); - // 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++) { - 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.data(), wide_from.data(), NULL, 0, NULL, NULL)) - return std::error_code(); - - DWORD 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) + // We normally expect this loop to succeed after a few iterations. If it + // requires more than 200 tries, it's more likely that the failures are due to + // a true error, so stop trying. + for (unsigned Retry = 0; Retry != 200; ++Retry) { + auto EC = rename_internal(FromHandle, To, true); + if (!EC || EC != errc::permission_denied) + return EC; + + // The destination file probably exists and is currently open in another + // process, either because the file was opened without FILE_SHARE_DELETE or + // it is mapped into memory (e.g. using MemoryBuffer). Rename it in order to + // move it out of the way of the source file. Use FILE_FLAG_DELETE_ON_CLOSE + // to arrange for the destination file to be deleted when the other process + // closes it. + ScopedFileHandle ToHandle( + ::CreateFileW(WideTo.begin(), GENERIC_READ | DELETE, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + NULL, OPEN_EXISTING, + FILE_ATTRIBUTE_NORMAL | FILE_FLAG_DELETE_ON_CLOSE, NULL)); + if (!ToHandle) { + auto EC = mapWindowsError(GetLastError()); + // Another process might have raced with us and moved the existing file + // out of the way before we had a chance to open it. If that happens, try + // to rename the source file again. + if (EC == errc::no_such_file_or_directory) continue; - // We get ERROR_FILE_NOT_FOUND if the destination file is missing. - // MoveFileEx can handle this case. - if (ReplaceError != ERROR_ACCESS_DENIED && - ReplaceError != ERROR_FILE_NOT_FOUND && - ReplaceError != ERROR_SHARING_VIOLATION) - break; + return EC; } - if (::MoveFileExW(wide_from.begin(), wide_to.begin(), - MOVEFILE_COPY_ALLOWED | MOVEFILE_REPLACE_EXISTING)) - return std::error_code(); + BY_HANDLE_FILE_INFORMATION FI; + if (!GetFileInformationByHandle(ToHandle, &FI)) + return mapWindowsError(GetLastError()); + + // Try to find a unique new name for the destination file. + for (unsigned UniqueId = 0; UniqueId != 200; ++UniqueId) { + std::string TmpFilename = (To + ".tmp" + utostr(UniqueId)).str(); + if (auto EC = rename_internal(ToHandle, TmpFilename, false)) { + if (EC == errc::file_exists || EC == errc::permission_denied) { + // Again, another process might have raced with us and moved the file + // before we could move it. Check whether this is the case, as it + // might have caused the permission denied error. If that was the + // case, we don't need to move it ourselves. + ScopedFileHandle ToHandle2(::CreateFileW( + WideTo.begin(), 0, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL, + OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL)); + if (!ToHandle2) { + auto EC = mapWindowsError(GetLastError()); + if (EC == errc::no_such_file_or_directory) + break; + return EC; + } + BY_HANDLE_FILE_INFORMATION FI2; + if (!GetFileInformationByHandle(ToHandle2, &FI2)) + return mapWindowsError(GetLastError()); + if (FI.nFileIndexHigh != FI2.nFileIndexHigh || + FI.nFileIndexLow != FI2.nFileIndexLow || + FI.dwVolumeSerialNumber != FI2.dwVolumeSerialNumber) + break; + continue; + } + return EC; + } + break; + } - DWORD MoveError = ::GetLastError(); - ec = mapWindowsError(MoveError); - if (MoveError != ERROR_ACCESS_DENIED) break; + // Okay, the old destination file has probably been moved out of the way at + // this point, so try to rename the source file again. Still, another + // process might have raced with us to create and open the destination + // file, so we need to keep doing this until we succeed. } - return ec; + // The most likely root cause. + return errc::permission_denied; } std::error_code resize_file(int FD, uint64_t Size) { Index: llvm/trunk/unittests/Support/ReplaceFileTest.cpp =================================================================== --- llvm/trunk/unittests/Support/ReplaceFileTest.cpp +++ llvm/trunk/unittests/Support/ReplaceFileTest.cpp @@ -52,6 +52,21 @@ ~ScopedFD() { Process::SafelyCloseFileDescriptor(FD); } }; +bool FDHasContent(int FD, StringRef Content) { + auto Buffer = MemoryBuffer::getOpenFile(FD, "", -1); + assert(Buffer); + return Buffer.get()->getBuffer() == Content; +} + +bool FileHasContent(StringRef File, StringRef Content) { + int FD = 0; + auto EC = fs::openFileForRead(File, FD); + (void)EC; + assert(!EC); + ScopedFD EventuallyCloseIt(FD); + return FDHasContent(FD, Content); +} + TEST(rename, FileOpenedForReadingCanBeReplaced) { // Create unique temporary directory for this test. SmallString<128> TestDirectory; @@ -79,25 +94,15 @@ // 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(Buffer)); - EXPECT_EQ(Buffer.get()->getBuffer(), "!!target!!"); + EXPECT_TRUE(FDHasContent(ReadFD, "!!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(Buffer)); - - EXPECT_EQ(Buffer.get()->getBuffer(), "!!source!!"); - } + // If we obtain a new descriptor for the target file, we should find that it + // contains the content that was in the source file. + EXPECT_TRUE(FileHasContent(TargetFileName, "!!source!!")); // Rename the target file back to the source file name to confirm that rename // still works if the destination does not already exist. @@ -110,4 +115,59 @@ ASSERT_NO_ERROR(fs::remove(TestDirectory.str())); } +TEST(rename, ExistingTemp) { + // Test that existing .tmpN files don't get deleted by the Windows + // sys::fs::rename implementation. + SmallString<128> TestDirectory; + ASSERT_NO_ERROR( + fs::createUniqueDirectory("ExistingTemp-test", TestDirectory)); + + SmallString<128> SourceFileName(TestDirectory); + path::append(SourceFileName, "source"); + + SmallString<128> TargetFileName(TestDirectory); + path::append(TargetFileName, "target"); + + SmallString<128> TargetTmp0FileName(TestDirectory); + path::append(TargetTmp0FileName, "target.tmp0"); + + SmallString<128> TargetTmp1FileName(TestDirectory); + path::append(TargetTmp1FileName, "target.tmp1"); + + ASSERT_NO_ERROR(CreateFileWithContent(SourceFileName, "!!source!!")); + ASSERT_NO_ERROR(CreateFileWithContent(TargetFileName, "!!target!!")); + ASSERT_NO_ERROR(CreateFileWithContent(TargetTmp0FileName, "!!target.tmp0!!")); + + { + // Use mapped_file_region to make sure that the destination file is mmap'ed. + // This will cause SetInformationByHandle to fail when renaming to the + // destination, and we will follow the code path that tries to give target + // a temporary name. + int TargetFD; + std::error_code EC; + ASSERT_NO_ERROR(fs::openFileForRead(TargetFileName, TargetFD)); + ScopedFD X(TargetFD); + sys::fs::mapped_file_region MFR( + TargetFD, sys::fs::mapped_file_region::readonly, 10, 0, EC); + ASSERT_FALSE(EC); + + ASSERT_NO_ERROR(fs::rename(SourceFileName, TargetFileName)); + +#ifdef _WIN32 + // Make sure that target was temporarily renamed to target.tmp1 on Windows. + // This is signified by a permission denied error as opposed to no such file + // or directory when trying to open it. + int Tmp1FD; + EXPECT_EQ(errc::permission_denied, + fs::openFileForRead(TargetTmp1FileName, Tmp1FD)); +#endif + } + + EXPECT_TRUE(FileHasContent(TargetTmp0FileName, "!!target.tmp0!!")); + + ASSERT_NO_ERROR(fs::remove(TargetFileName)); + ASSERT_NO_ERROR(fs::remove(TargetTmp0FileName)); + ASSERT_NO_ERROR(fs::remove(TestDirectory.str())); +} + } // anonymous namespace