From: Greg Bedwell Date: Mon, 12 Oct 2015 15:11:47 +0000 (+0000) Subject: Fix rename() sometimes failing if another process uses openFileForRead() X-Git-Url: http://plrg.eecs.uci.edu/git/?p=oota-llvm.git;a=commitdiff_plain;h=d02add1701a126f37332217bbe17356bfa2a9a0f 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 1ee45c6f362..2c111321c60 100644 --- 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 9c2aab43a6a..fd8324c836d 100644 --- 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 index 00000000000..8b16daf3233 --- /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(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(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