Use a single file name to represent a lock

[• rafael]

Summary

Currently the lock logic is

• Create a temp file
• Create a symbolic link on unix or a hard link on windows
• Delete both when we are done with it

The problem is that CreateHardLink on windows will fail if the file has a DeleteFile disposition which I want to use in TempFile.

The solution I think is to use a single file for the lock. On windows that is simple as rename can be set to fail if the destination exists.

On posix we first create the link as we do now, but immediately rename the original file onto it.

[zturner]

Out of curiosity, what do we use LockFileManager for in LLVM, and what are you trying to use it for in this specific use case? Because of certain quirks in Windows file system semantics, lock files are in general not a robust solution most of the time -- maybe even ever.

If all use cases of lock files are to synchronize cross-process, why don't we just re-write LockFileManager to use a named mutex?

[amccarth]

I share zturner's concerns that file-based lock implementations on Windows are inherently fraught with pitfalls. Can't we use a Mutex or other OS-provided synchronization mechanism? Those will be more efficient, more robust, and they won't leave artifacts if the program is interrupted or crashes.

include/llvm/Support/FileSystem.h

386

I'm not a fan of this interface, with both From and FD. It's confusing and seems prone to being called erroneously. What if the caller accidentally specifies a From that doesn't correspond to FD?

And it appears that neither implementation currently uses FD. I see the FIXME that suggests it might be useful in the future. Can we remove FD for now and revisit it later?

738

I had to read the second sentence a couple times to understand it, and, once I did, it still left me with questions. For example, if there is an existing file with Name, does the function return an error? May I suggest:

// Change the name of the temporary file.  If a file with Name already exists,
// this does nothing and returns an Error.  This will still be a temporary
// file, so keep or discard should still be called.

lib/Support/Unix/Path.inc

433

s/istead/instead/

lib/Support/Windows/Path.inc

1042

s/.begin()/.data()/

[• rafael]

Don't use DELETE access unconditionally. It can block non llvm code from accessing the file if they don't use SHARE_DELETE.

[• rafael]

Address review comments.

[• rafael]

rebased

[• rafael]

I found a way to use FILE_FLAG_DELETE_ON_CLOSE without this.

I think think that using a single file for the lock is an improvement, but it is now independent.

[rnk]

I just assume that LockFileManager doesn't actually work on Windows. It's really only used by implicit modules, and we're pushing people away from those anyway. If we don't need this for the TempFile migration, I say let sleeping broken code lie, unless you think this simplification is worth it for Posix systems.

include/llvm/Support/FileSystem.h

 /// 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.
 std::error_code rename(const Twine &from, const Twine &to);
 
+/// Rename from From to To, but don't replace To if it exists.
+///
+/// This is not atomic on posix. Both names will be visible for a short time.
+///
+/// FD should correspond to From. It is used instead of From when possible.
+std::error_code rename_dont_replace(const Twine &From, int FD, const Twine &To);
+
 /// @brief Copy the contents of \a From to \a To.
 ///
 /// @param From The path to copy from.
 /// @param To The path to copy to. This is created.
 std::error_code copy_file(const Twine &From, const Twine &To);
 
 /// @brief Resize path to size. File is resized as if by POSIX truncate().
 ///
public:
   int FD = -1;
 
   // Keep this with the given name.
   Error keep(const Twine &Name);
 
   // Keep this with the temporary name.
   Error keep();
 
+  // Change the name of the temporary file. This doesn't replace Name if it
+  // already exits. This will still be a temporary file and keep or discard
+  // should still be called.
+  Error rename(const Twine &Name);
+
   // Delete the file.
   Error discard();
 
   // This checks that keep or delete was called.
   ~TempFile();
 };
 
 /// @brief Create a file in the system temporary directory.

lib/Support/LockFileManager.cpp

