[Support] [Windows] Manually clean up temp files on network shares

[mstorsjo]

Summary

Since D81803 / 79657e2339b58bc01fe1b85a448bb073d57d90bb, temp files
created on network shares don't set "Disposition.DeleteFile = true".
This flag normally takes care of removing the temp file both if the
process exits abnormally, and when the file is closed cleanly.

Even if we can't seem to use the flag on network shares, we can
at least make sure to remove the temp files when they are closed
cleanly.

This patch adjusts setDeleteDisposition to take a bool &Delete
reference, to let the caller know whether it actually set the
flag or not (on network shares, it returns a success error code
but doesn't do what it was asked to).

When setDeleteDisposition is called from openNativeFile, we can't
easily pass this information back to the caller without adding
extra plumbing through all the layers of functions up to the
caller in TempFile::create.

Instead have TempFile::create() do a duplicate call to
setDeleteDisposition, to see whether it actually sets the
desired flag. If not, set a Windows specific flag in TempFile,
and manually remove the file in discard().

This fixes https://github.com/mstorsjo/llvm-mingw/issues/233.

[jhenderson]

llvm/lib/Support/Path.cpp

1212

I'm not really liking the duplicate code between this and the below non-Windows block. Is there a way we could avoid this duplication?

1245–1246

Similar comment to above: this code is duplicated a few lines down. Perhaps another function, or maybe even mode the code into setDeleteDisposition somehow?

[mstorsjo]

llvm/lib/Support/Path.cpp

1212

Sure, it should be pretty straightforward to make it one generic codepath with a couple smaller platform specific ifdefs instead.

1245–1246

Moving it into setDeleteDisposition is hard as that one isn't a member of TempFile, but I can try to factorize the duplication at least.

[mstorsjo]

Get rid of most of the code duplication as requested.

[jhenderson]

Mostly minor comments. LGTM with them addressed (or not, if you disagree).

llvm/lib/Support/Path.cpp

1209–1211

Whilst modifying the comment, let's capitalize windows -> Windows (since it's a proper noun). I also think it needs a comma.

1237–1238

I think the consensus I've seen elsewhere is that a lambda is a function object, i.e. a variable, not a function, so needs UpperCamelCase name styling.

1317

Nit: clang-format.

1325

llvm/lib/Support/Windows/Path.inc

404

Perhaps this variable should be renamed to WillAutoDelete or something like that? That might help express the meaning of changing its value. Although I'm not certain, since there's really two meanings - should delete and will auto delete.

(I don't particularly like the non-const input reference, but I don't see a better alternative at this point)

1184–1185

Perhaps ShouldDelete.

[mstorsjo]

Thanks for the review! I'll leave this open for further comments and then apply tomorrow unless there's differing opinions.

[mstorsjo]

Applied the suggested changes.

[rnk]

Here's an idea to try to make this a bit cleaner. Maybe setDeleteDisposition should return an error code when the path is on a network drive. That return value would be propagated back up to the caller who sets OF_Delete. In this case, that is TempFile::create. At that point, TempFile can retry temporary file creation without OF_Delete.

The downside to this is that we will create the file, set the disposition (fails), delete the file, and recreate it again without OF_Delete. To improve on that, we could have some kind of canSetDeleteDisposition portable API, which would feed into the choice to set the OF_Delete flag.

[mstorsjo]

In D111875#3071269, @rnk wrote:

Here's an idea to try to make this a bit cleaner. Maybe setDeleteDisposition should return an error code when the path is on a network drive. That return value would be propagated back up to the caller who sets OF_Delete. In this case, that is TempFile::create. At that point, TempFile can retry temporary file creation without OF_Delete.

Hmm... It would indeed be cleaner in the sense that setDeleteDisposition doesn't lie about whether it succeeded or not - but that would require using a specific error code for it that the caller would look for (so we don't retry without the flag for any random error) and hope that we don't map other real system errors to that error code.

The downside to this is that we will create the file, set the disposition (fails), delete the file, and recreate it again without OF_Delete.

