refactor: give up all work in shutdown() and destroy services once

doc/modules/ROOT/pages/8.examples/8n.custom-executor.adoc

@@ -33,7 +33,7 @@ target_link_libraries(custom_executor PRIVATE Boost::capy)
 include::example$custom-executor/custom_executor.cpp[tag=inherit]
 ----
 
-The destructor must call `shutdown()`, give up any work still queued with cpp:abandon[], then call `destroy()`. Destroying a service can post more work, and unwinding an abandoned chain can create a service, so the destructor repeats `destroy()` and the drain until a drain finds nothing. The queue holds copies of each cpp:continuation[], so the owner that cpp:abandon[] needs travels with the handle.
+The destructor must call `shutdown()`, give up any work still queued with cpp:abandon[], then call `destroy()`. The drain also takes work posted while it runs. That includes the work of a service an unwound chain creates, which is shut down as it is created. Services give up their work in `shutdown()`, so destroying them posts nothing. The destructor drains once more for stray work a destructor posts anyway. No service may be created after `destroy()`. The queue holds copies of each cpp:continuation[], so the owner that cpp:abandon[] needs travels with the handle.
 
 === The Executor Concept
 

example/custom-executor/custom_executor.cpp

@@ -52,25 +52,19 @@ class run_loop : public capy::execution_context
         // are unwound. An unwind posted here is given up in turn.
         auto drain = [this]
         {
-            bool any = false;
             while (!queue_.empty())
             {
                 auto c = queue_.front();
                 queue_.pop();
-                any = true;
                 capy::abandon(c);
             }
-            return any;
         };
         shutdown();
         drain();
-        // A service's destructor may post too: repeat until nothing
-        // is left of either.
-        do
-        {
-            destroy();
-        }
-        while (drain());
+        destroy();
+        // Services give up their work in shutdown(), but anything a
+        // destructor posts anyway is still given up.
+        drain();
     }
 
     run_loop(run_loop const&) = delete;

example/gui-integration/gui_integration.cpp

@@ -89,30 +89,25 @@ class gui_app : public capy::execution_context
         // are unwound. An unwind posted here is given up in turn.
         auto drain = [this]
         {
-            bool any = false;
             for(;;)
             {
                 capy::continuation c;
                 {
                     std::lock_guard<std::mutex> lock(m_);
                     if(queue_.empty())
-                        return any;
+                        return;
                     c = queue_.front();
                     queue_.pop_front();
                 }
-                any = true;
                 capy::abandon(c);
             }
         };
         shutdown();
         drain();
-        // A service's destructor may post too: repeat until nothing
-        // is left of either.
-        do
-        {
-            destroy();
-        }
-        while(drain());
+        destroy();
+        // Services give up their work in shutdown(), but anything a
+        // destructor posts anyway is still given up.
+        drain();
     }
 
     gui_app(gui_app const&) = delete;

include/boost/capy/ex/execution_context.hpp

