Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Binary cache: async push_success #908

Draft
wants to merge 55 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
55 commits
Select commit Hold shift + click to select a range
95f0438
Binary cache: async push_success
autoantwort Feb 15, 2023
9d999d8
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Feb 28, 2023
163d9cd
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Mar 2, 2023
2a54205
Apply suggestions from code review
autoantwort Mar 2, 2023
0912655
Adapt code review
autoantwort Mar 2, 2023
5d7288c
Update src/vcpkg/binarycaching.cpp
autoantwort Mar 2, 2023
10189ac
Adapt code review
autoantwort Mar 2, 2023
2567607
Remove unnecessary actions_to_push_notifier.notify_all()
autoantwort Mar 2, 2023
ecdd000
Prevent deadlock and don't be on the crtl+c path
autoantwort Mar 2, 2023
8e7ae61
Add and use BGMessageSink to print IBinaryProvider::push_success mess…
autoantwort Mar 3, 2023
850d7c9
Restore old upload message
autoantwort Mar 3, 2023
548be38
Don't join yourself
autoantwort Mar 4, 2023
6dbbf06
Print messages about remaining packages to upload
autoantwort Mar 4, 2023
74b86fd
Localization
autoantwort Mar 5, 2023
5171d3e
Improve messages
autoantwort Mar 5, 2023
d69ed8f
No singleton and explicit calls to wait_for_async_complete()
autoantwort Mar 5, 2023
2df42d5
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Mar 8, 2023
5f1786e
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Mar 10, 2023
93303c3
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Mar 16, 2023
8a26c8b
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Mar 19, 2023
aa7e52f
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Mar 22, 2023
d46a4d6
Apply code review
autoantwort Mar 22, 2023
5e51718
Trigger Build
autoantwort Mar 22, 2023
a9ac558
No rename dance
autoantwort Mar 22, 2023
4faf674
Print upload to provider only once and not once per provider
autoantwort Mar 22, 2023
b9be8c6
Fix tests
autoantwort Mar 22, 2023
78ca081
Don't create unnecessary strings
autoantwort Mar 31, 2023
579bfa9
Rename to m_published_lock
autoantwort Mar 31, 2023
103968e
BinaryPackageInformation use Optional and make BinaryProviderPushRequ…
autoantwort Mar 31, 2023
dd32416
Merge branch 'main' into feature/async-binary-cache-push-success and …
autoantwort May 31, 2023
b666f94
Add missing files
autoantwort May 31, 2023
15bb503
Add missing includes
autoantwort May 31, 2023
d995bfd
Make BianryCache a unique_ptr
autoantwort May 31, 2023
24cd026
Reduce changes
autoantwort May 31, 2023
92fc76b
Fix output
autoantwort May 31, 2023
3527227
Fix bug
autoantwort May 31, 2023
48305b3
Format
autoantwort May 31, 2023
27fa076
Use lock_guard
autoantwort May 31, 2023
bcd459a
Revert "Use lock_guard"
autoantwort May 31, 2023
f958d36
Use enum
autoantwort May 31, 2023
7a24007
BGMessageSink::print_published apply code review
autoantwort May 31, 2023
50114f9
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Jun 14, 2023
ca5f2b1
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Aug 19, 2023
eccd9ee
Fix typo
autoantwort Aug 24, 2023
e7837e0
Fix typo in file name
autoantwort Aug 24, 2023
969e7fc
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Aug 24, 2023
2d5586f
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Oct 11, 2023
809d0b6
Renamings
autoantwort Oct 11, 2023
455e29b
format
autoantwort Oct 12, 2023
03fdfea
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Nov 4, 2023
f4bad8c
BinaryCache and std::unique_ptr
autoantwort Nov 4, 2023
26bbbd5
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Nov 12, 2023
814e434
BinaryCache: save data in std::unique_ptr so that the object can be m…
autoantwort Nov 14, 2023
290e586
fix
autoantwort Nov 14, 2023
3cc3378
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Dec 13, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 43 additions & 4 deletions include/vcpkg/binarycaching.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,14 @@
#include <vcpkg/base/files.h>

#include <vcpkg/packagespec.h>
#include <vcpkg/sourceparagraph.h>

#include <condition_variable>
#include <iterator>
#include <queue>
#include <set>
#include <string>
#include <thread>
#include <unordered_map>
#include <vector>

