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

7 files changed, 104 insertions, 60 deletions

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<Pool<Connection>>(
             std::max(1, (int) maxConnections),
-            [this]() { return openConnection(); }
+            [this]() { return openConnection(); },
+            [](const ref<Connection> & 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<Connection> & r) {
                 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;
             }
             ))
@@ -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::function<void(Sink & sin
 
     Finally joinStderrThread([&]()
     {
-        stderrThread.join();
-        if (ex) {
-            std::rethrow_exception(ex);
+        if (stderrThread.joinable()) {
+            stderrThread.join();
+            if (ex) {
+                try {
+                    std::rethrow_exception(ex);
+                } catch (...) {
+                    ignoreException();
+                }
+            }
         }
     });
 
-    FramedSink sink((*this)->to, 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 <cassert>
+#include <exception>
+
 /**
  * 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 <exception>
 #include <functional>
 #include <limits>
 #include <list>
@@ -103,24 +102,12 @@ public:
     private:
         Pool & pool;
         std::shared_ptr<R> r;
+        bool bad = false;
 
         friend Pool;
 
         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:
         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>(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<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);
-    }
 }