@@ -38,10 +38,12 @@ namespace capy {
     execution facilities like event loops and thread pools. Derived
     class destructors must call `shutdown()` and `destroy()` to ensure
     proper service cleanup before member destruction. Between the two,
-    a context that queues work abandons what is still queued. Since a
-    service's destructor can post and an unwound chain can create a
-    service, it then repeats `destroy()` and that drain until the
-    drain finds nothing.
+    a context that queues work abandons what is still queued, including
+    work posted while it does so. A service created by an unwound chain
+    is shut down as it is created, so its work is taken too. Services
+    do not post from their destructors. A context may drain once more
+    after `destroy()`, so that stray work is still destroyed, but no
+    service may be created after `destroy()` returns.
 
     @par Service Lifecycle
     Services are created on first use via `use_service()` or explicitly
@@ -106,8 +108,14 @@ class BOOST_CAPY_DECL
 
         Derived classes must implement the pure virtual `shutdown()` member
         function, which is called when the owning execution context is
-        being destroyed. The `shutdown()` function should release resources
-        and cancel outstanding operations without blocking.
+        being destroyed. The `shutdown()` function gives up all of the
+        service's work, releases resources and cancels outstanding
+        operations. It does not block, except to wait for work the
+        service already started, such as its own threads or operations
+        it submitted to the system, so that nothing that work posts
+        arrives later. The
+        destructor must not post: by the time it runs, the context has
+        already drained its queue.
 
         @par Deriving from service
         @li Implement `shutdown()` to perform cleanup.
@@ -133,10 +141,13 @@ class BOOST_CAPY_DECL
 
         /** Called when the owning execution context shuts down.
 
-            Implementations should release resources and cancel any
-            outstanding asynchronous operations. This function must
-            not block and must not throw exceptions. Services are
-            shut down in reverse order of creation.
+            Implementations give up all of the service's work here,
+            release resources and cancel any outstanding asynchronous
+            operations, rather than in the destructor. This function
+            must not throw exceptions, and must not block except to
+            wait for work the service already started, such as threads
+            it owns. Services are shut down in
+            reverse order of creation.
 
             @par Exception Safety
             No-throw guarantee.
@@ -472,8 +483,9 @@ class BOOST_CAPY_DECL
     /** Destroy all services.
 
         Deletes all services in reverse order of creation. Derived
-        classes should call this as the final step of destruction.
-        This function is idempotent; subsequent calls have no effect.
+        classes call this after giving up the work still queued, so
+        that work is destroyed while every service exists. This
+        function is idempotent; subsequent calls have no effect.
 
         @par Preconditions
         @li `shutdown()` was called.
@@ -496,6 +508,11 @@ class BOOST_CAPY_DECL
         @par Thread Safety
         Not thread-safe. Must not be called concurrently with other
         operations on this execution_context.
+
+        @note No service may be created after this returns. It would
+        outlive the derived context's members, which its destructor
+        may still reach. Debug builds check this when the context is
+        destroyed.
     */
     void destroy() noexcept;
 
@@ -536,6 +553,7 @@ class BOOST_CAPY_DECL
     std::pmr::memory_resource* frame_alloc_ = nullptr;
     service* head_ = nullptr;
     bool shutdown_ = false;
+    bool destroyed_ = false;
 };
 
 template< typename Derived >

include/boost/capy/ex/thread_pool.hpp

@@ -61,7 +61,9 @@ class BOOST_CAPY_DECL
         (or to a strand built on it) concurrently with, or after,
         destruction. Doing so is undefined behavior. Submit such work
         through @ref run_async or @ref run and call @ref join before
-        the pool is destroyed, so it has completed first.
+        the pool is destroyed, so it has completed first. A thread
+        owned by one of this pool's services may post or dispatch
+        until that service's `shutdown()` returns.
     */
     ~thread_pool();
 

src/ex/execution_context.cpp

@@ -23,6 +23,10 @@ execution_context()
 execution_context::
 ~execution_context()
 {
+    // A service created after a derived destructor destroyed the
+    // services would be destroyed here, after the derived members it
+    // may still reach.
+    BOOST_CAPY_ASSERT(!destroyed_ || !head_);
     shutdown();
     destroy();
 }
@@ -54,6 +58,7 @@ destroy() noexcept
         head_ = p->next_;
         delete p;
     }
+    destroyed_ = true;
 }
 
 execution_context::service*

src/ex/thread_pool.cpp

@@ -115,16 +115,12 @@ class thread_pool::impl
         return current_.get() == this;
     }
 
-    // Abandon queued coroutines. Called after the services shut down,
-    // since their shutdown can post more, and again after they are
-    // destroyed, for anything their destructors post. Each is taken
-    // under the lock, as other threads may still post, and abandoned
-    // outside it, as abandoning can post. Returns whether there was
-    // anything to abandon.
-    bool
+    // Abandon queued coroutines. Each is taken under the lock, as other
+    // threads may still post, and abandoned outside it, as abandoning
+    // can post.
+    void
     drain_abandoned() noexcept
     {
-        bool any = false;
         for(;;)
         {
             continuation* c;
@@ -133,8 +129,7 @@ class thread_pool::impl
                 c = pop();
             }
             if(!c)
-                return any;
-            any = true;
+                return;
             if(c->h && c->h != std::noop_coroutine())
                 capy::abandon(*c);
         }