Yeah, that's in practice probably a much bigger overhead than just calling setDeleteDisposition one more time.

To improve on that, we could have some kind of canSetDeleteDisposition portable API, which would feed into the choice to set the OF_Delete flag.

Hmm, I guess that could work too. It requires mimicing the logic/checks from setDeleteDisposition - or maybe we could remove the logic from there altogether? (I think it should be kept though, so that OF_Delete behaves roughly as documented even in those cases, though.)

[rnk]

In D111875#3071350, @mstorsjo wrote:

In D111875#3071269, @rnk wrote:

Here's an idea to try to make this a bit cleaner. Maybe setDeleteDisposition should return an error code when the path is on a network drive. That return value would be propagated back up to the caller who sets OF_Delete. In this case, that is TempFile::create. At that point, TempFile can retry temporary file creation without OF_Delete.

Hmm... It would indeed be cleaner in the sense that setDeleteDisposition doesn't lie about whether it succeeded or not - but that would require using a specific error code for it that the caller would look for (so we don't retry without the flag for any random error) and hope that we don't map other real system errors to that error code.

Yep, it mostly requires making a new error category and defining a new error. A bit annoying, but not too hard. Alternatively, if this is all handled in TempFile::create and no recovery is needed, you can use a generic error code.

The downside to this is that we will create the file, set the disposition (fails), delete the file, and recreate it again without OF_Delete.

Yeah, that's in practice probably a much bigger overhead than just calling setDeleteDisposition one more time.

To improve on that, we could have some kind of canSetDeleteDisposition portable API, which would feed into the choice to set the OF_Delete flag.

Hmm, I guess that could work too. It requires mimicing the logic/checks from setDeleteDisposition - or maybe we could remove the logic from there altogether? (I think it should be kept though, so that OF_Delete behaves roughly as documented even in those cases, though.)

I think it's already reasonably well-factored already. It all just boils down to calling is_local on the directory path where the temporary file will be created, right?

[mstorsjo]

In D111875#3071388, @rnk wrote:

In D111875#3071350, @mstorsjo wrote:

In D111875#3071269, @rnk wrote:

Here's an idea to try to make this a bit cleaner. Maybe setDeleteDisposition should return an error code when the path is on a network drive. That return value would be propagated back up to the caller who sets OF_Delete. In this case, that is TempFile::create. At that point, TempFile can retry temporary file creation without OF_Delete.

Hmm... It would indeed be cleaner in the sense that setDeleteDisposition doesn't lie about whether it succeeded or not - but that would require using a specific error code for it that the caller would look for (so we don't retry without the flag for any random error) and hope that we don't map other real system errors to that error code.

Yep, it mostly requires making a new error category and defining a new error. A bit annoying, but not too hard. Alternatively, if this is all handled in TempFile::create and no recovery is needed, you can use a generic error code.

Well there's a couple uses in TempFile::keep() too. Then again, with a bit more changes, we could stop calling them too, if we aren't using the delete disposition for this file altogether.

The downside to this is that we will create the file, set the disposition (fails), delete the file, and recreate it again without OF_Delete.

Yeah, that's in practice probably a much bigger overhead than just calling setDeleteDisposition one more time.

To improve on that, we could have some kind of canSetDeleteDisposition portable API, which would feed into the choice to set the OF_Delete flag.

Hmm, I guess that could work too. It requires mimicing the logic/checks from setDeleteDisposition - or maybe we could remove the logic from there altogether? (I think it should be kept though, so that OF_Delete behaves roughly as documented even in those cases, though.)

I think it's already reasonably well-factored already. It all just boils down to calling is_local on the directory path where the temporary file will be created, right?

Yes, I guess so.

I guess this sounds like a decent improvement overall - I can try to give this a shot, in a day or two.

[rnk]

In D111875#3071392, @mstorsjo wrote:

Well there's a couple uses in TempFile::keep() too. Then again, with a bit more changes, we could stop calling them too, if we aren't using the delete disposition for this file altogether.

