[PATCH] D40401: Use FILE_FLAG_DELETE_ON_CLOSE for TempFile on windows

[Rafael Avila de Espindola via llvm-commits]

Reid Kleckner via Phabricator <reviews at reviews.llvm.org> writes:

> rnk added inline comments.
>
>
> ================
> Comment at: lib/Support/Windows/Path.inc:403-405
> +/// * It cannot be used. An attempt to open or rename the file will fail.
> +/// * It is not atomic. A crash just after CreateFileW will leave the file on
> +/// disk.
> ----------------
> The atomicity concern didn't bother me that much. There are two windows where the temp file may remain: when the process dies between CreateFileW and the SetFileInformationByHandle Delete=true call, and when the process dies between SetFileInformationByHandle Delete=false and the file rename. It's highly unlikely that we will crash in either of these places. There would have to be an external source of raciness (a second thread or external process killing the process) to cause this bug. It doesn't seem like a show-stopper. The FILE_FLAG_DELETE_ON_CLOSE solution doesn't close the second window.

Correct. It is pretty minor given that the existing solution has exactly
the same problem.

> Can you elaborate on the usability problem, since it's more important? If we tolerate the raciness above, not being able to rename a temporary file isn't such a big deal as long as the TempFile abstraction turns off the delete-on-close disposition before renaming. Do we have uses of TempFile that want to be able to pass the path to some other open() call?

The main difference is that with FILE_FLAG_DELETE_ON_CLOSE the only
restriction is that any new uses must have FILE_SHARE_DELETE.

With DeleteFile=true disposition *everything* fails afterwards. The
documentation on SetFileInformationByHandle is sparse, but I assume that
is how DeleteFile is implemented and it says:

---------------------------------------------------------------
The DeleteFile function marks a file for deletion on close. Therefore,
the file deletion does not occur until the last handle to the file is
closed. Subsequent calls to CreateFile to open the file fail with
ERROR_ACCESS_DENIED.
---------------------------------------------------------------

So it is not just rename. Everything trying to open the file would have
to first change the disposition.

Using unix terminology: It seems FILE_FLAG_DELETE_ON_CLOSE is a property
of the file descriptor. When that descriptor is closed, the DeleteFile
disposition is set on the inode. With that set, not new descriptors to
that inode are allowed.

> ================
> Comment at: lib/Support/Windows/Path.inc:407-411
> +/// Using FILE_FLAG_DELETE_ON_CLOSE solves both issues, but there is no way to
> +/// cancel it in place. What works is to create a second handle to prevent the
> +/// deletion, close the first one and then clear DeleteFile with
> +/// SetFileInformationByHandle. This requires changing the handle and file
> +/// descriptor the caller uses.
> ----------------
> Is this something you figured out through experimentation or is there some reference, documentation, stack overflow post, etc that explains this technique? It's an awfully convoluted way to get the obviously desirable behavior. :)

I figured it out when I realized that DeleteFile is probably just a
CreateFile + SetFileInformationByHandle. I will probably write a small
blog post about it since it is it is pretty crazy, but nice that it is
at least possible (if not fully atomic) to do it on win32 apis but not
on posix.

> ================
> Comment at: lib/Support/Windows/Path.inc:423-424
> +    return mapWindowsError(::GetLastError());
> +  if (!CloseHandle(Handle))
> +    return mapWindowsError(::GetLastError());
> +
> ----------------
> I think we should call `::close(FD)` here instead to avoid leaking the FD table entry in the CRT. Stack Overflow suggests that there is no way to close an FD without calling CloseHandle on the underlying handle. I suspect you can still use GetLastError, but I can't find any docs to confirm that.

Will do.

Thanks,
Rafael