Expand Down Expand Up @@ -42,6 +46,21 @@ namespace vcpkg
const IBinaryProvider* m_available_provider = nullptr; // meaningful iff m_status == available
};

struct BinaryPackageInformation
ras0219-msft marked this conversation as resolved.
Show resolved Hide resolved
{
explicit BinaryPackageInformation(const InstallPlanAction& action);
std::string package_abi;
// The following fields are only needed for the nuget binary cache
PackageSpec spec;
autoantwort marked this conversation as resolved.
Show resolved Hide resolved
SourceControlFile& source_control_file;
std::string& raw_version;
std::string compiler_id;
std::string compiler_version;
std::string triplet_abi;
InternalFeatureSet feature_list;
std::vector<PackageSpec> package_dependencies;
autoantwort marked this conversation as resolved.
Show resolved Hide resolved
};

struct IBinaryProvider
{
virtual ~IBinaryProvider() = default;
Expand All @@ -52,7 +71,9 @@ namespace vcpkg

/// Called upon a successful build of `action` to store those contents in the binary cache.
/// Prerequisite: action has a package_abi()
virtual void push_success(const InstallPlanAction& action) const = 0;
virtual void push_success(const BinaryPackageInformation& info,
const Path& packages_dir,
MessageSink& msg_sink) = 0;

/// Gives the IBinaryProvider an opportunity to batch any downloading or server communication for
/// executing `actions`.
Expand All @@ -77,7 +98,7 @@ namespace vcpkg
std::vector<std::string> headers_for_get;

LocalizedString valid() const;
std::string instantiate_variables(const InstallPlanAction& action) const;
std::string instantiate_variables(const BinaryPackageInformation& info) const;
};

struct BinaryConfigParserState
Expand Down Expand Up @@ -121,17 +142,21 @@ namespace vcpkg

struct BinaryCache
{
BinaryCache() = default;
static BinaryCache* current_instance;
static void wait_for_async_complete();
ras0219-msft marked this conversation as resolved.
Show resolved Hide resolved
BinaryCache(Filesystem& filesystem);
explicit BinaryCache(const VcpkgCmdArguments& args, const VcpkgPaths& paths);

~BinaryCache();

void install_providers(std::vector<std::unique_ptr<IBinaryProvider>>&& providers);
void install_providers_for(const VcpkgCmdArguments& args, const VcpkgPaths& paths);

/// Attempts to restore the package referenced by `action` into the packages directory.
RestoreResult try_restore(const InstallPlanAction& action);

/// Called upon a successful build of `action` to store those contents in the binary cache.
void push_success(const InstallPlanAction& action);
void push_success(const InstallPlanAction& action, Path package_dir);

/// Gives the IBinaryProvider an opportunity to batch any downloading or server communication for
/// executing `actions`.
Expand All @@ -143,8 +168,22 @@ namespace vcpkg
std::vector<CacheAvailability> precheck(View<InstallPlanAction> actions);

private:
struct ActionToPush
{
BinaryPackageInformation info;
bool clean_after_push;
Path packages_dir;
autoantwort marked this conversation as resolved.
Show resolved Hide resolved
};
void push_thread_main();

std::unordered_map<std::string, CacheStatus> m_status;
std::vector<std::unique_ptr<IBinaryProvider>> m_providers;
std::condition_variable actions_to_push_notifier;
std::mutex actions_to_push_mutex;
std::queue<ActionToPush> actions_to_push;
autoantwort marked this conversation as resolved.
Show resolved Hide resolved
std::thread push_thread;
std::atomic_bool end_push_thread;
Filesystem& filesystem;
};

ExpectedS<DownloadManagerConfig> parse_download_configuration(const Optional<std::string>& arg);
Expand Down
7 changes: 6 additions & 1 deletion include/vcpkg/binarycaching.private.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

#include <vcpkg/base/strings.h>

#include <vcpkg/binarycaching.h>
#include <vcpkg/dependencies.h>