The RemoveOnClose boolean seems like it could work there, though. It essentially tracks, "do I need to delete manually, or will the OS take care of it for me". Maybe I misunderstood and it isn't quite that.

Anyway, thanks for giving it a try.

[mstorsjo]

Updated according to what was discussed with @rnk.

I ran into one big surprise regarding this though: If we don't pass OF_Delete, we don't set the DELETE access mode on the opened file. And without that, renaming the temp file via SetFileInformationByHandle(FileRenameInfo) fails. (Renaming it via the fallback codepath MoveFileEx succeeds though.) So either we need to always pass DELETE, or we'd need to add yet another windows specific open flag, OF_Deletable (in addition to the existing OF_Delete).

Then on the other hand; if we always end up opening files with DELETE, then we could just skip passing OF_Delete and try calling setDeleteDisposition altogether, and return the result in an output parameter like in the previous iteration (although I guess it can be argued that this approach is cleaner anyway).

Other open questions: I now change setDeleteDisposition to error out on a network file system. This is a behaviour change if someone else is using OF_Delete on a network file system - but it's probably ok? There are no other uses of OF_Delete in the llvm-project monorepo though.

Should we give RemoveOnClose an even more explicit name? As it's now unclear whether it says that the OS will implicitly remove it on close, or that we must remove it manually on close. ManuallyRemoveOnClose sounds quite inelegant though...

[rnk]

I think this is reasonable

@probinson, is there someone at Sony with Windows FS experience who can sign off on this change? LLVM will now request delete access when it opens files for writing.

llvm/lib/Support/Windows/Path.inc

1110

I think this should probably be under if (FA_Write), since only need delete access when opening a file for writing. We want to be able to open readonly files for reading.

Otherwise, yeah, I think it makes sense to request delete access for any file that LLVM needs to write. LLVM rarely attempts to update files in place, and it matches the Unix model (I think).

[mstorsjo]

Moved to setting DELETE based on FA_Write, kept the old clause for OF_Delete too for clarity. Removed conditions around calling setDeleteDisposition(false) as the temp files are opened with the DELETE flag anyway.

[akhuang]

