Skip to content

Commit

Permalink
[tf] AtomicRenameUtil - add short retry period on windows
Browse files Browse the repository at this point in the history
On windows, things like virus checkers and indexing service can often grab
a handle to newly-created files; they usually release them fairly quickly,
though, so just need a short retry period

Closes:
#2141
  • Loading branch information
pmolodo committed Aug 14, 2024
1 parent 30392d9 commit 601a06d
Show file tree
Hide file tree
Showing 6 changed files with 565 additions and 12 deletions.
19 changes: 12 additions & 7 deletions pxr/base/arch/fileSystem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1082,14 +1082,9 @@ static int Arch_FileAccessError()
}
}

int ArchFileAccess(const char* path, int mode)
int ArchWindowsFileAccess(const char* path, DWORD accessMask)
{
// Simple existence check is handled specially.
std::wstring wpath{ ArchWindowsUtf8ToUtf16(path) };
if (mode == F_OK) {
return (GetFileAttributesW(wpath.c_str()) != INVALID_FILE_ATTRIBUTES)
? 0 : Arch_FileAccessError();
}

const SECURITY_INFORMATION securityInfo = OWNER_SECURITY_INFORMATION |
GROUP_SECURITY_INFORMATION |
Expand Down Expand Up @@ -1140,7 +1135,6 @@ int ArchFileAccess(const char* path, int mode)
mapping.GenericExecute = FILE_GENERIC_EXECUTE;
mapping.GenericAll = FILE_ALL_ACCESS;

DWORD accessMask = ArchModeToAccess(mode);
MapGenericMask(&accessMask, &mapping);

if (AccessCheck(security,
Expand All @@ -1166,6 +1160,17 @@ int ArchFileAccess(const char* path, int mode)
return result ? 0 : -1;
}

int ArchFileAccess(const char* path, int mode)
{
// Simple existence check is handled specially.
if (mode == F_OK) {
std::wstring wpath{ ArchWindowsUtf8ToUtf16(path) };
return (GetFileAttributesW(wpath.c_str()) != INVALID_FILE_ATTRIBUTES)
? 0 : Arch_FileAccessError();
}
return ArchWindowsFileAccess(path, ArchModeToAccess(mode));
}

// https://msdn.microsoft.com/en-us/library/windows/hardware/ff552012.aspx

#define MAX_REPARSE_DATA_SIZE (16 * 1024)
Expand Down
1 change: 1 addition & 0 deletions pxr/base/arch/fileSystem.h
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ ArchOpenFile(char const* fileName, char const* mode);
#endif

#if defined(ARCH_OS_WINDOWS)
ARCH_API int ArchWindowsFileAccess(const char* path, DWORD accessMask);
ARCH_API int ArchFileAccess(const char* path, int mode);
#else
# define ArchFileAccess(path, mode) access(path, mode)
Expand Down
16 changes: 16 additions & 0 deletions pxr/base/tf/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,15 @@ pxr_build_test_shared_lib(TestTfRegistryFunctionPlugin
testenv/TestTfRegistryFunctionPlugin.cpp
)

if(WIN32)
pxr_build_test(testAtomicRenameUtil
LIBRARIES
tf
CPPFILES
testenv/testAtomicRenameUtil.cpp
)
endif()

pxr_build_test(testTfCast
LIBRARIES
tf
Expand Down Expand Up @@ -495,6 +504,7 @@ pxr_build_test(testTf
)

pxr_test_scripts(
testenv/testTfAtomicRenameUtil.py
testenv/testTfCrashHandler.py
testenv/testTfFileUtils.py
testenv/testTfMallocTagReport.py
Expand Down Expand Up @@ -549,6 +559,12 @@ pxr_register_test(TfAnyUniquePtr
pxr_register_test(TfAtomicOfstreamWrapper
COMMAND "${CMAKE_INSTALL_PREFIX}/tests/testTf TfAtomicOfstreamWrapper"
)
if(WIN32)
pxr_register_test(TfAtomicRenameUtil
PYTHON
COMMAND "${CMAKE_INSTALL_PREFIX}/tests/testTfAtomicRenameUtil ${CMAKE_INSTALL_PREFIX}/tests/testAtomicRenameUtil.exe"
)
endif()
pxr_register_test(TfBitUtils
COMMAND "${CMAKE_INSTALL_PREFIX}/tests/testTf TfBitUtils"
)
Expand Down
84 changes: 79 additions & 5 deletions pxr/base/tf/atomicRenameUtil.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,62 @@
#include "pxr/base/tf/envSetting.h"

#if defined(ARCH_OS_WINDOWS)
#include "pxr/base/tf/envSetting.h"
#include <Windows.h>
#include <algorithm>
#include <io.h>
#include <thread>
#endif

#include <string>
#include <cerrno>

#if defined(ARCH_OS_WINDOWS)
namespace {
PXR_NAMESPACE_USING_DIRECTIVE

bool TryMove(std::wstring const &wsrc, std::wstring const &wdst) {
return MoveFileExW(wsrc.c_str(), wdst.c_str(),
MOVEFILE_REPLACE_EXISTING | MOVEFILE_COPY_ALLOWED) != FALSE;
}

bool HaveMovePermissions(std::string const &src, std::string const &dst) {
// Docs for MovFileExW say:
// To delete or rename a file, you must have either delete permission
// on the file or delete child permission in the parent directory.
// https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-movefileexa

if (ArchWindowsFileAccess(src.c_str(), DELETE) != 0) {
// Don't have delete perms on file, check for FILE_DELETE_CHILD on parent dir
std::string srcParent = TfGetPathName(src);
if (ArchWindowsFileAccess(srcParent.c_str(), FILE_DELETE_CHILD) != 0) {
return false;
}
}
// presumably you need create child permission in the parent directory of dst
std::string dstParent = TfGetPathName(dst);
return ArchWindowsFileAccess(dstParent.c_str(), FILE_ADD_FILE) == 0;
}
}
#endif


PXR_NAMESPACE_OPEN_SCOPE

#if defined(ARCH_OS_WINDOWS)

// On Windows, it's not uncommon for some external process to grab a handle to
// newly created files (ie, Anti-Virus, Windows File Indexing), which can make
// that file inaccessible, and make the move fail. The duration of the lock
// is usually brief, though, so add a short-ish retry period if it's locked.

// By default, we wait ~.3 seconds before giving up
TF_DEFINE_ENV_SETTING(TF_FILE_LOCK_NUM_RETRIES, 15,
"Number of times to retry file renaming if a lock held");

TF_DEFINE_ENV_SETTING(TF_FILE_LOCK_RETRY_WAIT_MS, 20,
"Time in microseconds to wait between retries when lock held on renamed file");

// Older networked filesystems have reported incorrect file permissions
// on Windows so the write permissions check has been disabled as a default
static const bool requireWritePermissionDefault = false;
Expand All @@ -53,16 +99,44 @@ Tf_AtomicRenameFileOver(std::string const &srcFileName,
#if defined(ARCH_OS_WINDOWS)
const std::wstring wsrc{ ArchWindowsUtf8ToUtf16(srcFileName) };
const std::wstring wdst{ ArchWindowsUtf8ToUtf16(dstFileName) };
bool moved = MoveFileExW(wsrc.c_str(),
wdst.c_str(),
MOVEFILE_REPLACE_EXISTING |
MOVEFILE_COPY_ALLOWED) != FALSE;

// On Windows, it's not uncommon for some external process to grab a handle to
// newly created files (ie, Anti-Virus, Windows File Indexing), which can make
// that file inaccessible, and make the move fail. The duration of the lock
// is usually brief, though, so add a short-ish retry period if it's locked.

static const int numRetries = std::max(TfGetEnvSetting(TF_FILE_LOCK_NUM_RETRIES), 0);
static const int waitMS = std::max(TfGetEnvSetting(TF_FILE_LOCK_RETRY_WAIT_MS), 0);

bool moved = false;
DWORD lastError = 0;

for (int i = 0; i <= numRetries; i++) {
moved = TryMove(wsrc, wdst);
if (moved) {
break;
}
lastError = ::GetLastError();
// Only check file perms the first time as an optimization - it's a
// filesystem operation, and possibly slow
if (i == 0 && !HaveMovePermissions(srcFileName, dstFileName)) {
break;
}
if (lastError != ERROR_SHARING_VIOLATION
&& lastError != ERROR_LOCK_VIOLATION
&& lastError != ERROR_ACCESS_DENIED
) {
break;
}
std::this_thread::sleep_for(std::chrono::milliseconds(waitMS));
}

if (!moved) {
*error = TfStringPrintf(
"Failed to rename temporary file '%s' to '%s': %s",
srcFileName.c_str(),
dstFileName.c_str(),
ArchStrSysError(::GetLastError()).c_str());
ArchStrSysError(lastError).c_str());
result = false;
}
#else
Expand Down
140 changes: 140 additions & 0 deletions pxr/base/tf/testenv/testAtomicRenameUtil.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
//
// Copyright 2023 Pixar
//
// Licensed under the Apache License, Version 2.0 (the "Apache License")
// with the following modification; you may not use this file except in
// compliance with the Apache License and the following modification to it:
// Section 6. Trademarks. is deleted and replaced with:
//
// 6. Trademarks. This License does not grant permission to use the trade
// names, trademarks, service marks, or product names of the Licensor
// and its affiliates, except as required to comply with Section 4(c) of
// the License and to reproduce the content of the NOTICE file.
//
// You may obtain a copy of the Apache License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the Apache License with the above modification is
// distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the Apache License for the specific
// language governing permissions and limitations under the Apache License.
//

#include "pxr/pxr.h"
#include "pxr/base/tf/errorMark.h"
#include "pxr/base/tf/fileUtils.h"
#include "pxr/base/tf/ostreamMethods.h"
#include "pxr/base/tf/pathUtils.h"
#include "pxr/base/tf/safeOutputFile.h"
#include "pxr/base/tf/stackTrace.h"
#include "pxr/base/tf/stringUtils.h"

#include <algorithm>
#include <chrono>
#include <iostream>
#include <fstream>
#include <thread>
#include <vector>

using std::string;

PXR_NAMESPACE_USING_DIRECTIVE

namespace {
constexpr int WRONG_NUMBER_ARGS = 2;
constexpr float MAX_WAIT_FOR_FILE_SECONDS = 10;
constexpr float WAIT_FOR_SLEEP_SECONDS = .1;
constexpr auto FINAL_EXT = ".final";

// We're testing using TfSafeOutputFile::Replace, which first writes to
// a temporary file, then moves it to the final file.
// In our tests, our final files end with a ".final" filename suffix.
// This checks for the existence of the temp files, by finding matches
// that DON'T have the ".final" suffix
static size_t
Tf_CountTempFileMatches(const string& pattern)
{
std::vector<string> matches = TfGlob(pattern, 0);

// Count matches that don't end with ".final"
return std::count_if(
matches.begin(), matches.end(),
[](const string& x) {
return !TfStringEndsWith(x, FINAL_EXT);
}
);
}

} // end annonymous namespace

/// Tries to run a TfSafeOutputFile::Replace
///
/// If a non-empty waitForFile is provided, then it will pause after the temp files
/// are created, but before the file move is made, until the waitForFile exists.
///
/// This provides a means of communication for our external testing program, so
/// it can run arbitrary code at this point, then create the waitForFile to
/// signal that this process should proceed with the file move.
static void
RunSafeOutputFileReplace(const string& fileBaseName, const string& waitForFile)
{
// We want to test Tf_AtomicRenameFileOver, but that's not exposed publicly, so we test
// TfSafeOutputFile::Replace, which uses it

TfErrorMark tfErrors;

string fileFinalName = fileBaseName + FINAL_EXT;
string fileTempPattern = fileBaseName + ".*";
auto outf = TfSafeOutputFile::Replace(fileFinalName);
TF_AXIOM(outf.Get());
TF_AXIOM(tfErrors.IsClean());

// Temporary file exists.
TF_AXIOM(Tf_CountTempFileMatches(fileTempPattern) == 1);

// Write content to the stream.
fprintf(outf.Get(), "New Content\n");

// If a waitForFile was given, pause until that file exists
if (!waitForFile.empty()) {
auto start = std::chrono::steady_clock::now();
auto maxTime = start + std::chrono::duration<double>(MAX_WAIT_FOR_FILE_SECONDS);
auto sleepTime = std::chrono::duration<double>(WAIT_FOR_SLEEP_SECONDS);
while(!TfPathExists(waitForFile)) {
TF_AXIOM(std::chrono::steady_clock::now() < maxTime);
std::this_thread::sleep_for(sleepTime);
}
}

// Commit.
outf.Close();
TF_AXIOM(!outf.Get());
TF_AXIOM(tfErrors.IsClean());

// Temporary file is gone.
TF_AXIOM(Tf_CountTempFileMatches(fileTempPattern) == 0);

// Verify destination file content.
std::ifstream ifs(fileFinalName);
string newContent;
getline(ifs, newContent);
TF_AXIOM(newContent == "New Content");
}

int
main(int argc, char *argv[])
{
if (argc < 2) {
string progName(argv[0]);
std::cerr << "Usage: " << progName << " FILE_BASE_NAME [WAIT_FOR_FILE]" << std::endl;
return WRONG_NUMBER_ARGS;
}
string waitForFile;
if (argc > 2) {
waitForFile = argv[2];
}
RunSafeOutputFileReplace(argv[1], waitForFile);
return 0;
}
Loading

0 comments on commit 601a06d

Please sign in to comment.