diff --git a/doc/design/lifetime-contract.md b/doc/design/lifetime-contract.md index 3d04e2b4a..de8958611 100644 --- a/doc/design/lifetime-contract.md +++ b/doc/design/lifetime-contract.md @@ -183,7 +183,11 @@ reach an unrelated file: `io_context` destruction runs every service's `shutdown()` newest first, then deletes the services in the same order. Services created after the scheduler come before it; the timer service, which the -scheduler's constructor creates, comes after it. The contract for that +scheduler's constructor creates, comes after it. Every service gives +up all its work in `shutdown()` and posts nothing from its destructor. +So nothing reaches the queue after the drain in step 3. Waiting for +the kernel or the OS to return operations a service submitted, as in +step 2, is part of giving up its work. The contract for that sequence: 0. **Timers first.** Every scheduler shuts the timer service down @@ -218,9 +222,11 @@ sequence: context, and work that crosses contexts is finished before either is destroyed. The count is pinned before each delete, so references an implementation holds on itself cannot re-enter `retire()`. -5. **Late work.** Work posted after the drain, for example from a - service's destructor, is destroyed when the scheduler is deleted, - after every service created after it. +5. **Late work.** Nothing should reach this step. Work posted after + the drain anyway, for example from a service's destructor, is + destroyed when the scheduler is deleted, after every service created + after it. That work must not use those services: an op embedded in + one of their implementations has already been freed. A step that cannot complete must not be skipped silently. For example, a cancel that cannot be submitted is the one case where @@ -254,5 +260,5 @@ How the code meets each rule: | §5.3 IOCP | Conforms: socket and acceptor initiators re-check cancellation before `on_pending`, as the handle initiators do. IOCP is checked only by Windows CI. | | §5.5 descriptor numbers | Conforms: posix file gate, uring flush before close or release, cancelled waits skip the probe on uring and the IOCP wait reactor. | | §5.4 executor queues | Conforms: every awaitable owns its continuation; no op embeds one. | -| §6 teardown | Conforms on all backends, timers first everywhere; implemented per backend with no shared statement. | +| §6 teardown | Conforms on all backends, timers first everywhere; implemented per backend with no shared statement. Every service gives up its work in `shutdown()` and posts nothing from its destructor, so every frame is destroyed before any service (`testShutdownDestroysFramesBeforeServices` in `teardown_destroy.cpp`). | | §7 recycling | Mixed. Some `reuse()` methods assert fields others reset. | diff --git a/doc/modules/ROOT/pages/4.guide/4c.io-context.adoc b/doc/modules/ROOT/pages/4.guide/4c.io-context.adoc index f7d28cd34..58726ad27 100644 --- a/doc/modules/ROOT/pages/4.guide/4c.io-context.adoc +++ b/doc/modules/ROOT/pages/4.guide/4c.io-context.adoc @@ -256,9 +256,10 @@ another. No order of destruction is then safe, because its unwind runs after one of the two contexts is gone. Stop such work, and run both contexts until it ends, before destroying either. -Work posted after the services shut down, for example from a service's -destructor, is destroyed after every other service. Its destructors must not -use them. +Every service gives up its work while the services shut down, so nothing is +left when they are destroyed. Work posted after that anyway, for example from a +service's destructor, is destroyed while the services are being destroyed. Its +destructors must not use any service. == Inheritance from execution_context diff --git a/doc/modules/ROOT/pages/4.guide/4t.lifetime.adoc b/doc/modules/ROOT/pages/4.guide/4t.lifetime.adoc index e6a319fa3..989177748 100644 --- a/doc/modules/ROOT/pages/4.guide/4t.lifetime.adoc +++ b/doc/modules/ROOT/pages/4.guide/4t.lifetime.adoc @@ -113,6 +113,10 @@ Check each of these: . *Give up, never destroy.* A holder that is shutting down, and so can no longer resume a continuation, passes it to `capy::abandon`. It never calls `destroy()` on the handle. +. *A service gives up its work in `shutdown()`.* Its destructor posts nothing. + Work posted once every service has shut down is destroyed while the + services are being destroyed, and its destructors may reach one already + gone. . *One owner of a continuation.* Each continuation is resumed, abandoned, or still held by the caller after a `post` that threw. Never two of these, and never none. diff --git a/include/boost/corosio/detail/thread_pool.hpp b/include/boost/corosio/detail/thread_pool.hpp index ba623f93f..8408c5333 100644 --- a/include/boost/corosio/detail/thread_pool.hpp +++ b/include/boost/corosio/detail/thread_pool.hpp @@ -141,13 +141,12 @@ class BOOST_COROSIO_SYMBOL_VISIBLE thread_pool final throw std::logic_error("thread_pool requires at least 1 thread"); } - /** Destroy the pool, joining any worker `shutdown()` never reached. + /** Destroy the pool, joining any worker `shutdown()` did not. - The context's shutdown walk is the normal path; this only - catches a pool created after that walk, whose `shutdown()` is - therefore never called and whose joinable threads would - otherwise terminate the process. A pool that was never posted - to holds no thread and needs neither. + A context shuts down the pool it owns before destroying it, or + at creation if the context's shutdown already began. A pool + constructed directly gets no such call, so this shuts it down + rather than leave a joinable worker to terminate the process. */ ~thread_pool() override { diff --git a/include/boost/corosio/io_context.hpp b/include/boost/corosio/io_context.hpp index d21822df5..20e403259 100644 --- a/include/boost/corosio/io_context.hpp +++ b/include/boost/corosio/io_context.hpp @@ -407,9 +407,11 @@ class BOOST_COROSIO_DECL io_context : public capy::execution_context whatever the order of destruction. Stop such work and run both contexts until it ends before destroying either. - @note Work posted after the services have shut down, for example - from a service's destructor, is abandoned while the services - are being destroyed. Its destructors must not use them. + @note Every service gives up its work while the services shut + down, so nothing is left when they are destroyed. Work posted + after that anyway, for example from a service's destructor, is + abandoned while the services are being destroyed. Its + destructors must not use any service. */ ~io_context(); diff --git a/include/boost/corosio/native/detail/iocp/win_scheduler.hpp b/include/boost/corosio/native/detail/iocp/win_scheduler.hpp index e002de352..3139b539e 100644 --- a/include/boost/corosio/native/detail/iocp/win_scheduler.hpp +++ b/include/boost/corosio/native/detail/iocp/win_scheduler.hpp @@ -875,7 +875,8 @@ inline win_scheduler::~win_scheduler() if (iocp_ != nullptr) { - // Service destructors run after the drain and may still post. + // Services give up their work in shutdown(), but anything posted + // after the drain, such as from a destructor, is still destroyed. sweep_port(); ::CloseHandle(iocp_); } diff --git a/include/boost/corosio/native/detail/reactor/reactor_scheduler.hpp b/include/boost/corosio/native/detail/reactor/reactor_scheduler.hpp index 148b28c02..a32f0a204 100644 --- a/include/boost/corosio/native/detail/reactor/reactor_scheduler.hpp +++ b/include/boost/corosio/native/detail/reactor/reactor_scheduler.hpp @@ -842,7 +842,8 @@ reactor_scheduler::shutdown_drain() inline reactor_scheduler::~reactor_scheduler() { - // Service destructors run after the drain and may still post. + // Services give up their work in shutdown(), but anything posted + // after the drain, such as from a destructor, is still destroyed. lock_type lock(mutex_); destroy_queued(lock); } diff --git a/include/boost/corosio/native/detail/uring/uring_scheduler.hpp b/include/boost/corosio/native/detail/uring/uring_scheduler.hpp index 9788b8af9..81ce3e3c5 100644 --- a/include/boost/corosio/native/detail/uring/uring_scheduler.hpp +++ b/include/boost/corosio/native/detail/uring/uring_scheduler.hpp @@ -583,7 +583,8 @@ inline uring_scheduler::uring_scheduler( inline uring_scheduler::~uring_scheduler() { - // Service destructors run after the drain and may still post. + // Services give up their work in shutdown(), but anything posted + // after the drain, such as from a destructor, is still destroyed. { lock_type lock(dispatch_mutex_); destroy_queued(lock); diff --git a/test/unit/pool_teardown.hpp b/test/unit/pool_teardown.hpp index 60e19fb4c..dc5162824 100644 --- a/test/unit/pool_teardown.hpp +++ b/test/unit/pool_teardown.hpp @@ -66,25 +66,76 @@ class pool_blocker : public detail::pool_work_item } }; +/** Post probes to a pool until it refuses one, then release a blocker. + + A refused post is the only sign that the pool's shutdown has begun. + The probes accepted before then queue harmlessly behind the rest and + are disposed of by that shutdown, so `probes` must outlive it. + + @param pool The pool whose shutdown is awaited. + @param blocker The item occupying the pool's only worker. + @param probes Storage for the probes the pool accepts. +*/ +inline void +release_when_refused( + detail::thread_pool& pool, + pool_blocker& blocker, + std::list& probes) +{ + for (;;) + { + auto& p = probes.emplace_back(); + p.func_ = [](detail::pool_work_item*) noexcept {}; + p.destroy_ = [](detail::pool_work_item*) noexcept {}; + if (pool.post(&p)) + break; + std::this_thread::yield(); + } + blocker.release(); +} + /** A service that releases a @ref pool_blocker as the context shuts down. Create it last so the newest-first shutdown walk runs it before any - library service: the blocked worker is let go at the very top of - teardown, and the operation queued behind it is therefore still - pending, on the pool or as a queued completion, while the context - is being destroyed. + library service. Armed with the blocker alone, it lets the blocked + worker go at the very top of teardown, so the operation queued + behind it is still pending, on the pool or as a queued completion, + while the context is being destroyed. + + Armed with the pool too, it holds the worker until the pool's own + shutdown has begun, so the queued operation is always disposed of + by the pool rather than run. */ class pool_release_gate final : public capy::execution_context::service { - pool_blocker* blocker_ = nullptr; + pool_blocker* blocker_ = nullptr; + detail::thread_pool* pool_ = nullptr; + std::list probes_; + std::thread releaser_; void shutdown() override { - if (blocker_) + if (!blocker_) + return; + if (!pool_) + { blocker_->release(); + return; + } + // The pool is older, so it shuts down after this returns; the + // wait for it has to happen on another thread. + releaser_ = std::thread( + [this] { release_when_refused(*pool_, *blocker_, probes_); }); } public: + /// Destroy the gate, joining its releaser. + ~pool_release_gate() + { + if (releaser_.joinable()) + releaser_.join(); + } + /// The lookup key for this service. using key_type = pool_release_gate; @@ -94,10 +145,12 @@ class pool_release_gate final : public capy::execution_context::service /** Arm the gate. @param b The blocker to release when the context shuts down. + @param pool If set, release only once this pool refuses work. */ - void arm(pool_blocker& b) noexcept + void arm(pool_blocker& b, detail::thread_pool* pool = nullptr) noexcept { blocker_ = &b; + pool_ = pool; } }; @@ -105,16 +158,23 @@ class pool_release_gate final : public capy::execution_context::service @param ioc The context whose pool is occupied. @param blocker The item to post; must outlive `ioc`. + @param until_pool_shutdown If true, the blocker is released only + once the pool's shutdown has begun, so everything queued behind + it is disposed of rather than run. @return True if the pool accepted the item. */ inline bool -park_pool_worker(io_context& ioc, pool_blocker& blocker) +park_pool_worker( + io_context& ioc, + pool_blocker& blocker, + bool until_pool_shutdown = false) { auto* pool = ioc.find_service(); if (!pool) return false; - ioc.use_service().arm(blocker); + ioc.use_service().arm( + blocker, until_pool_shutdown ? pool : nullptr); return !pool->post(&blocker); } @@ -130,27 +190,10 @@ park_pool_worker(io_context& ioc, pool_blocker& blocker) inline void shutdown_pool_behind(io_context& ioc, pool_blocker& blocker) { - struct probe : detail::pool_work_item - { - static void ignore(detail::pool_work_item*) noexcept {} - - probe() noexcept - { - func_ = &probe::ignore; - destroy_ = &probe::ignore; - } - }; - auto& pool = ioc.use_service(); + std::list probes; std::thread stopper([&pool] { pool.shutdown(); }); - - // A refused post is the only sign that shutdown has begun; the - // probes accepted before then queue harmlessly behind the rest. - std::list probes; - while (!pool.post(&probes.emplace_back())) - std::this_thread::yield(); - - blocker.release(); + release_when_refused(pool, blocker, probes); stopper.join(); } diff --git a/test/unit/teardown_destroy.cpp b/test/unit/teardown_destroy.cpp index a063ed4e7..93b283d6d 100644 --- a/test/unit/teardown_destroy.cpp +++ b/test/unit/teardown_destroy.cpp @@ -8,6 +8,7 @@ // #include +#include #include #include #include @@ -20,6 +21,7 @@ #include #include #include +#include #include #include @@ -44,6 +46,13 @@ namespace boost::corosio { template struct teardown_destroy_test { +#if BOOST_COROSIO_HAS_URING + static constexpr bool on_ring = + std::is_same_v, uring_t>; +#else + static constexpr bool on_ring = false; +#endif + // Frames from the recycling allocator are never really freed, which // hides a use-after-free from ASan; these go through operator new. static auto spawn(io_context& ioc) @@ -219,12 +228,6 @@ struct teardown_destroy_test #if BOOST_COROSIO_POSIX test::temp_file tmp("teardown_destroy_", "hello world"); -#if BOOST_COROSIO_HAS_URING - constexpr bool on_ring = - std::is_same_v, uring_t>; -#else - constexpr bool on_ring = false; -#endif // io_uring reads files through the ring, not the pool. if constexpr (!on_ring) { @@ -439,12 +442,6 @@ struct teardown_destroy_test void testCrossContextSessionDrains() { -#if BOOST_COROSIO_HAS_URING - constexpr bool on_ring = - std::is_same_v, uring_t>; -#else - constexpr bool on_ring = false; -#endif // io_uring reads files through the ring, not the pool. if constexpr (!on_ring) checkCrossContextSessionDrains(true); @@ -452,6 +449,130 @@ struct teardown_destroy_test } #endif + // Records, when the context destroys its services, how many frames + // were already destroyed. Made last, it is destroyed first. + struct teardown_witness : capy::execution_context::service + { + int const* down; + int* seen; + + teardown_witness(capy::execution_context&, int const& d, int& s) + : down(&d) + , seen(&s) + { + } + + ~teardown_witness() + { + *seen = *down; + } + + void shutdown() override {} + }; + + // Shutting a context down destroys every frame suspended in it before + // any service is destroyed. The frames that own their I/O object, the + // timers and the pool read are still held by a service when it shuts + // down; those that borrow one are cancelled when it closes, before + // the context goes. + void testShutdownDestroysFramesBeforeServices() + { + int down = 0; + int seen = -1; + int expected = 0; + auto counted = [&] { + ++expected; + return destroy_count_t(down); + }; +#if BOOST_COROSIO_POSIX + test::temp_file tmp("teardown_destroy_", "hello world"); + test::pool_blocker blocker; +#endif + { + io_context ioc(Backend); + auto [a, b] = test::make_socket_pair(ioc); + auto [c, d] = test::make_socket_pair(ioc); + tcp_acceptor acc(ioc); + BOOST_TEST(!acc.open()); + acc.set_option(socket_option::reuse_address(true)); + BOOST_TEST(!acc.bind(endpoint(ipv4_address::loopback(), 0))); + BOOST_TEST(!acc.listen()); + tcp_acceptor owned(ioc); + BOOST_TEST(!owned.open()); + BOOST_TEST( + !owned.bind(endpoint(ipv4_address::loopback(), 0))); + BOOST_TEST(!owned.listen()); + signal_set sigs(ioc, SIGINT); + capy::strand st(ioc.get_executor()); + + auto on_delay = [](destroy_count_t) -> capy::task<> { + std::ignore = co_await delay(std::chrono::hours(1)); + }; + auto on_read = [](tcp_socket& s, destroy_count_t) -> capy::task<> { + char ch; + std::ignore = + co_await s.read_some(capy::mutable_buffer(&ch, 1)); + }; + auto owns_socket = [](tcp_socket s, + destroy_count_t) -> capy::task<> { + char ch; + std::ignore = + co_await s.read_some(capy::mutable_buffer(&ch, 1)); + }; + auto on_accept = [](tcp_acceptor& ac, + destroy_count_t) -> capy::task<> { + std::ignore = co_await ac.accept(); + }; + auto owns_acceptor = [](tcp_acceptor ac, + destroy_count_t) -> capy::task<> { + std::ignore = co_await ac.accept(); + }; + auto on_signal = [](signal_set& ss, + destroy_count_t) -> capy::task<> { + std::ignore = co_await ss.wait(); + }; + auto owns_set = [](signal_set ss, destroy_count_t) -> capy::task<> { + std::ignore = co_await ss.wait(); + }; + + spawn(ioc)(on_delay(counted())); + spawn(ioc)(on_read(a, counted())); + spawn(ioc)(owns_socket(std::move(c), counted())); + spawn(ioc)(on_accept(acc, counted())); + spawn(ioc)( + owns_acceptor(std::move(owned), counted())); + spawn(ioc)(on_signal(sigs, counted())); + spawn(ioc)( + owns_set(signal_set(ioc, SIGINT), counted())); + capy::run_async(st, std::allocator{})( + on_delay(counted())); +#if BOOST_COROSIO_POSIX + // io_uring reads files through the ring, so a read there can + // complete; elsewhere it waits behind the parked pool worker, + // which is held until the pool's own shutdown disposes of it. + std::optional f; + if constexpr (!on_ring) + { + BOOST_TEST(test::park_pool_worker(ioc, blocker, true)); + f.emplace(ioc); + BOOST_TEST(!f->open(tmp.path, file_base::read_only)); + auto on_file = [](random_access_file& rf, + destroy_count_t) -> capy::task<> { + char ch; + std::ignore = co_await rf.read_some_at( + 0, capy::mutable_buffer(&ch, 1)); + }; + spawn(ioc)(on_file(*f, counted())); + } +#endif + std::ignore = ioc.poll(); + BOOST_TEST_EQ(down, 0); + ioc.make_service(down, seen); + } + BOOST_TEST_EQ(seen, expected); + BOOST_TEST_EQ(down, expected); + } + // Posts from a service's destructor, after the teardown drain. struct late_poster : capy::execution_context::service { @@ -475,8 +596,8 @@ struct teardown_destroy_test void shutdown() override {} }; - // Work posted after the drain is destroyed with the context, not - // left queued. + // A service should not post from its destructor, but work one posts + // anyway is destroyed with the context, not left queued. void testPostAfterDrainDestroyed() { int down = 0; @@ -506,6 +627,7 @@ struct teardown_destroy_test testPoolOpIsWorkOnItsOwnContext(); testCrossContextSessionDrains(); #endif + testShutdownDestroysFramesBeforeServices(); testPostAfterDrainDestroyed(); } };