This also appears to fix another llvm bug filed on a windows ram drive issue (https://bugs.llvm.org/show_bug.cgi?id=52080), which is cool.

llvm/lib/Support/Windows/Path.inc

417

Should we change this comment (or maybe just remove this section?), since it seems like this patch tries to avoid this path?

[mstorsjo]

In D111875#3076260, @akhuang wrote:

This also appears to fix another llvm bug filed on a windows ram drive issue (https://bugs.llvm.org/show_bug.cgi?id=52080), which is cool.

Oh, nice!

llvm/lib/Support/Windows/Path.inc

417

Good catch! Yes we should change that. I'll revise the patch.

[mstorsjo]

Update a comment about leaving temp files behind.

[mstorsjo]

@rnk - Actually, we need to backtrack a bit on this. This breaks two unit tests in clang, CrossTU/./CrossTUTests.exe/CrossTranslationUnit.CanLoadFunctionDefinition and CrossTU/./CrossTUTests.exe/CrossTranslationUnit.IndexFormatCanBeParsed.

The core of the issue lies here: https://github.com/llvm/llvm-project/blob/main/clang/unittests/CrossTU/CrossTranslationUnitTest.cpp#L164-L171

The test first creates a temporary file with llvm::sys::fs::createTemporaryFile (which is a different thing than TempFile and uses RemoveFileOnSignal for cleanup), writes data to it, then while keeping the file open, opens with with std::ifstream here: https://github.com/llvm/llvm-project/blob/main/clang/lib/CrossTU/CrossTranslationUnit.cpp#L152-L157

If the temporary file is created with the DELETE flag, opening it a second time fails.

So with that in mind, we either need to add another open flag, OF_DeleteAccessButDontTryToSetDeleteDisposition which we only set in TempFile::create, or backtrack to just always passing OF_Delete, letting setDeleteDisposition fail silently for non-local filesystems and either call setDeleteDisposition another time (with a reference parameter indicating whether it succeeded or not) or just guessing with is_local whether it did succeed or not.

Neither of them are really clean...

Or should we unwind the whole design of using OF_Delete and just make TempFile use the same sys::RemoveFileOnSignal design as on unix? That would be to revert https://github.com/llvm/llvm-project/commit/3ecd20430cace7eb9e78ef0a9ff9cc95a377feb9. It doesn't say the reason for switching to this mechanism in the first place, other than "We won't see the temp file no more.". (The other major commit involved, that adjusted the design for handling TempFile on windows, is https://github.com/llvm/llvm-project/commit/881ba104656c40098d4bc90c52613c08136f0fe1 / D48051. I think parts of that can be reverted/simplified in that case then too?)

[mstorsjo]

Changed OF_Delete to only set the delete access mode but not set the delete disposition. This simplifies the decision making, as TempFile::Create can just call setDeleteDisposition on its own and check the return value.

In theory, we could still consider using RemoveFileOnSignal for the case when manually removing the file at the end, but I think that'd complicate the code even further, so the files will be left behind on network drives on crashes.

[akhuang]

lgtm!

[andrewevstyukhin]

In D112376#3089850, @mstorsjo wrote:

Hmm, I think this would work with D111875 then, if I’d update it to remove the check for the specific error code on the return from setDeleteDisposition?

It'll be great! Current diff still doesn't work with the RAM-disk https://sourceforge.net/projects/imdisk-toolkit/:
if (EC == errc::not_supported) { misses errc::function_not_supported.

I've successfully tested locally this approach: if (EC == errc::not_supported || EC == errc::function_not_supported) {

[mstorsjo]

In D111875#3091485, @andrewevstyukhin wrote:

In D112376#3089850, @mstorsjo wrote:

Hmm, I think this would work with D111875 then, if I’d update it to remove the check for the specific error code on the return from setDeleteDisposition?

It'll be great! Current diff still doesn't work with the RAM-disk https://sourceforge.net/projects/imdisk-toolkit/:
if (EC == errc::not_supported) { misses errc::function_not_supported.

I've successfully tested locally this approach: if (EC == errc::not_supported || EC == errc::function_not_supported) {

Thanks for testing! Maybe it'd be best and safest to remove that if statement altogether - as long as we successfully opened the file, we don't need to bother about the various reasons why setDeleteDisposition can fail, as we can just skip that and go on with the manual deletion.

[mstorsjo]

Don't check the exact error code from setDeleteDisposition, just proceed with the alternative codepath if it failed, for whatever reason. This fixes creating temp files on a ram disk (which errors out from realPathFromHandle in setDeleteDisposition).

[mstorsjo]

Just for the record for clarity, in D112376 I considered an alternative approach, to remove the whole use of delete-on-close flags on Windows, to just use the RemoveFileOnSignal function instead. That has the much more visible drawback that temp files are left behind if processes are killed.

I'll go ahead and land this probably tomorrow, if there's nothing more to add here - it seems that this one doesn't have any objections now.

[rnk]

Thanks for working on this, this was quite the rewrite.

llvm/include/llvm/Support/FileSystem.h

enum OpenFlags : unsigned {
   /// The file should be opened in text mode and use a carriage linefeed '\r\n'.
   /// This flag has the same functionality as OF_Text on z/OS but adds a
   /// carriage linefeed on Windows.
   OF_TextWithCRLF = OF_Text | OF_CRLF,
 
   /// The file should be opened in append mode.
   OF_Append = 4,
 
-  /// Delete the file on close. Only makes a difference on windows.
+  /// The returned handle can be used for deleting the file. Only makes a
+  /// difference on windows.
   OF_Delete = 8,
 
   /// When a child process is launched, this file should remain open in the
   /// child process.
   OF_ChildInherit = 16,
 
   /// Force files Atime to be updated on access. Only makes a difference on
   /// Windows.
public:
   TempFile &operator=(TempFile &&Other);
 
   // Name of the temporary file.
   std::string TmpName;
 
   // The open file descriptor.
   int FD = -1;
 
+#ifdef _WIN32
+  // Whether we need to manually remove the file on close.
+  bool RemoveOnClose = false;
+#endif
+
   // Keep this with the given name.
   Error keep(const Twine &Name);
 
   // Keep this with the temporary name.
   Error keep();
 
   // Delete the file.
   Error discard();

llvm/lib/Support/Path.cpp

 TempFile::TempFile(StringRef Name, int FD)
     : TmpName(std::string(Name)), FD(FD) {}
 TempFile::TempFile(TempFile &&Other) { *this = std::move(Other); }
 TempFile &TempFile::operator=(TempFile &&Other) {
   TmpName = std::move(Other.TmpName);
   FD = Other.FD;
   Other.Done = true;
   Other.FD = -1;
+#ifdef _WIN32
+  RemoveOnClose = Other.RemoveOnClose;
+  Other.RemoveOnClose = false;
+#endif
   return *this;
 }
 
 TempFile::~TempFile() { assert(Done); }
 
 Error TempFile::discard() {
   Done = true;
   if (FD != -1 && close(FD) == -1) {
     std::error_code EC = std::error_code(errno, std::generic_category());
     return errorCodeToError(EC);
   }
   FD = -1;
 
 #ifdef _WIN32
-  // On windows closing will remove the file.
-  TmpName = "";
-  return Error::success();
+  // On Windows, closing will remove the file, if we set the delete
+  // disposition. If not, remove it manually.
+  bool Remove = RemoveOnClose;
 #else
-  // Always try to close and remove.
+  // Always try to remove the file.
+  bool Remove = true;
+#endif
   std::error_code RemoveEC;
-  if (!TmpName.empty()) {
+  if (Remove && !TmpName.empty()) {
     RemoveEC = fs::remove(TmpName);
+#ifndef _WIN32
     sys::DontRemoveFileOnSignal(TmpName);
+#endif
     if (!RemoveEC)
       TmpName = "";
+  } else {
+    TmpName = "";
   }
   return errorCodeToError(RemoveEC);
-#endif
 }
 
 Error TempFile::keep(const Twine &Name) {
   assert(!Done);
   Done = true;
   // Always try to close and rename.
 #ifdef _WIN32
   // If we can't cancel the delete don't rename.
   auto H = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
   std::error_code RenameEC = setDeleteDisposition(H, false);
+  bool ShouldDelete = false;
   if (!RenameEC) {
     RenameEC = rename_handle(H, Name);
     // If rename failed because it's cross-device, copy instead
     if (RenameEC ==
       std::error_code(ERROR_NOT_SAME_DEVICE, std::system_category())) {
       RenameEC = copy_file(TmpName, Name);
-      setDeleteDisposition(H, true);
+      ShouldDelete = true;
     }
   }
 
-  // If we can't rename, discard the temporary file.
+  // If we can't rename or copy, discard the temporary file.
   if (RenameEC)
+    ShouldDelete = true;
+  if (ShouldDelete) {
+    if (!RemoveOnClose)
       setDeleteDisposition(H, true);
+    else
+      remove(TmpName);
+  }
 #else
   std::error_code RenameEC = fs::rename(TmpName, Name);
   if (RenameEC) {
     // If we can't rename, try to copy to work around cross-device link issues.
     RenameEC = sys::fs::copy_file(TmpName, Name);
     // If we can't rename or copy, discard the temporary file.
     if (RenameEC)
       remove(TmpName);
Expected<TempFile> TempFile::create(const Twine &Model, unsigned Mode,
                                     OpenFlags ExtraFlags) {
   int FD;
   SmallString<128> ResultPath;
   if (std::error_code EC =
           createUniqueFile(Model, FD, ResultPath, OF_Delete | ExtraFlags, Mode))
     return errorCodeToError(EC);
 
   TempFile Ret(ResultPath, FD);
-#ifndef _WIN32
+#ifdef _WIN32
+  auto H = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
+  if (std::error_code EC = setDeleteDisposition(H, true)) {
+    Ret.RemoveOnClose = true;
+  }
+#else
   if (sys::RemoveFileOnSignal(ResultPath)) {
     // Make sure we delete the file when RemoveFileOnSignal fails.
     consumeError(Ret.discard());
     std::error_code EC(errc::operation_not_permitted);
     return errorCodeToError(EC);
   }
 #endif
   return std::move(Ret);
 }
 } // namespace fs
 
 } // namespace sys
 } // namespace llvm

llvm/lib/Support/Windows/Path.inc

std::error_code is_local(int FD, bool &Result) {
   HANDLE Handle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
 
   if (std::error_code EC = realPathFromHandle(Handle, FinalPath))
     return EC;
 
   return is_local_internal(FinalPath, Result);
 }
 
 static std::error_code setDeleteDisposition(HANDLE Handle, bool Delete) {
   // Clear the FILE_DISPOSITION_INFO flag first, before checking if it's a
   // network file. On Windows 7 the function realPathFromHandle() below fails
   // if the FILE_DISPOSITION_INFO flag was already set to 'DeleteFile = true' by
   // a prior call.
   FILE_DISPOSITION_INFO Disposition;
   Disposition.DeleteFile = false;
   if (!SetFileInformationByHandle(Handle, FileDispositionInfo, &Disposition,
                                   sizeof(Disposition)))
     return mapWindowsError(::GetLastError());
   if (!Delete)
     return std::error_code();
 
   // Check if the file is on a network (non-local) drive. If so, don't
   // continue when DeleteFile is true, since it prevents opening the file for
-  // writes. Note -- this will leak temporary files on disk, but only when the
-  // target file is on a network drive.
+  // writes.
   SmallVector<wchar_t, 128> FinalPath;
   if (std::error_code EC = realPathFromHandle(Handle, FinalPath))
     return EC;
 
   bool IsLocal;
   if (std::error_code EC = is_local_internal(FinalPath, IsLocal))
     return EC;
 
   if (!IsLocal)
-    return std::error_code();
+    return errc::not_supported;
 
   // The file is on a local drive, we can safely set FILE_DISPOSITION_INFO's
   // flag.
   Disposition.DeleteFile = true;
   if (!SetFileInformationByHandle(Handle, FileDispositionInfo, &Disposition,
                                   sizeof(Disposition)))
     return mapWindowsError(::GetLastError());
   return std::error_code();
case CD_OpenExisting:
     return OPEN_EXISTING;
   }
   llvm_unreachable("unreachable!");
 }
 
 static DWORD nativeAccess(FileAccess Access, OpenFlags Flags) {
   DWORD Result = 0;
   if (Access & FA_Read)
     Result |= GENERIC_READ;
   if (Access & FA_Write)
     Result |= GENERIC_WRITE;
   if (Flags & OF_Delete)
     Result |= DELETE;
   if (Flags & OF_UpdateAtime)
     Result |= FILE_WRITE_ATTRIBUTES;
   return Result;
 }
if (Flags & OF_UpdateAtime) {
     GetSystemTime(&SystemTime);
     if (SystemTimeToFileTime(&SystemTime, &FileTime) == 0 ||
         SetFileTime(Result, NULL, &FileTime, NULL) == 0) {
       DWORD LastError = ::GetLastError();
       ::CloseHandle(Result);
       return errorCodeToError(mapWindowsError(LastError));
     }
   }
 
-  if (Flags & OF_Delete) {
-    if ((EC = setDeleteDisposition(Result, true))) {
-      ::CloseHandle(Result);
-      return errorCodeToError(EC);
-    }
-  }
   return Result;
 }
 
 std::error_code openFile(const Twine &Name, int &ResultFD,
                          CreationDisposition Disp, FileAccess Access,
                          OpenFlags Flags, unsigned int Mode) {
   Expected<file_t> Result = openNativeFile(Name, Disp, Access, Flags);
   if (!Result)
     return errorToErrorCode(Result.takeError());