@@ -302,14 +297,13 @@ thread_pool::
     impl_->stop();
     impl_->join();
     shutdown();
+    // Takes what the unwinds post too, and the work of any service they
+    // create, which is shut down as it is created.
+    impl_->drain_abandoned();
+    destroy();
+    // Services give up their work in shutdown(), but anything a
+    // destructor posts anyway is still destroyed.
     impl_->drain_abandoned();
-    // A frame the drain unwinds can create a service, whose destructor
-    // can post again: repeat until nothing is left of either.
-    do
-    {
-        destroy();
-    }
-    while(impl_->drain_abandoned());
     delete impl_;
 }
 

src/test/run_blocking.cpp

@@ -46,36 +46,32 @@ blocking_context::blocking_context()
 
 blocking_context::~blocking_context()
 {
-    // Nothing will run these now; abandon them as their holder. The
-    // services may post while they shut down or are destroyed, so the
-    // queue outlives both. Each is taken under the lock, as other
-    // threads may still post, and abandoned outside it.
+    // Nothing will run these now; abandon them as their holder. Each is
+    // taken under the lock and abandoned outside it, since abandoning
+    // one may post another.
     auto drain = [this]
     {
-        bool any = false;
         for(;;)
         {
             continuation c;
             {
                 std::lock_guard<std::mutex> lock(impl_->mtx);
                 if(impl_->queue.empty())
-                    return any;
+                    return;
                 c = impl_->queue.front();
                 impl_->queue.pop();
             }
-            any = true;
             capy::abandon(c);
         }
     };
     shutdown();
+    // Takes what the unwinds post too, and the work of any service they
+    // create, which is shut down as it is created.
+    drain();
+    destroy();
+    // Services give up their work in shutdown(), but anything a
+    // destructor posts anyway is still destroyed.
     drain();
-    // A frame the drain unwinds can create a service, whose destructor
-    // can post again: repeat until nothing is left of either.
-    do
-    {
-        destroy();
-    }
-    while(drain());
     delete impl_;
 }
 

test/unit/death_test.hpp

@@ -15,6 +15,7 @@
 #include <boost/capy/ex/await_context.hpp>
 
 #include <coroutine>
+#include <csignal>
 #include <exception>
 #include <type_traits>
 
@@ -70,6 +71,15 @@ terminates(F fn)
     return WIFEXITED(status) && WEXITSTATUS(status) == terminated_status;
 }
 
+/// Return true when `fn` aborts, as a failed assertion does.
+template<class F>
+bool
+aborts(F fn)
+{
+    int status = run_in_child(fn);
+    return WIFSIGNALED(status) && WTERMSIG(status) == SIGABRT;
+}
+
 /// Return true when `fn` returns, and returns true if it returns a bool.
 template<class F>
 bool

test/unit/ex/execution_context.cpp

@@ -11,6 +11,7 @@
 #include <boost/capy/ex/execution_context.hpp>
 #include <boost/capy/ex/recycling_memory_resource.hpp>
 
+#include "death_test.hpp"
 #include "test_helpers.hpp"
 
 #include <atomic>
@@ -519,6 +520,28 @@ struct execution_context_test
         BOOST_TEST_NE(new_mr, default_mr);
     }
 
+#if !defined(_WIN32) && !defined(NDEBUG)
+    // Once a context has destroyed its services, a service created
+    // later would outlive the derived context's members, so a debug
+    // build stops the program.
+    void
+    testServiceCreatedAfterDestroyAsserts()
+    {
+        struct late_creating_context : execution_context
+        {
+            ~late_creating_context()
+            {
+                shutdown();
+                destroy();
+                make_service<simple_service>();
+            }
+        };
+        BOOST_TEST(aborts([] {
+            late_creating_context ctx;
+        }));
+    }
+#endif
+
     void
     run()
     {
@@ -542,6 +565,9 @@ struct execution_context_test
         testGetFrameAllocator();
         testSetFrameAllocatorRawPointer();
         testSetFrameAllocatorTemplate();
+#if !defined(_WIN32) && !defined(NDEBUG)
+        testServiceCreatedAfterDestroyAsserts();
+#endif
     }
 };