namespace vcpkg
Expand Down Expand Up @@ -35,6 +36,10 @@ namespace vcpkg
{
return {Strings::concat(prefix, spec.dir()), format_version_for_nugetref(raw_version, abi_tag)};
}
inline NugetReference make_nugetref(const BinaryPackageInformation& info, const std::string& prefix)
{
return make_nugetref(info.spec, info.raw_version, info.package_abi, prefix);
}
inline NugetReference make_nugetref(const InstallPlanAction& action, const std::string& prefix)
{
return make_nugetref(action.spec,
Expand All @@ -57,7 +62,7 @@ namespace vcpkg
}

std::string generate_nuspec(const Path& package_dir,
const InstallPlanAction& action,
const BinaryPackageInformation& info,
const NugetReference& ref,
details::NuGetRepoInfo rinfo = details::get_nuget_repo_info_from_env());
}
1 change: 0 additions & 1 deletion include/vcpkg/build.h
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,6 @@ namespace vcpkg
ExtendedBuildResult build_package(const VcpkgCmdArguments& args,
const VcpkgPaths& paths,
const InstallPlanAction& config,
BinaryCache& binary_cache,
const IBuildLogsRecorder& build_logs_recorder,
const StatusParagraphs& status_db);

Expand Down
1 change: 1 addition & 0 deletions include/vcpkg/fwd/binarycaching.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,5 @@ namespace vcpkg
struct IBinaryProvider;
struct BinaryCache;
struct BinaryConfigParserState;
struct BinaryPackageInformation;
}
20 changes: 12 additions & 8 deletions src/vcpkg-test/binarycaching.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,20 @@ struct KnowNothingBinaryProvider : IBinaryProvider
return RestoreResult::unavailable;
}

virtual void push_success(const InstallPlanAction& action) const override { CHECK(action.has_package_abi()); }
void push_success(const BinaryPackageInformation& info, const Path&, MessageSink&) override
{
CHECK_FALSE(info.package_abi.empty());
}