if (Out.has_error()) {
       std::string S("failed to write to ");
       S.append(UniqueLockFile->TmpName);
       setError(Out.error(), S);
       return;
     }
   }
 
   while (true) {
-    // Create a link from the lock file name. If this succeeds, we're done.
-    std::error_code EC =
-        sys::fs::create_link(UniqueLockFile->TmpName, LockFileName);
-    if (!EC) {
+    // Rename the lock. If this succeeds, we're done.
+    Error E = UniqueLockFile->rename(LockFileName);
+    if (!E) {
       RemoveTempFile.cancel();
       return;
     }
-
+    handleAllErrors(std::move(E), [&](const ECError &E) {
+      std::error_code EC = E.convertToErrorCode();
       if (EC != errc::file_exists) {
         std::string S("failed to create link ");
         raw_string_ostream OSS(S);
         OSS << LockFileName.str() << " to " << UniqueLockFile->TmpName;
         setError(EC, OSS.str());
-      return;
       }
+    });
+    if (ErrorCode)
+      return;
 
     // Someone else managed to create the lock file first. Read the process ID
     // from the lock file.
     if ((Owner = readLockFile(LockFileName)))
       return; // RemoveTempFile will delete out our unique lock file.
 
     if (!sys::fs::exists(LockFileName)) {
       // The previous owner released the lock file before we could read it.
       // Try to get ownership again.
       continue;
     }
 
     // There is a lock file that nobody owns; try to clean it up and get
     // ownership.
-    if ((EC = sys::fs::remove(LockFileName))) {
+    if (std::error_code EC = sys::fs::remove(LockFileName)) {
       std::string S("failed to remove lockfile ");
       S.append(LockFileName.str());
       setError(EC, S);
       return;
     }
   }
 }
 
std::string LockFileManager::getErrorMessage() const {
   }
   return "";
 }
 
 LockFileManager::~LockFileManager() {
   if (getState() != LFS_Owned)
     return;
 
-  // Since we own the lock, remove the lock file and our own unique lock file.
-  sys::fs::remove(LockFileName);
+  // Since we own the lock, remove it.
   consumeError(UniqueLockFile->discard());
 }
 
 LockFileManager::WaitForUnlockResult LockFileManager::waitForUnlock() {
   if (getState() != LFS_Shared)
     return Res_Success;
 
 #if LLVM_ON_WIN32

lib/Support/Path.cpp

if (close(FD) == -1) {
     std::error_code EC(errno, std::generic_category());
     return errorCodeToError(EC);
   }
   FD = -1;
 
   return Error::success();
 }
 
+Error TempFile::rename(const Twine &Name) {
+  SmallString<128> NameStorage;
+  StringRef N = Name.toStringRef(NameStorage);
+  if (std::error_code EC = rename_dont_replace(TmpName, FD, N))
+    return errorCodeToError(EC);
+  TmpName = N;
+  return Error::success();
+}
+
 Expected<TempFile> TempFile::create(const Twine &Model, unsigned Mode) {
   int FD;
   SmallString<128> ResultPath;
   if (std::error_code EC = createUniqueFile(Model, FD, ResultPath, Mode))
     return errorCodeToError(EC);
 
   // Make sure we delete the file when RemoveFileOnSignal fails.
   TempFile Ret(ResultPath, FD);

lib/Support/Unix/Path.inc

std::error_code rename(const Twine &from, const Twine &to) {
   StringRef t = to.toNullTerminatedStringRef(to_storage);
 
   if (::rename(f.begin(), t.begin()) == -1)
     return std::error_code(errno, std::generic_category());
 
   return std::error_code();
 }
 
+std::error_code rename_dont_replace(const Twine &From, int FD,
+                                    const Twine &To) {
+  // FIXME: on linux we could use
+  // renameat2(AT_FDCWD, from, AT_FDCWD, to, RENAME_NOREPLACE)
+
+  // The natural way of implementing this would be to create a hard
+  // link named To and then unlink From.
+  // It seems we have to support SMB file systems and hard links don't work on
+  // those.
+  // What we do istead is create a soft link and rename From over To. That way
+  // we avoid hard links and any reader will always see a valid file.
+  if (std::error_code EC = create_link(From, To))
+    return EC;
+  return rename(From, To);
+}
+
 std::error_code resize_file(int FD, uint64_t Size) {
 #if defined(HAVE_POSIX_FALLOCATE)
   // If we have posix_fallocate use it. Unlike ftruncate it always allocates
   // space, so we get an error if the disk is full.
   if (int Err = ::posix_fallocate(FD, 0, Size)) {
     if (Err != EINVAL && Err != EOPNOTSUPP)
       return std::error_code(Err, std::generic_category());
   }

lib/Support/Windows/Path.inc

if (FromHandle)
       break;
   }
   if (!FromHandle)
     return mapWindowsError(GetLastError());
 
   return rename_handle(FromHandle, To);
 }
 
+std::error_code rename_dont_replace(const Twine &From, int FD,
+                                    const Twine &To) {
+  HANDLE FromHandle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
+  return rename_internal(FromHandle, To, false);
+}
+
 std::error_code resize_file(int FD, uint64_t Size) {
 #ifdef HAVE__CHSIZE_S
   errno_t error = ::_chsize_s(FD, Size);
 #else
   errno_t error = ::_chsize(FD, Size);
 #endif
   return std::error_code(error, std::generic_category());
 }
std::error_code openFileForWrite(const Twine &Name, int &ResultFD,
   DWORD CreationDisposition;
   if (Flags & F_Excl)
     CreationDisposition = CREATE_NEW;
   else if (Flags & F_Append)
     CreationDisposition = OPEN_ALWAYS;
   else
     CreationDisposition = CREATE_ALWAYS;
 
-  DWORD Access = GENERIC_WRITE;
+  DWORD Access = GENERIC_WRITE | DELETE;
   if (Flags & F_RW)
     Access |= GENERIC_READ;
 
   HANDLE H =
       ::CreateFileW(PathUTF16.begin(), Access,
                     FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
                     NULL, CreationDisposition, 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