From bd65eb2e2204e5b50cee53f7e4542e6ee4260dba Mon Sep 17 00:00:00 2001 From: Jean-Louis Leroy Date: Sat, 12 Sep 2026 09:35:28 -0400 Subject: [PATCH] initialize: save the state of the policies it initializes, and only those #95 made initialize() transactional by copying the registry's whole `policies` tuple and putting it back if anything throws. The comment justified the width - "restoring an untouched state is harmless, and simpler than picking" - but it is not harmless, in two ways. A state that no `initialize` writes to is not derived data the call is about to replace; it is configuration the caller owns. The error handler is the case that bites: it is *called* from inside the transaction window by design, since fast_perfect_hash reports a search failure through it. A handler that disarms itself - installs another handler with set() before throwing, so the failure is reported once - had that undone on the way out. Reported here as a change to the handler surviving or not: before: handler still installed after the failed initialize = 1 after: handler still installed after the failed initialize = 0 And copying the tuple made copy-constructibility a hard requirement of every policy state in the registry, including states of policies that have no `initialize` at all. That rules out a state holding a std::mutex, std::unique_ptr, std::atomic or std::ostringstream - and an ostringstream is exactly what an `output` policy written to the documented state pattern holds. It compiled before #95 and stopped compiling after, with 29 lines of deleted-copy-constructor diagnostics on gcc 13 (207 on clang 18) naming the whole policy list 21 times. Filter the saved states to the policies whose `initialize` will actually run, using the same has_initialize test initialize_policy makes, so the two cannot disagree about which policies run. The saved tuple is now a subset of the registry's, so it is filled and restored element-wise. A static_assert spells out the requirement that remains, in the manner of the one preamble.hpp already carries for duplicate state types. One correction to how this was reported to me: it does *not* remove the copy of the control vector and the vptr vector from the success path. Those two policies define `initialize`, so their states must still be saved - that is what makes a rollback possible. What is no longer copied is the states of the policies that do not initialize: in default_registry, the error handler's std::function and the output policy's state. Not addressed: the restore is still a move-assignment in a `noexcept` destructor, so a policy state whose move-assignment throws still terminates. Stock registries are nothrow, so nothing in the suite can show it. The test covers both halves. It fails to compile without this change, because its configuration policy's state is deliberately move-only; with the state made copyable to isolate the other half, it fails 1 != 2 on the rolled-back generation counter. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JQa4fuiwcfsheZYTCyfPPr --- include/boost/openmethod/initialize.hpp | 94 +++++++++++--- test/test_initialize_policy_state_scope.cpp | 132 ++++++++++++++++++++ 2 files changed, 211 insertions(+), 15 deletions(-) create mode 100644 test/test_initialize_policy_state_scope.cpp diff --git a/include/boost/openmethod/initialize.hpp b/include/boost/openmethod/initialize.hpp index 7f4c4e8c..e4b7dbc7 100644 --- a/include/boost/openmethod/initialize.hpp +++ b/include/boost/openmethod/initialize.hpp @@ -124,27 +124,83 @@ struct initialize_policies> { } }; +// Selects the policies whose state the transaction has to save: those that +// have a `state`, *and* whose `initialize` will actually be called for this +// Context and Options - the same test initialize_policy makes, so the two +// cannot disagree about which policies run. +template +struct policy_state_is_volatile_q { + template + using fn = mp11::mp_bool< + has_policy_state::value && + has_initialize>; +}; + // Saves the policies' states on construction, and puts them back on // destruction unless commit() was called - so a policy's `initialize` that // throws, after itself or another policy has already written to shared // state, leaves the registry as it was. The registry's mutable state is one -// variable, registry_state::st; its `policies` tuple is copied -// whole, states of policies without an `initialize` included, since -// restoring an untouched state is harmless, and simpler than picking. The -// other members need no saving: initialize() only reads the class and -// method lists, and write_global_data() replaces dispatch_data at commit -// time only, after which nothing can throw - on rollback it still holds the -// previous tables, which the classes' static_vptrs point into. (That is -// also why a copy could not stand in for it: it would be another buffer.) -template +// variable, registry_state::st. Its other members need no saving: +// initialize() only reads the class and method lists, and write_global_data() +// replaces dispatch_data at commit time only, after which nothing can throw - +// on rollback it still holds the previous tables, which the classes' +// static_vptrs point into. (That is also why a copy could not stand in for +// it: it would be another buffer.) +// +// Only the states of the policies that are about to be initialized are saved. +// Copying the `policies` tuple whole is simpler, but it is not harmless: a +// state that no `initialize` touches is not derived data this call is about to +// replace, it is configuration the caller owns. The error handler is the case +// that bites - it is *called* from inside the window, by design +// (fast_perfect_hash reports a search failure through it), so a handler that +// disarms itself with set() before throwing would have that undone on the way +// out. Saving wide also forces every policy state in the registry to be +// copyable, including those of policies that have no `initialize` at all - +// which rules out the std::ostringstream an `output` policy written to the +// documented state pattern naturally holds - and copies each of them, vectors +// and all, on every successful initialize(). +template class registry_state_transaction { + using policy_fns = mp11::mp_transform_q< + policy_fn_q, typename Registry::policy_list>; + using saved_states = mp11::mp_transform< + policy_state_t, + mp11::mp_filter_q< + policy_state_is_volatile_q, policy_fns>>; + using saved_type = mp11::mp_apply; + + static_assert( + mp11::mp_all_of::value, + "the `state` of a policy that defines `initialize` must be copyable: " + "initialize() saves it, and puts it back if a policy throws"); + + // Element-wise: `saved` holds a subset of the registry's tuple. + template + struct each; + + template + struct each> { + static void save(detail::tuple& to) { + (..., + (detail::get(to) = + detail::get(Registry::state().policies))); + } + + static void restore(detail::tuple& from) { + (..., + (detail::get(Registry::state().policies) = + std::move(detail::get(from)))); + } + }; + public: - registry_state_transaction() : saved(Registry::state().policies) { + registry_state_transaction() { + each::save(saved); } ~registry_state_transaction() { if (!committed) { - Registry::state().policies = std::move(saved); + each::restore(saved); } } @@ -157,7 +213,7 @@ class registry_state_transaction { } private: - typename registry_state_type::policies_type saved; + saved_type saved; bool committed = false; }; @@ -1878,7 +1934,9 @@ void registry::compiler::write_global_data() { ++tr << rflush(4, dispatch_data_size) << " " << gv_iter << " end\n"; - detail::registry_state_transaction transaction; + detail::registry_state_transaction< + registry, compiler, std::tuple> + transaction; detail::initialize_policies::fn(*this, options); transaction.commit(); @@ -2222,8 +2280,14 @@ void registry::compiler::print_slots() { //! call: no static v-table pointer, `next` pointer, dispatch table or policy //! state is modified. The registry is nonetheless marked as not initialized, //! since that state does not reflect the current registrations; `initialize` -//! must be called again, successfully, before any method is called. Policy -//! states are restored from a copy, so a policy's `state` must be copyable. +//! must be called again, successfully, before any method is called. +//! +//! Only the policies that define `initialize` have their `state` saved and +//! restored, so only those states have to be copyable. A state that no +//! `initialize` writes to is configuration, not derived data, and is left +//! alone: an @ref error_handler policy is *called* during `initialize`, and a +//! handler that changes the configuration - installing a different handler +//! with `set`, say - keeps that change whether the call succeeds or throws. //! //! @par Example //! diff --git a/test/test_initialize_policy_state_scope.cpp b/test/test_initialize_policy_state_scope.cpp new file mode 100644 index 00000000..155bba89 --- /dev/null +++ b/test/test_initialize_policy_state_scope.cpp @@ -0,0 +1,132 @@ +// Copyright (c) 2017-2026 Jean-Louis Leroy +// Distributed under the Boost Software License, Version 1.0. +// See accompanying file LICENSE_1_0.txt +// or copy at http://www.boost.org/LICENSE_1_0.txt) + +// initialize() saves and restores the state of the policies it initializes, +// and only those. A state that no `initialize` writes to is configuration the +// caller owns, not derived data the call is about to replace, so a failed +// initialize() must leave it alone - the error handler is the case that +// matters, since it is called from inside the transaction window by design. +// +// Two consequences, both checked here: such a state survives a rollback, and +// it does not have to be copyable - which it would if the transaction copied +// the whole policy tuple, ruling out the std::ostringstream an `output` policy +// written to the documented state pattern naturally holds. + +#include +#include + +#define BOOST_TEST_MODULE initialize_policy_state_scope +#include + +#include "test_util.hpp" + +#include +#include +#include +#include +#include + +using namespace boost::openmethod; + +// Configuration: it has a `state`, and deliberately no `initialize`. The state +// is move-only, like the std::ostringstream it carries - the transaction must +// not require it to be copyable. +struct config_policy { + using category = config_policy; + + template + struct fn { + struct state { + state() = default; + state(const state&) = delete; + auto operator=(const state&) -> state& = delete; + + int generation = 0; + std::ostringstream os; + }; + }; +}; + +// Writes to its own state and to the configuration policy's - the way an error +// handler called from inside the window changes the handler it installs - then +// throws on demand. +struct explosive_policy { + using category = explosive_policy; + + template + struct fn { + struct state { + int generation = 0; + }; + + inline static bool armed = false; + + template + static void initialize(const Context&, const std::tuple&) { + ++Registry::template state().generation; + ++Registry::template state().generation; + + if (armed) { + throw std::runtime_error("boom"); + } + } + }; +}; + +template +struct test_registry : + test_registry_::template with {}; + +using test_reg = test_registry<__COUNTER__>; +using config_state = config_policy::fn::state; +using explosive = explosive_policy::fn; + +// The point of the exercise: a policy that has no `initialize` may hold a +// state the transaction could not copy even if it wanted to. If this ever +// becomes copyable the test below stops proving anything. +static_assert(!std::is_copy_constructible_v); +static_assert(!std::is_copy_assignable_v); + +struct Animal { + virtual ~Animal() = default; +}; + +struct Dog : Animal {}; + +struct BOOST_OPENMETHOD_ID(poke); + +using poke = method< + BOOST_OPENMETHOD_ID(poke), auto(virtual_)->std::string, test_reg>; + +auto poke_animal(Animal&) -> std::string { + return "silence"; +} + +BOOST_AUTO_TEST_CASE(config_state_survives_a_failed_initialize) { + BOOST_OPENMETHOD_REGISTER(use_classes); + BOOST_OPENMETHOD_REGISTER(poke::override); + + initialize(); + BOOST_TEST(test_reg::state().generation == 1); + BOOST_TEST(test_reg::state().generation == 1); + + explosive::armed = true; + BOOST_CHECK_THROW(initialize(), std::runtime_error); + explosive::armed = false; + + // The initializing policy's own state is derived data: rolled back, so the + // second run's increment is undone. + BOOST_TEST(test_reg::state().generation == 1); + + // The configuration policy's is not: the write made inside the window + // stands, exactly as an error handler that disarms itself before throwing + // would expect. + BOOST_TEST(test_reg::state().generation == 2); + + // And the registry still works. + initialize(); + Dog dog; + BOOST_TEST(poke::fn(dog) == "silence"); +}