virtual void prefetch(View<InstallPlanAction> actions, View<CacheStatus* const> cache_status) const override
void prefetch(View<InstallPlanAction> actions, View<CacheStatus* const> cache_status) const override
{
REQUIRE(actions.size() == cache_status.size());
for (size_t idx = 0; idx < cache_status.size(); ++idx)
{
CHECK(actions[idx].has_package_abi() == (cache_status[idx] != nullptr));
}
}
virtual void precheck(View<InstallPlanAction> actions, View<CacheStatus* const> cache_status) const override
void precheck(View<InstallPlanAction> actions, View<CacheStatus* const> cache_status) const override
{
REQUIRE(actions.size() == cache_status.size());
for (const auto c : cache_status)
Expand Down Expand Up @@ -277,7 +280,7 @@ Build-Depends: bzip
REQUIRE(ref.nupkg_filename() == "zlib2_x64-windows.1.5.0-vcpkgpackageabi.nupkg");

{
auto nuspec = generate_nuspec(pkgPath, ipa, ref, {});
auto nuspec = generate_nuspec(pkgPath, BinaryPackageInformation{ipa}, ref, {});
std::string expected = R"(<package>
<metadata>
<id>zlib2_x64-windows</id>
Expand Down Expand Up @@ -305,7 +308,7 @@ Features: a, b
}

{
auto nuspec = generate_nuspec(pkgPath, ipa, ref, {"urlvalue"});
auto nuspec = generate_nuspec(pkgPath, BinaryPackageInformation{ipa}, ref, {"urlvalue"});
std::string expected = R"(<package>
<metadata>
<id>zlib2_x64-windows</id>
Expand Down Expand Up @@ -333,7 +336,8 @@ Features: a, b
REQUIRE_EQUAL_TEXT(nuspec, expected);
}
{
auto nuspec = generate_nuspec(pkgPath, ipa, ref, {"urlvalue", "branchvalue", "commitvalue"});
auto nuspec =
generate_nuspec(pkgPath, BinaryPackageInformation{ipa}, ref, {"urlvalue", "branchvalue", "commitvalue"});
std::string expected = R"(<package>
<metadata>
<id>zlib2_x64-windows</id>
Expand Down Expand Up @@ -365,7 +369,7 @@ Features: a, b
TEST_CASE ("Provider nullptr checks", "[BinaryCache]")
{
// create a binary cache to test
BinaryCache uut;
BinaryCache uut(get_real_filesystem());
std::vector<std::unique_ptr<IBinaryProvider>> providers;
providers.emplace_back(std::make_unique<KnowNothingBinaryProvider>());
uut.install_providers(std::move(providers));
Expand All @@ -391,7 +395,7 @@ Version: 1.5
InstallPlanAction& ipa_without_abi = install_plan.back();

// test that the binary cache does the right thing. See also CHECKs etc. in KnowNothingBinaryProvider
uut.push_success(ipa_without_abi); // should have no effects
uut.push_success(ipa_without_abi, {}); // should have no effects
CHECK(uut.try_restore(ipa_without_abi) == RestoreResult::unavailable);
uut.prefetch(install_plan); // should have no effects
}
Expand Down
2 changes: 2 additions & 0 deletions src/vcpkg.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <vcpkg/base/system.debug.h>
#include <vcpkg/base/system.process.h>

#include <vcpkg/binarycaching.h>
#include <vcpkg/cgroup-parser.h>
#include <vcpkg/commands.contact.h>
#include <vcpkg/commands.h>
Expand Down Expand Up @@ -154,6 +155,7 @@ namespace vcpkg::Checks
// Implements link seam from basic_checks.h
void on_final_cleanup_and_exit()
{
BinaryCache::wait_for_async_complete();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think we can do this here. This is on the critical path for ctrl-c handling and should only be used for extremely fast, emergency tear-down behavior (like restoring the console).

If there happens to be an exit anywhere in any BinaryCache implementation, this would deadlock. Importantly, this include any sort of assertion we might want to do, like checking pointers for null.

Unfortunately, the only path forward I see is to call this (or appropriately scope the BinaryCache itself) at the relevant callers. The consequence of possibly not uploading some set of binary caches in the case of some unhandled program error (such as permissions issue on a directory expected to be writable) is vastly preferable to deadlocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed the BinaryCache::wait_for_async_complete() implementation so it does not deadlock anymore.

I also moved the call to Checks::exit_with_code which is not called when crtl+c is handled. (I personally would like to have a way to terminate vcpkg but wait until the binary cache is done so that I don't lose progress.)

And I prefer it when build packages are uploaded to the binary caches before vcpkg exits because of an error, otherwise I have to build the already build packages again at a later point when there is no cache entry.

Copy link
Contributor

@ras0219-msft ras0219-msft Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that it is, desirable to finish uploading on "understood" errors. For example, if a package failed to build or failed to be installed.

I was also wrong about my original assessment of a deadlock. My concern was the call path of the binary upload thread calling Checks::unreachable() or .value_or_exit(), but it seems that std::thread::join() does have a carve-out to handle this specific case: it will throw a resource_deadlock_would_occur if you try to join yourself.

I've put some other concerns below, but I don't want those to distract from my main point: We must make it as trivial / correct-by-construction as possible to guarantee that the binary cache thread NEVER attempts to wait on itself. I think the best approach for vcpkg right now is to add calls from Install::perform() etc to BinaryCache::wait_for_async_complete() before any "user-facing" error, such as the exit guarded by result.code != BuildResult::SUCCEEDED && keep_going == KeepGoing::NO. This is motivated by the perspective that it's always safer to terminate than to join and possibly deadlock / race condition / etc.


There's still a UB data race if the main thread and binary upload thread attempt to exit at the same time:

Concurrently calling join() on the same thread object from multiple threads constitutes a data race that results in undefined behavior.
-- https://en.cppreference.com/w/cpp/thread/thread/join

There's also a serious "scalability" problem if we ever want a second background thread for whatever reason, because BGThread A would join on BGThread B, while BGThread B tries to join on BGThread A. This might be solvable with ever more complex structures, such as a thread ownership DAG that gets threads to join only on their direct children, but I don't think the benefit is worth the cost.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UB and the joining itself could simply be prevented by doing a if (std::this_thread::get_id() == instance->push_thread.get_id()). My concern with the explicit approach is that it is easy to forget to call the waiting function of the BinaryCache and every time you want to exit you have to remember to call it. This seems to me to be very prone to human error.

Copy link
Contributor Author

@autoantwort autoantwort Mar 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have now implemented your request

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ras0219-msft Is there anything left that is preventing this PR from being merged?

const auto elapsed_us_inner = g_total_time.microseconds();
bool debugging = Debug::g_debugging;

Expand Down
Loading