Skip to content

Commit 347fe44

Browse files
BillyONealBillyONeal
authored
Improve resiliency of concurrent use the downloads directory. (#1600)
* Add filesystem::rename_cas_like * Add comment to configure-environment describing that updates are racy, avoid getting exe path 3 times. * Use rename_cas_like in tools, and reuse extract_archive_to_temp_subdirectory in configure-environment. Allows outright deletion of set_directory_to_archive_contents. * Use rename_cas_like for git registry manipulation too. * format * rename_cas_like -> rename_or_delete * Add tags to filesystem tests to avoid accidental test-to-test interaction
1 parent bec4296 commit 347fe44

File tree

8 files changed

+172
-44
lines changed

8 files changed

+172
-44
lines changed

‎include/vcpkg/archives.h

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -36,12 +36,7 @@ namespace vcpkg
3636
MessageSink& status_sink,
3737
const Path& archive,
3838
const Path& to_path);
39-
// set `to_path` to `archive` contents.
40-
void set_directory_to_archive_contents(const Filesystem& fs,
41-
const ToolCache& tools,
42-
MessageSink& status_sink,
43-
const Path& archive,
44-
const Path& to_path);
39+
// extract `archive` to a sibling temporary subdirectory of `to_path` and returns that path
4540
Path extract_archive_to_temp_subdirectory(const Filesystem& fs,
4641
const ToolCache& tools,
4742
MessageSink& status_sink,

‎include/vcpkg/base/files.h

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,17 @@ namespace vcpkg
246246
void rename_with_retry(const Path& old_path, const Path& new_path, std::error_code& ec) const;
247247
void rename_with_retry(const Path& old_path, const Path& new_path, LineInfo li) const;
248248

249+
// Rename old_path -> new_path, but consider new_path already existing as acceptable.
250+
// Traditionally used to interact with downloads, or git tree cache, where multiple
251+
// instances of vcpkg may be trying to do the same action at the same time.
252+
//
253+
// Returns whether the rename actually happened. Note that `rename` has 'replace if exists' behavior for files
254+
// but not directories, so if old_path and new_path are files, this function always returns true.
255+
//
256+
// If `old_path` and `new_path` resolve to the same file, the behavior is undefined.
257+
bool rename_or_delete(const Path& old_path, const Path& new_path, std::error_code& ec) const;
258+
bool rename_or_delete(const Path& old_path, const Path& new_path, LineInfo li) const;
259+
249260
virtual void rename_or_copy(const Path& old_path,
250261
const Path& new_path,
251262
StringLiteral temp_suffix,

‎src/vcpkg-test/files.cpp

Lines changed: 106 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,10 @@ namespace
2929
{
3030
using urbg_t = std::mt19937_64;
3131

32-
std::string get_random_filename(urbg_t& urbg) { return Strings::b32_encode(urbg()); }
32+
std::string get_random_filename(urbg_t& urbg, StringLiteral tag)
33+
{
34+
return Strings::b32_encode(urbg()).append(tag.data(), tag.size());
35+
}
3336

3437
bool is_valid_symlink_failure(const std::error_code& ec) noexcept
3538
{
@@ -131,7 +134,7 @@ namespace
131134
CHECK_EC_ON_FILE(base, ec);
132135
for (unsigned int i = 0; i < 5; ++i)
133136
{
134-
create_directory_tree(urbg, fs, base / get_random_filename(urbg), remaining_depth - 1);
137+
create_directory_tree(urbg, fs, base / get_random_filename(urbg, "_tree"), remaining_depth - 1);
135138
}
136139

137140
#if !defined(_WIN32)
@@ -177,7 +180,7 @@ namespace
177180

178181
auto& fs = setup();
179182

180-
auto temp_dir = base_temporary_directory() / get_random_filename(urbg);
183+
auto temp_dir = base_temporary_directory() / get_random_filename(urbg, "_enum");
181184
INFO("temp dir is: " << temp_dir.native());
182185

183186
const auto target_root = temp_dir / "target";
@@ -619,7 +622,7 @@ TEST_CASE ("remove readonly", "[files]")
619622

620623
auto& fs = setup();
621624

622-
auto temp_dir = base_temporary_directory() / get_random_filename(urbg);
625+
auto temp_dir = base_temporary_directory() / get_random_filename(urbg, "_remove_readonly");
623626
INFO("temp dir is: " << temp_dir.native());
624627

625628
fs.create_directory(temp_dir, VCPKG_LINE_INFO);
@@ -672,7 +675,7 @@ TEST_CASE ("remove all", "[files]")
672675

673676
auto& fs = setup();
674677

675-
auto temp_dir = base_temporary_directory() / get_random_filename(urbg);
678+
auto temp_dir = base_temporary_directory() / get_random_filename(urbg, "_remove_all");
676679
INFO("temp dir is: " << temp_dir.native());
677680

678681
create_directory_tree(urbg, fs, temp_dir);
@@ -692,7 +695,7 @@ TEST_CASE ("remove all symlinks", "[files]")
692695

693696
auto& fs = setup();
694697

695-
auto temp_dir = base_temporary_directory() / get_random_filename(urbg);
698+
auto temp_dir = base_temporary_directory() / get_random_filename(urbg, "_remove_all_symlinks");
696699
INFO("temp dir is: " << temp_dir.native());
697700

698701
const auto target_root = temp_dir / "target";
@@ -840,7 +843,7 @@ TEST_CASE ("copy_file", "[files]")
840843

841844
auto& fs = setup();
842845

843-
auto temp_dir = base_temporary_directory() / get_random_filename(urbg);
846+
auto temp_dir = base_temporary_directory() / get_random_filename(urbg, "_copy_file");
844847
INFO("temp dir is: " << temp_dir.native());
845848

846849
fs.create_directory(temp_dir, VCPKG_LINE_INFO);
@@ -914,13 +917,107 @@ TEST_CASE ("copy_file", "[files]")
914917
CHECK_EC_ON_FILE(temp_dir, ec);
915918
}
916919

920+
TEST_CASE ("rename", "[files]")
921+
{
922+
urbg_t urbg;
923+
924+
auto& fs = setup();
925+
926+
auto temp_dir = base_temporary_directory() / get_random_filename(urbg, "_rename");
927+
INFO("temp dir is: " << temp_dir.native());
928+
929+
static constexpr StringLiteral FileTxt = "file.txt";
930+
fs.remove_all(temp_dir, VCPKG_LINE_INFO);
931+
fs.create_directory(temp_dir, VCPKG_LINE_INFO);
932+
auto temp_dir_a = temp_dir / "a";
933+
fs.create_directory(temp_dir_a, VCPKG_LINE_INFO);
934+
auto temp_dir_a_file = temp_dir_a / FileTxt;
935+
auto temp_dir_b = temp_dir / "b";
936+
auto temp_dir_b_file = temp_dir_b / FileTxt;
937+
938+
static constexpr StringLiteral text_file_contents = "hello there";
939+
fs.write_contents(temp_dir_a_file, text_file_contents, VCPKG_LINE_INFO);
940+
941+
// try rename_with_retry
942+
{
943+
fs.rename_with_retry(temp_dir_a, temp_dir_b, VCPKG_LINE_INFO);
944+
REQUIRE(!fs.exists(temp_dir_a, VCPKG_LINE_INFO));
945+
REQUIRE(fs.read_contents(temp_dir_b_file, VCPKG_LINE_INFO) == text_file_contents);
946+
947+
// put things back
948+
fs.rename(temp_dir_b, temp_dir_a, VCPKG_LINE_INFO);
949+
REQUIRE(fs.read_contents(temp_dir_a_file, VCPKG_LINE_INFO) == text_file_contents);
950+
REQUIRE(!fs.exists(temp_dir_b, VCPKG_LINE_INFO));
951+
}
952+
953+
// try rename_or_delete directory, target does not exist
954+
{
955+
REQUIRE(fs.rename_or_delete(temp_dir_a, temp_dir_b, VCPKG_LINE_INFO));
956+
REQUIRE(!fs.exists(temp_dir_a, VCPKG_LINE_INFO));
957+
REQUIRE(fs.read_contents(temp_dir_b_file, VCPKG_LINE_INFO) == text_file_contents);
958+
959+
// put things back
960+
fs.rename(temp_dir_b, temp_dir_a, VCPKG_LINE_INFO);
961+
REQUIRE(fs.read_contents(temp_dir_a_file, VCPKG_LINE_INFO) == text_file_contents);
962+
REQUIRE(!fs.exists(temp_dir_b, VCPKG_LINE_INFO));
963+
}
964+
965+
// try rename_or_delete directory, target exists
966+
{
967+
fs.create_directory(temp_dir_b, VCPKG_LINE_INFO);
968+
fs.write_contents(temp_dir_b_file, text_file_contents, VCPKG_LINE_INFO);
969+
970+
// Note that the VCPKG_LINE_INFO overload implicitly tests that ec got cleared
971+
REQUIRE(!fs.rename_or_delete(temp_dir_a, temp_dir_b, VCPKG_LINE_INFO));
972+
REQUIRE(!fs.exists(temp_dir_a, VCPKG_LINE_INFO));
973+
REQUIRE(fs.read_contents(temp_dir_b_file, VCPKG_LINE_INFO) == text_file_contents);
974+
975+
// put things back
976+
fs.rename(temp_dir_b, temp_dir_a, VCPKG_LINE_INFO);
977+
REQUIRE(fs.read_contents(temp_dir_a_file, VCPKG_LINE_INFO) == text_file_contents);
978+
REQUIRE(!fs.exists(temp_dir_b, VCPKG_LINE_INFO));
979+
}
980+
981+
// try rename_or_delete file, target does not exist
982+
{
983+
fs.create_directory(temp_dir_b, VCPKG_LINE_INFO);
984+
REQUIRE(fs.rename_or_delete(temp_dir_a_file, temp_dir_b_file, VCPKG_LINE_INFO));
985+
REQUIRE(!fs.exists(temp_dir_a_file, VCPKG_LINE_INFO));
986+
REQUIRE(fs.read_contents(temp_dir_b_file, VCPKG_LINE_INFO) == text_file_contents);
987+
988+
// put things back
989+
fs.rename(temp_dir_b_file, temp_dir_a_file, VCPKG_LINE_INFO);
990+
REQUIRE(fs.read_contents(temp_dir_a_file, VCPKG_LINE_INFO) == text_file_contents);
991+
REQUIRE(!fs.exists(temp_dir_b_file, VCPKG_LINE_INFO));
992+
fs.remove(temp_dir_b, VCPKG_LINE_INFO);
993+
}
994+
995+
// try rename_or_delete file, target exists
996+
{
997+
fs.create_directory(temp_dir_b, VCPKG_LINE_INFO);
998+
fs.write_contents(temp_dir_b_file, text_file_contents, VCPKG_LINE_INFO);
999+
// Note that the VCPKG_LINE_INFO overload implicitly tests that ec got cleared
1000+
// Also note that POSIX rename() will just delete the target like we want by itself so
1001+
// this returns true.
1002+
REQUIRE(fs.rename_or_delete(temp_dir_a_file, temp_dir_b_file, VCPKG_LINE_INFO));
1003+
REQUIRE(!fs.exists(temp_dir_a_file, VCPKG_LINE_INFO));
1004+
REQUIRE(fs.read_contents(temp_dir_b_file, VCPKG_LINE_INFO) == text_file_contents);
1005+
1006+
// put things back
1007+
fs.rename(temp_dir_b_file, temp_dir_a_file, VCPKG_LINE_INFO);
1008+
REQUIRE(fs.read_contents(temp_dir_a_file, VCPKG_LINE_INFO) == text_file_contents);
1009+
REQUIRE(!fs.exists(temp_dir_b_file, VCPKG_LINE_INFO));
1010+
fs.remove(temp_dir_b, VCPKG_LINE_INFO);
1011+
}
1012+
}
1013+
9171014
TEST_CASE ("copy_symlink", "[files]")
9181015
{
9191016
urbg_t urbg;
9201017

9211018
auto& fs = setup();
9221019

923-
auto temp_dir = base_temporary_directory() / get_random_filename(urbg);
1020+
auto temp_dir = base_temporary_directory() / get_random_filename(urbg, "_copy_symlink");
9241021
INFO("temp dir is: " << temp_dir.native());
9251022

9261023
fs.create_directory(temp_dir, VCPKG_LINE_INFO);
@@ -1080,7 +1177,7 @@ TEST_CASE ("remove all -- benchmarks", "[files][!benchmark]")
10801177
temp_dirs.resize(meter.runs());
10811178

10821179
std::generate(begin(temp_dirs), end(temp_dirs), [&] {
1083-
Path temp_dir = base_temporary_directory() / get_random_filename(urbg);
1180+
Path temp_dir = base_temporary_directory() / get_random_filename(urbg, "_remove_all_bench");
10841181
create_directory_tree(urbg, fs, temp_dir, max_depth);
10851182
return temp_dir;
10861183
});

‎src/vcpkg/archives.cpp

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -310,18 +310,6 @@ namespace vcpkg
310310
msg::path = archive);
311311
}
312312

313-
void set_directory_to_archive_contents(const Filesystem& fs,
314-
const ToolCache& tools,
315-
MessageSink& status_sink,
316-
const Path& archive,
317-
const Path& to_path)
318-
319-
{
320-
fs.remove_all(to_path, VCPKG_LINE_INFO);
321-
Path to_path_partial = extract_archive_to_temp_subdirectory(fs, tools, status_sink, archive, to_path);
322-
fs.rename_with_retry(to_path_partial, to_path, VCPKG_LINE_INFO);
323-
}
324-
325313
bool ZipTool::compress_directory_to_zip(DiagnosticContext& context,
326314
const Filesystem& fs,
327315
const Path& source,

‎src/vcpkg/base/files.cpp

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2008,6 +2008,44 @@ namespace vcpkg
20082008
}
20092009
}
20102010

2011+
bool Filesystem::rename_or_delete(const Path& old_path, const Path& new_path, LineInfo li) const
2012+
{
2013+
std::error_code ec;
2014+
bool result = this->rename_or_delete(old_path, new_path, ec);
2015+
if (ec)
2016+
{
2017+
exit_filesystem_call_error(li, ec, __func__, {old_path, new_path});
2018+
}
2019+
2020+
return result;
2021+
}
2022+
2023+
bool Filesystem::rename_or_delete(const Path& old_path, const Path& new_path, std::error_code& ec) const
2024+
{
2025+
this->rename(old_path, new_path, ec);
2026+
using namespace std::chrono_literals;
2027+
for (const auto& delay : {10ms, 100ms, 1000ms, 10000ms})
2028+
{
2029+
if (!ec)
2030+
{
2031+
return true;
2032+
}
2033+
else if (ec == std::make_error_condition(std::errc::directory_not_empty) ||
2034+
ec == std::make_error_condition(std::errc::file_exists) || this->exists(new_path, ec))
2035+
{
2036+
// either the rename failed with a target already exists error, or the target explicitly exists,
2037+
// assume another process 'won' the 'CAS'.
2038+
this->remove_all(old_path, ec);
2039+
return false;
2040+
}
2041+
2042+
std::this_thread::sleep_for(delay);
2043+
this->rename(old_path, new_path, ec);
2044+
}
2045+
2046+
return false;
2047+
}
2048+
20112049
bool Filesystem::remove(const Path& target, LineInfo li) const
20122050
{
20132051
std::error_code ec;

‎src/vcpkg/configure-environment.cpp

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,8 @@ namespace vcpkg
155155
auto& fs = paths.get_filesystem();
156156

157157
// if artifacts is deployed in development, with Visual Studio, or with the One Liner, it will be deployed here
158-
Path vcpkg_artifacts_path = get_exe_path_of_current_process();
158+
const Path exe_path = get_exe_path_of_current_process();
159+
Path vcpkg_artifacts_path = exe_path;
159160
vcpkg_artifacts_path.replace_filename("vcpkg-artifacts");
160161
vcpkg_artifacts_path.make_preferred();
161162
Path vcpkg_artifacts_main_path = vcpkg_artifacts_path / "main.js";
@@ -177,9 +178,9 @@ namespace vcpkg
177178
#endif // ^^^ !VCPKG_STANDALONE_BUNDLE_SHA
178179
if (out_of_date)
179180
{
180-
fs.remove_all(vcpkg_artifacts_path, VCPKG_LINE_INFO);
181-
auto temp = get_exe_path_of_current_process();
182-
temp.replace_filename("vcpkg-artifacts-temp");
181+
// This is racy; the reason for the temp-path-then-rename dance is to get only the
182+
// 'vcpkg-artifacts' directory out of the standalone bundle, while extracting
183+
// the standalone bundle over VCPKG_ROOT would overwrite scripts/triplets/etc.
183184
auto maybe_tarball = download_vcpkg_standalone_bundle(
184185
console_diagnostic_context, paths.get_asset_cache_settings(), fs, paths.downloads);
185186
auto tarball = maybe_tarball.get();
@@ -188,10 +189,12 @@ namespace vcpkg
188189
Checks::exit_fail(VCPKG_LINE_INFO);
189190
}
190191

191-
set_directory_to_archive_contents(fs, paths.get_tool_cache(), null_sink, *tarball, temp);
192+
Path temp = extract_archive_to_temp_subdirectory(
193+
fs, paths.get_tool_cache(), null_sink, *tarball, vcpkg_artifacts_path);
194+
fs.remove_all(vcpkg_artifacts_path, VCPKG_LINE_INFO);
192195
fs.rename_with_retry(temp / "vcpkg-artifacts", vcpkg_artifacts_path, VCPKG_LINE_INFO);
193-
fs.remove(*tarball, VCPKG_LINE_INFO);
194196
fs.remove_all(temp, VCPKG_LINE_INFO);
197+
fs.remove(*tarball, VCPKG_LINE_INFO);
195198
#if defined(VCPKG_STANDALONE_BUNDLE_SHA)
196199
fs.write_contents(vcpkg_artifacts_version_path, VCPKG_BASE_VERSION_AS_STRING, VCPKG_LINE_INFO);
197200
#endif // ^^^ VCPKG_STANDALONE_BUNDLE_SHA
@@ -233,7 +236,7 @@ namespace vcpkg
233236
}
234237

235238
cmd.string_arg("--vcpkg-root").string_arg(paths.root);
236-
cmd.string_arg("--z-vcpkg-command").string_arg(get_exe_path_of_current_process());
239+
cmd.string_arg("--z-vcpkg-command").string_arg(exe_path);
237240

238241
cmd.string_arg("--z-vcpkg-artifacts-root").string_arg(paths.artifacts());
239242
cmd.string_arg("--z-vcpkg-downloads").string_arg(paths.downloads);

‎src/vcpkg/tools.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -871,12 +871,14 @@ namespace vcpkg
871871
if (tool_data.is_archive)
872872
{
873873
status_sink.println(Color::none, msgExtractingTool, msg::tool_name = tool_data.name);
874-
set_directory_to_archive_contents(fs, *this, status_sink, download_path, tool_dir_path);
874+
Path to_path_partial =
875+
extract_archive_to_temp_subdirectory(fs, *this, status_sink, download_path, tool_dir_path);
876+
fs.rename_or_delete(to_path_partial, tool_dir_path, IgnoreErrors{});
875877
}
876878
else
877879
{
878880
fs.create_directories(exe_path.parent_path(), IgnoreErrors{});
879-
fs.rename(download_path, exe_path, IgnoreErrors{});
881+
fs.rename_or_delete(download_path, exe_path, IgnoreErrors{});
880882
}
881883

882884
if (!fs.exists(exe_path, IgnoreErrors{}))

‎src/vcpkg/vcpkgpaths.cpp

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1199,12 +1199,6 @@ namespace vcpkg
11991199
return format_filesystem_call_error(ec, "create_directory", {git_tree_temp});
12001200
}
12011201

1202-
fs.remove_all(destination, ec);
1203-
if (ec)
1204-
{
1205-
return format_filesystem_call_error(ec, "remove_all", {destination});
1206-
}
1207-
12081202
auto git_archive = git_cmd_builder(dot_git_dir, git_tree_temp)
12091203
.string_arg("read-tree")
12101204
.string_arg("-m")
@@ -1232,11 +1226,11 @@ namespace vcpkg
12321226
return error;
12331227
}
12341228

1235-
fs.rename_with_retry(git_tree_temp, destination, ec);
1229+
fs.rename_or_delete(git_tree_temp, destination, ec);
12361230
if (ec)
12371231
{
12381232
return error_prefix().append(
1239-
format_filesystem_call_error(ec, "rename_with_retry", {git_tree_temp, destination}));
1233+
format_filesystem_call_error(ec, "rename_or_delete", {git_tree_temp, destination}));
12401234
}
12411235

12421236
fs.remove(git_tree_index, IgnoreErrors{});

0 commit comments

Comments
 (0)