Merge changes I1fa30114,I3ca208b6,Ide4c6e00,I74c46b9f,I05fa6a9d, ... into main

* changes:
  Revert "libutil: drop Pool resources on exceptional free"
  Revert "libutil: remove Pool::Handle::bad"
  Revert "libstore: remove one Resource::good flag"
  Revert "libstore: using throwing finally in withFramedSink"
  Revert "libutil: allow graceful dropping of Pool::Handle"
  Revert "libutil: drop Fs{Source,Sink}::good"
  libutil: guard Finally against invalid exception throws
This commit is contained in:
eldritch horrors 2024-04-05 23:17:18 +00:00 committed by Gerrit Code Review
commit e9e1b6963c
7 changed files with 104 additions and 60 deletions

View file

@ -46,6 +46,7 @@ struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Stor
FdSink to; FdSink to;
FdSource from; FdSource from;
ServeProto::Version remoteVersion; ServeProto::Version remoteVersion;
bool good = true;
/** /**
* Coercion to `ServeProto::ReadConn`. This makes it easy to use the * 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) , host(host)
, connections(make_ref<Pool<Connection>>( , connections(make_ref<Pool<Connection>>(
std::max(1, (int) maxConnections), std::max(1, (int) maxConnections),
[this]() { return openConnection(); } [this]() { return openConnection(); },
[](const ref<Connection> & r) { return r->good; }
)) ))
, master( , master(
host, host,
@ -194,7 +196,12 @@ struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Stor
<< info.ultimate << info.ultimate
<< info.sigs << info.sigs
<< renderContentAddress(info.ca); << renderContentAddress(info.ca);
copyNAR(source, conn->to); try {
copyNAR(source, conn->to);
} catch (...) {
conn->good = false;
throw;
}
conn->to.flush(); conn->to.flush();
} else { } else {
@ -202,7 +209,12 @@ struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Stor
conn->to conn->to
<< ServeProto::Command::ImportPaths << ServeProto::Command::ImportPaths
<< 1; << 1;
copyNAR(source, conn->to); try {
copyNAR(source, conn->to);
} catch (...) {
conn->good = false;
throw;
}
conn->to conn->to
<< exportMagic << exportMagic
<< printStorePath(info.path); << printStorePath(info.path);

View file

@ -39,7 +39,9 @@ RemoteStore::RemoteStore(const Params & params)
}, },
[this](const ref<Connection> & r) { [this](const ref<Connection> & r) {
return return
std::chrono::duration_cast<std::chrono::seconds>( r->to.good()
&& r->from.good()
&& std::chrono::duration_cast<std::chrono::seconds>(
std::chrono::steady_clock::now() - r->startTime).count() < maxConnectionAge; std::chrono::steady_clock::now() - r->startTime).count() < maxConnectionAge;
} }
)) ))
@ -153,6 +155,7 @@ void RemoteStore::setOptions(Connection & conn)
RemoteStore::ConnectionHandle::~ConnectionHandle() RemoteStore::ConnectionHandle::~ConnectionHandle()
{ {
if (!daemonException && std::uncaught_exceptions()) { if (!daemonException && std::uncaught_exceptions()) {
handle.markBad();
debug("closing daemon connection because of an exception"); 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) 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(..)"); 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; throw;
} }
} }
@ -1069,15 +1068,27 @@ void RemoteStore::ConnectionHandle::withFramedSink(std::function<void(Sink & sin
Finally joinStderrThread([&]() Finally joinStderrThread([&]()
{ {
stderrThread.join(); if (stderrThread.joinable()) {
if (ex) { stderrThread.join();
std::rethrow_exception(ex); if (ex) {
try {
std::rethrow_exception(ex);
} catch (...) {
ignoreException();
}
}
} }
}); });
FramedSink sink((*this)->to, ex); {
fun(sink); FramedSink sink((*this)->to, ex);
sink.flush(); fun(sink);
sink.flush();
}
stderrThread.join();
if (ex)
std::rethrow_exception(ex);
} }
} }

View file

@ -1,6 +1,9 @@
#pragma once #pragma once
///@file ///@file
#include <cassert>
#include <exception>
/** /**
* A trivial class to run a function at the end of a scope. * 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)) { Finally(Finally &&other) : fun(std::move(other.fun)) {
other.movedFrom = true; 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;
}
}
}; };

View file

@ -1,7 +1,6 @@
#pragma once #pragma once
///@file ///@file
#include <exception>
#include <functional> #include <functional>
#include <limits> #include <limits>
#include <list> #include <list>
@ -103,24 +102,12 @@ public:
private: private:
Pool & pool; Pool & pool;
std::shared_ptr<R> r; std::shared_ptr<R> r;
bool bad = false;
friend Pool; friend Pool;
Handle(Pool & pool, std::shared_ptr<R> r) : pool(pool), r(r) { } Handle(Pool & pool, std::shared_ptr<R> 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: public:
Handle(Handle && h) : pool(h.pool), r(h.r) { h.r.reset(); } Handle(Handle && h) : pool(h.pool), r(h.r) { h.r.reset(); }
@ -128,27 +115,25 @@ public:
~Handle() ~Handle()
{ {
if (r) if (!r) return;
drop(std::uncaught_exceptions() == 0); {
} auto state_(pool.state.lock());
if (!bad)
void release() state_->idle.push_back(ref<R>(r));
{ assert(state_->inUse);
drop(true); state_->inUse--;
}
pool.wakeup.notify_one();
} }
R * operator -> () { return &*r; } R * operator -> () { return &*r; }
R & operator * () { return *r; } R & operator * () { return *r; }
void markBad() { bad = true; }
}; };
Handle get() 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()); auto state_(state.lock());

View file

@ -52,7 +52,18 @@ FdSink::~FdSink()
void FdSink::writeUnbuffered(std::string_view data) void FdSink::writeUnbuffered(std::string_view data)
{ {
written += data.size(); 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(); checkInterrupt();
n = ::read(fd, data, len); n = ::read(fd, data, len);
} while (n == -1 && errno == EINTR); } while (n == -1 && errno == EINTR);
if (n == -1) { throw SysError("reading from file"); } if (n == -1) { _good = false; throw SysError("reading from file"); }
if (n == 0) { throw EndOfFile(std::string(*endOfFileError)); } if (n == 0) { _good = false; throw EndOfFile(std::string(*endOfFileError)); }
read += n; read += n;
return n; return n;
} }
bool FdSource::good()
{
return _good;
}
size_t StringSource::read(char * data, size_t len) size_t StringSource::read(char * data, size_t len)
{ {
if (pos == s.size()) throw EndOfFile("end of string reached"); if (pos == s.size()) throw EndOfFile("end of string reached");

View file

@ -18,6 +18,7 @@ struct Sink
{ {
virtual ~Sink() { } virtual ~Sink() { }
virtual void operator () (std::string_view data) = 0; 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 size_t read(char * data, size_t len) = 0;
virtual bool good() { return true; }
void drainInto(Sink & sink); void drainInto(Sink & sink);
std::string drain(); std::string drain();
@ -133,6 +136,11 @@ struct FdSink : BufferedSink
~FdSink(); ~FdSink();
void writeUnbuffered(std::string_view data) override; void writeUnbuffered(std::string_view data) override;
bool good() override;
private:
bool _good = true;
}; };
@ -157,8 +165,11 @@ struct FdSource : BufferedSource
return *this; return *this;
} }
bool good() override;
protected: protected:
size_t readUnbuffered(char * data, size_t len) override; size_t readUnbuffered(char * data, size_t len) override;
private:
bool _good = true;
}; };

View file

@ -109,19 +109,4 @@ namespace nix {
ASSERT_NE(h->num, counter); ASSERT_NE(h->num, counter);
} }
} }
TEST(Pool, throwingOperationDropsResource)
{
auto createResource = []() { return make_ref<TestResource>(); };
Pool<TestResource> pool = Pool<TestResource>((size_t)1, createResource);
ASSERT_THROW({
auto _r = pool.get();
ASSERT_EQ(pool.count(), 1);
throw 1;
}, int);
ASSERT_EQ(pool.count(), 0);
}
} }