[PATCH] D40401: Use FILE_FLAG_DELETE_ON_CLOSE for TempFile on windows

[Reid Kleckner via Phabricator via llvm-commits]

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.

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?

I think it simplifies the code to use separate calls to set the file disposition, if that's possible. Then we don't need to change the user FD and handle all as many error edge cases.


================
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. :)


================
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.


https://reviews.llvm.org/D40401