diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc index e06d4e08d..2d8667a85 100644 --- a/src/libstore/legacy-ssh-store.cc +++ b/src/libstore/legacy-ssh-store.cc @@ -46,6 +46,7 @@ struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Stor FdSink to; FdSource from; ServeProto::Version remoteVersion; + bool good = true; /** * Coercion to `ServeProto::ReadConn`. This makes it easy to use the @@ -96,7 +97,8 @@ struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Stor , host(host) , connections(make_ref>( std::max(1, (int) maxConnections), - [this]() { return openConnection(); } + [this]() { return openConnection(); }, + [](const ref & r) { return r->good; } )) , master( host, @@ -194,7 +196,12 @@ struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Stor << info.ultimate << info.sigs << renderContentAddress(info.ca); - copyNAR(source, conn->to); + try { + copyNAR(source, conn->to); + } catch (...) { + conn->good = false; + throw; + } conn->to.flush(); } else { @@ -202,7 +209,12 @@ struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Stor conn->to << ServeProto::Command::ImportPaths << 1; - copyNAR(source, conn->to); + try { + copyNAR(source, conn->to); + } catch (...) { + conn->good = false; + throw; + } conn->to << exportMagic << printStorePath(info.path); diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 2ce047acd..20c1c50f2 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -39,7 +39,9 @@ RemoteStore::RemoteStore(const Params & params) }, [this](const ref & r) { return - std::chrono::duration_cast( + r->to.good() + && r->from.good() + && std::chrono::duration_cast( std::chrono::steady_clock::now() - r->startTime).count() < maxConnectionAge; } )) @@ -153,6 +155,7 @@ void RemoteStore::setOptions(Connection & conn) RemoteStore::ConnectionHandle::~ConnectionHandle() { if (!daemonException && std::uncaught_exceptions()) { + handle.markBad(); debug("closing daemon connection because of an exception"); } } @@ -178,10 +181,6 @@ void RemoteStore::ConnectionHandle::processStderr(Sink * sink, Source * source, m.find("Derive([") != std::string::npos) throw Error("%s, this might be because the daemon is too old to understand dependencies on dynamic derivations. Check to see if the raw derivation is in the form '%s'", std::move(m), "DrvWithVersion(..)"); } - // the daemon can still handle more requests, so the connection itself - // is still valid. the current *handle* however should be considered a - // lost cause and abandoned entirely. - handle.release(); throw; } } @@ -1069,15 +1068,27 @@ void RemoteStore::ConnectionHandle::withFramedSink(std::functionto, ex); - fun(sink); - sink.flush(); + { + FramedSink sink((*this)->to, ex); + fun(sink); + sink.flush(); + } + + stderrThread.join(); + if (ex) + std::rethrow_exception(ex); } } diff --git a/src/libutil/finally.hh b/src/libutil/finally.hh index f2293e5d4..cbfd6195b 100644 --- a/src/libutil/finally.hh +++ b/src/libutil/finally.hh @@ -1,6 +1,9 @@ #pragma once ///@file +#include +#include + /** * A trivial class to run a function at the end of a scope. */ @@ -19,5 +22,25 @@ public: Finally(Finally &&other) : fun(std::move(other.fun)) { other.movedFrom = true; } - ~Finally() noexcept(false) { if (!movedFrom) fun(); } + ~Finally() noexcept(false) + { + try { + if (!movedFrom) + fun(); + } catch (...) { + // finally may only throw an exception if exception handling is not already + // in progress. if handling *is* in progress we have to return cleanly here + // but are still prohibited from doing so since eating the exception would, + // in almost all cases, mess up error handling even more. the only good way + // to handle this is to abort entirely and leave a message, so we'll assert + // (and rethrow anyway, just as a defense against possible NASSERT builds.) + if (std::uncaught_exceptions()) { + assert(false && + "Finally function threw an exception during exception handling. " + "this is not what you want, please use some other methods (like " + "std::promise or async) instead."); + } + throw; + } + } }; diff --git a/src/libutil/pool.hh b/src/libutil/pool.hh index bc71083e9..1cece71ec 100644 --- a/src/libutil/pool.hh +++ b/src/libutil/pool.hh @@ -1,7 +1,6 @@ #pragma once ///@file -#include #include #include #include @@ -103,24 +102,12 @@ public: private: Pool & pool; std::shared_ptr r; + bool bad = false; friend Pool; Handle(Pool & pool, std::shared_ptr r) : pool(pool), r(r) { } - void drop(bool stillValid) - { - { - auto state_(pool.state.lock()); - if (stillValid) - state_->idle.emplace_back(std::move(r)); - assert(state_->inUse); - state_->inUse--; - } - pool.wakeup.notify_one(); - r = nullptr; - } - public: Handle(Handle && h) : pool(h.pool), r(h.r) { h.r.reset(); } @@ -128,27 +115,25 @@ public: ~Handle() { - if (r) - drop(std::uncaught_exceptions() == 0); - } - - void release() - { - drop(true); + if (!r) return; + { + auto state_(pool.state.lock()); + if (!bad) + state_->idle.push_back(ref(r)); + assert(state_->inUse); + state_->inUse--; + } + pool.wakeup.notify_one(); } R * operator -> () { return &*r; } R & operator * () { return *r; } + + void markBad() { bad = true; } }; Handle get() { - // we do not want to handle the complexity that comes with allocating - // resources during stack unwinding. it would be possible to do this, - // but doing so requires more per-handle bookkeeping to properly free - // resources allocated during unwinding. that effort is not worth it. - assert(std::uncaught_exceptions() == 0); - { auto state_(state.lock()); diff --git a/src/libutil/serialise.cc b/src/libutil/serialise.cc index 6450a9651..692144b75 100644 --- a/src/libutil/serialise.cc +++ b/src/libutil/serialise.cc @@ -52,7 +52,18 @@ FdSink::~FdSink() void FdSink::writeUnbuffered(std::string_view data) { written += data.size(); - writeFull(fd, data); + try { + writeFull(fd, data); + } catch (SysError & e) { + _good = false; + throw; + } +} + + +bool FdSink::good() +{ + return _good; } @@ -117,13 +128,19 @@ size_t FdSource::readUnbuffered(char * data, size_t len) checkInterrupt(); n = ::read(fd, data, len); } while (n == -1 && errno == EINTR); - if (n == -1) { throw SysError("reading from file"); } - if (n == 0) { throw EndOfFile(std::string(*endOfFileError)); } + if (n == -1) { _good = false; throw SysError("reading from file"); } + if (n == 0) { _good = false; throw EndOfFile(std::string(*endOfFileError)); } read += n; return n; } +bool FdSource::good() +{ + return _good; +} + + size_t StringSource::read(char * data, size_t len) { if (pos == s.size()) throw EndOfFile("end of string reached"); diff --git a/src/libutil/serialise.hh b/src/libutil/serialise.hh index e6290a652..d1c791823 100644 --- a/src/libutil/serialise.hh +++ b/src/libutil/serialise.hh @@ -18,6 +18,7 @@ struct Sink { virtual ~Sink() { } virtual void operator () (std::string_view data) = 0; + virtual bool good() { return true; } }; /** @@ -79,6 +80,8 @@ struct Source */ virtual size_t read(char * data, size_t len) = 0; + virtual bool good() { return true; } + void drainInto(Sink & sink); std::string drain(); @@ -133,6 +136,11 @@ struct FdSink : BufferedSink ~FdSink(); void writeUnbuffered(std::string_view data) override; + + bool good() override; + +private: + bool _good = true; }; @@ -157,8 +165,11 @@ struct FdSource : BufferedSource return *this; } + bool good() override; protected: size_t readUnbuffered(char * data, size_t len) override; +private: + bool _good = true; }; diff --git a/tests/unit/libutil/pool.cc b/tests/unit/libutil/pool.cc index 3ad4ed3aa..90ee509ba 100644 --- a/tests/unit/libutil/pool.cc +++ b/tests/unit/libutil/pool.cc @@ -109,19 +109,4 @@ namespace nix { ASSERT_NE(h->num, counter); } } - - TEST(Pool, throwingOperationDropsResource) - { - auto createResource = []() { return make_ref(); }; - - Pool pool = Pool((size_t)1, createResource); - - ASSERT_THROW({ - auto _r = pool.get(); - ASSERT_EQ(pool.count(), 1); - throw 1; - }, int); - - ASSERT_EQ(pool.count(), 0); - } }