From 8c4ea7a4516c517a0dd37b446bf5c1a6b157064c Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 3 Sep 2019 12:51:14 +0200 Subject: Downloader: Remove a possible double call to Callback --- src/libstore/download.cc | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) (limited to 'src/libstore') diff --git a/src/libstore/download.cc b/src/libstore/download.cc index c322d267d..a7d059465 100644 --- a/src/libstore/download.cc +++ b/src/libstore/download.cc @@ -342,15 +342,9 @@ struct CurlDownloader : public Downloader (httpStatus == 200 || httpStatus == 201 || httpStatus == 204 || httpStatus == 206 || httpStatus == 304 || httpStatus == 226 /* FTP */ || httpStatus == 0 /* other protocol */)) { result.cached = httpStatus == 304; + act.progress(result.bodySize, result.bodySize); done = true; - - try { - act.progress(result.bodySize, result.bodySize); - callback(std::move(result)); - } catch (...) { - done = true; - callback.rethrow(); - } + callback(std::move(result)); } else { -- cgit v1.2.3 From 7348653ff4fc4e9b2dc24943aabdb57179b1c75a Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 3 Sep 2019 12:51:35 +0200 Subject: Ensure that Callback is called only once Also, make Callback movable but uncopyable. --- src/libstore/binary-cache-store.cc | 8 +++++--- src/libstore/download.cc | 6 +++--- src/libstore/http-binary-cache-store.cc | 12 +++++++----- src/libstore/store-api.cc | 8 +++++--- 4 files changed, 20 insertions(+), 14 deletions(-) (limited to 'src/libstore') diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc index 4527ee6ba..e56be625d 100644 --- a/src/libstore/binary-cache-store.cc +++ b/src/libstore/binary-cache-store.cc @@ -249,21 +249,23 @@ void BinaryCacheStore::queryPathInfoUncached(const Path & storePath, auto narInfoFile = narInfoFileFor(storePath); + auto callbackPtr = std::make_shared(std::move(callback)); + getFile(narInfoFile, {[=](std::future> fut) { try { auto data = fut.get(); - if (!data) return callback(nullptr); + if (!data) return (*callbackPtr)(nullptr); stats.narInfoRead++; - callback((std::shared_ptr) + (*callbackPtr)((std::shared_ptr) std::make_shared(*this, *data, narInfoFile)); (void) act; // force Activity into this lambda to ensure it stays alive } catch (...) { - callback.rethrow(); + callbackPtr->rethrow(); } }}); } diff --git a/src/libstore/download.cc b/src/libstore/download.cc index a7d059465..cdf56e09d 100644 --- a/src/libstore/download.cc +++ b/src/libstore/download.cc @@ -77,13 +77,13 @@ struct CurlDownloader : public Downloader DownloadItem(CurlDownloader & downloader, const DownloadRequest & request, - Callback callback) + Callback && callback) : downloader(downloader) , request(request) , act(*logger, lvlTalkative, actDownload, fmt(request.data ? "uploading '%s'" : "downloading '%s'", request.uri), {request.uri}, request.parentAct) - , callback(callback) + , callback(std::move(callback)) , finalSink([this](const unsigned char * data, size_t len) { if (this->request.dataCallback) { writtenToSink += len; @@ -665,7 +665,7 @@ struct CurlDownloader : public Downloader return; } - enqueueItem(std::make_shared(*this, request, callback)); + enqueueItem(std::make_shared(*this, request, std::move(callback))); } }; diff --git a/src/libstore/http-binary-cache-store.cc b/src/libstore/http-binary-cache-store.cc index df2fb9332..e631d95f0 100644 --- a/src/libstore/http-binary-cache-store.cc +++ b/src/libstore/http-binary-cache-store.cc @@ -137,17 +137,19 @@ protected: auto request(makeRequest(path)); + auto callbackPtr = std::make_shared(std::move(callback)); + getDownloader()->enqueueDownload(request, - {[callback, this](std::future result) { + {[callbackPtr, this](std::future result) { try { - callback(result.get().data); + (*callbackPtr)(result.get().data); } catch (DownloadError & e) { if (e.error == Downloader::NotFound || e.error == Downloader::Forbidden) - return callback(std::shared_ptr()); + return (*callbackPtr)(std::shared_ptr()); maybeDisable(); - callback.rethrow(); + callbackPtr->rethrow(); } catch (...) { - callback.rethrow(); + callbackPtr->rethrow(); } }}); } diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 3bb9db0b7..88a5b2f44 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -365,8 +365,10 @@ void Store::queryPathInfo(const Path & storePath, } catch (...) { return callback.rethrow(); } + auto callbackPtr = std::make_shared(std::move(callback)); + queryPathInfoUncached(storePath, - {[this, storePath, hashPart, callback](std::future> fut) { + {[this, storePath, hashPart, callbackPtr](std::future> fut) { try { auto info = fut.get(); @@ -386,8 +388,8 @@ void Store::queryPathInfo(const Path & storePath, throw InvalidPath("path '%s' is not valid", storePath); } - callback(ref(info)); - } catch (...) { callback.rethrow(); } + (*callbackPtr)(ref(info)); + } catch (...) { callbackPtr->rethrow(); } }}); } -- cgit v1.2.3 From f186000367978fbe590343f47951232f42a30bec Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 3 Sep 2019 13:00:55 +0200 Subject: Add some noexcepts This is to assert that callback functions should never throw (since the context in which they're called may not be able to handle the exception). --- src/libstore/binary-cache-store.cc | 4 ++-- src/libstore/binary-cache-store.hh | 4 ++-- src/libstore/http-binary-cache-store.cc | 2 +- src/libstore/legacy-ssh-store.cc | 2 +- src/libstore/local-store.cc | 2 +- src/libstore/local-store.hh | 2 +- src/libstore/remote-store.cc | 2 +- src/libstore/remote-store.hh | 2 +- src/libstore/store-api.cc | 9 +++++---- src/libstore/store-api.hh | 4 ++-- 10 files changed, 17 insertions(+), 16 deletions(-) (limited to 'src/libstore') diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc index e56be625d..10cde8704 100644 --- a/src/libstore/binary-cache-store.cc +++ b/src/libstore/binary-cache-store.cc @@ -55,7 +55,7 @@ void BinaryCacheStore::init() } void BinaryCacheStore::getFile(const std::string & path, - Callback> callback) + Callback> callback) noexcept { try { callback(getFile(path)); @@ -240,7 +240,7 @@ void BinaryCacheStore::narFromPath(const Path & storePath, Sink & sink) } void BinaryCacheStore::queryPathInfoUncached(const Path & storePath, - Callback> callback) + Callback> callback) noexcept { auto uri = getUri(); auto act = std::make_shared(*logger, lvlTalkative, actQueryPathInfo, diff --git a/src/libstore/binary-cache-store.hh b/src/libstore/binary-cache-store.hh index 953f3b90a..af108880c 100644 --- a/src/libstore/binary-cache-store.hh +++ b/src/libstore/binary-cache-store.hh @@ -47,7 +47,7 @@ public: /* Fetch the specified file and call the specified callback with the result. A subclass may implement this asynchronously. */ virtual void getFile(const std::string & path, - Callback> callback); + Callback> callback) noexcept; std::shared_ptr getFile(const std::string & path); @@ -73,7 +73,7 @@ public: bool isValidPathUncached(const Path & path) override; void queryPathInfoUncached(const Path & path, - Callback> callback) override; + Callback> callback) noexcept override; Path queryPathFromHashPart(const string & hashPart) override { unsupported("queryPathFromHashPart"); } diff --git a/src/libstore/http-binary-cache-store.cc b/src/libstore/http-binary-cache-store.cc index e631d95f0..779f89e68 100644 --- a/src/libstore/http-binary-cache-store.cc +++ b/src/libstore/http-binary-cache-store.cc @@ -131,7 +131,7 @@ protected: } void getFile(const std::string & path, - Callback> callback) override + Callback> callback) noexcept override { checkEnabled(); diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc index 7c9bc2b68..d5fbdd25a 100644 --- a/src/libstore/legacy-ssh-store.cc +++ b/src/libstore/legacy-ssh-store.cc @@ -88,7 +88,7 @@ struct LegacySSHStore : public Store } void queryPathInfoUncached(const Path & path, - Callback> callback) override + Callback> callback) noexcept override { try { auto conn(connections->get()); diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 63b11467e..2fcf08491 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -629,7 +629,7 @@ uint64_t LocalStore::addValidPath(State & state, void LocalStore::queryPathInfoUncached(const Path & path, - Callback> callback) + Callback> callback) noexcept { try { auto info = std::make_shared(); diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index af8b84bf5..3ae34c403 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -127,7 +127,7 @@ public: PathSet queryAllValidPaths() override; void queryPathInfoUncached(const Path & path, - Callback> callback) override; + Callback> callback) noexcept override; void queryReferrers(const Path & path, PathSet & referrers) override; diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 1c2e23f9c..e38fe49a7 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -349,7 +349,7 @@ void RemoteStore::querySubstitutablePathInfos(const PathSet & paths, void RemoteStore::queryPathInfoUncached(const Path & path, - Callback> callback) + Callback> callback) noexcept { try { std::shared_ptr info; diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh index 80f18ab71..82fbec092 100644 --- a/src/libstore/remote-store.hh +++ b/src/libstore/remote-store.hh @@ -41,7 +41,7 @@ public: PathSet queryAllValidPaths() override; void queryPathInfoUncached(const Path & path, - Callback> callback) override; + Callback> callback) noexcept override; void queryReferrers(const Path & path, PathSet & referrers) override; diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 88a5b2f44..5f63c53b5 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -329,13 +329,14 @@ ref Store::queryPathInfo(const Path & storePath) void Store::queryPathInfo(const Path & storePath, - Callback> callback) + Callback> callback) noexcept { - assertStorePath(storePath); - - auto hashPart = storePathToHash(storePath); + std::string hashPart; try { + assertStorePath(storePath); + + hashPart = storePathToHash(storePath); { auto res = state.lock()->pathInfoCache.get(hashPart); diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 599677376..7fb568602 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -360,12 +360,12 @@ public: /* Asynchronous version of queryPathInfo(). */ void queryPathInfo(const Path & path, - Callback> callback); + Callback> callback) noexcept; protected: virtual void queryPathInfoUncached(const Path & path, - Callback> callback) = 0; + Callback> callback) noexcept = 0; public: -- cgit v1.2.3 From e07ec8d27e08bf23eccab079b044a6f1b37f3ac9 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 3 Sep 2019 16:02:12 +0200 Subject: Support allowSubstitutes attribute in structured attribute derivations Hopefully fixes #3081 (didn't test). --- src/libstore/build.cc | 2 +- src/libstore/derivations.cc | 6 ------ src/libstore/derivations.hh | 2 -- src/libstore/misc.cc | 4 +++- src/libstore/parsed-derivations.cc | 5 +++++ src/libstore/parsed-derivations.hh | 2 ++ 6 files changed, 11 insertions(+), 10 deletions(-) (limited to 'src/libstore') diff --git a/src/libstore/build.cc b/src/libstore/build.cc index be52b66a7..ab725e8e9 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -1197,7 +1197,7 @@ void DerivationGoal::haveDerivation() /* We are first going to try to create the invalid output paths through substitutes. If that doesn't work, we'll build them. */ - if (settings.useSubstitutes && drv->substitutesAllowed()) + if (settings.useSubstitutes && parsedDrv->substitutesAllowed()) for (auto & i : invalidOutputs) addWaitee(worker.makeSubstitutionGoal(i, buildMode == bmRepair ? Repair : NoRepair)); diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 3961126ff..23fcfb281 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -36,12 +36,6 @@ Path BasicDerivation::findOutput(const string & id) const } -bool BasicDerivation::substitutesAllowed() const -{ - return get(env, "allowSubstitutes", "1") == "1"; -} - - bool BasicDerivation::isBuiltin() const { return string(builder, 0, 8) == "builtin:"; diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh index 9753e796d..8e02c9bc5 100644 --- a/src/libstore/derivations.hh +++ b/src/libstore/derivations.hh @@ -56,8 +56,6 @@ struct BasicDerivation the given derivation. */ Path findOutput(const string & id) const; - bool substitutesAllowed() const; - bool isBuiltin() const; /* Return true iff this is a fixed-output derivation. */ diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index adcce026f..dddf13430 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -1,4 +1,5 @@ #include "derivations.hh" +#include "parsed-derivations.hh" #include "globals.hh" #include "local-store.hh" #include "store-api.hh" @@ -189,6 +190,7 @@ void Store::queryMissing(const PathSet & targets, } Derivation drv = derivationFromPath(i2.first); + ParsedDerivation parsedDrv(i2.first, drv); PathSet invalid; for (auto & j : drv.outputs) @@ -197,7 +199,7 @@ void Store::queryMissing(const PathSet & targets, invalid.insert(j.second.path); if (invalid.empty()) return; - if (settings.useSubstitutes && drv.substitutesAllowed()) { + if (settings.useSubstitutes && parsedDrv.substitutesAllowed()) { auto drvState = make_ref>(DrvState(invalid.size())); for (auto & output : invalid) pool.enqueue(std::bind(checkOutput, i2.first, make_ref(drv), output, drvState)); diff --git a/src/libstore/parsed-derivations.cc b/src/libstore/parsed-derivations.cc index 17fde00a0..87be8a24e 100644 --- a/src/libstore/parsed-derivations.cc +++ b/src/libstore/parsed-derivations.cc @@ -108,4 +108,9 @@ bool ParsedDerivation::willBuildLocally() const return getBoolAttr("preferLocalBuild") && canBuildLocally(); } +bool ParsedDerivation::substitutesAllowed() const +{ + return getBoolAttr("allowSubstitutes", true); +} + } diff --git a/src/libstore/parsed-derivations.hh b/src/libstore/parsed-derivations.hh index ed07dc652..9bde4b4dc 100644 --- a/src/libstore/parsed-derivations.hh +++ b/src/libstore/parsed-derivations.hh @@ -30,6 +30,8 @@ public: bool canBuildLocally() const; bool willBuildLocally() const; + + bool substitutesAllowed() const; }; } -- cgit v1.2.3