Page MenuHomePhabricator

[Support] Fix TempFile::discard to not leave behind temporary files
ClosedPublic

Authored by andrewng on Feb 8 2019, 9:42 AM.

Details

Summary

Moved the remove of the temporary file to after the close to avoid
remove failures caused by ETXTBSY errors.

This issue was seen when FileOutputBuffer falls back to an in memory
buffer due to the inability to mmap the on disk file. This occurred when
running LLD on an Ubuntu VM in VirtualBox on a Windows host attempting
to write the output to a VirtualBox shared folder.

Diff Detail

Repository
rL LLVM

Event Timeline

andrewng created this revision.Feb 8 2019, 9:42 AM
ruiu added a comment.Feb 8 2019, 9:56 AM

This class should do a minimum number of attempt to remove a file, instead of trying to remove a file at various points to increase the chance to successfully removing the file. Why don't you close the file before removing the file?

This class should do a minimum number of attempt to remove a file, instead of trying to remove a file at various points to increase the chance to successfully removing the file. Why don't you close the file before removing the file?

Actually, that was my first approach to fix this issue; to move the remove after the close. However, I then recalled that doing the file remove (unlink) before the close is actually the "normal" approach for POSIX (perhaps because it's more "atomic" in behaviour), so I changed to this implementation instead. But if moving the remove after the close is acceptable, I'm happy for that approach.

ruiu added a comment.Feb 8 2019, 11:34 AM

I think that unlinking a file while opening a file is a common way to create an unnamed temporary file. By doing that, you don't have to worry about leaving temporary files behind. However, if your temporary file can be permanent by renaming it to a permanent filename, that method is irrelevant because you can't unlink a file (if you do, your file become unnamed, and there's no POSIX-compliant way to revive unnamed file as a named one AFAIK.)

What we are trying to do here is the latter, so I think the order of removing a file and closing a file descriptor doesn't matter.

andrewng updated this revision to Diff 186204.Feb 11 2019, 2:42 AM
andrewng edited the summary of this revision. (Show Details)

As suggested, I have switched back to what was my original change to fix this issue.

ruiu added inline comments.Feb 11 2019, 10:56 AM
lib/Support/Path.cpp
1137 ↗(On Diff #186204)

Now the rest of the code is virtuall a no-op on Windows, so you can wrap it entirely with an ifdef like this, which should improve readability.

#ifdef _WIN32
  // On windows closing will remove the file.
  TmpName = ""
  return Error::success();
#else
  std::error_code RemoveEC;
    // Always try to close and remove.
  if (!TmpName.empty()) {
    RemoveEC = fs::remove(TmpName);
    sys::DontRemoveFileOnSignal(TmpName);
  }
  if (!RemoveEC)
    TmpName = "";
  return errorCodeToError(RemoveEC);
#endif
andrewng updated this revision to Diff 186437.Feb 12 2019, 4:01 AM

Updated the patch based on review suggestion.

andrewng marked an inline comment as done.Feb 12 2019, 4:02 AM
ruiu accepted this revision.Feb 12 2019, 9:31 AM

LGTM

This revision is now accepted and ready to land.Feb 12 2019, 9:31 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2019, 3:08 AM
Herald added a subscriber: kristina. · View Herald Transcript