Skip to content

initialize: save the state of the policies it initializes, and only those - #105

Closed
jll63 wants to merge 1 commit into
boostorg:developfrom
jll63:fix/transaction-save-scope
Closed

initialize: save the state of the policies it initializes, and only those#105
jll63 wants to merge 1 commit into
boostorg:developfrom
jll63:fix/transaction-save-scope

Conversation

@jll63

@jll63 jll63 commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator

(Written by Claude Code, on behalf of @jll63.)

Addresses two problems with the transaction #95 introduced. Both come from the same
decision — copying the registry's whole policies tuple — and one change fixes both.

The comment justified the width as "restoring an untouched state is harmless, and simpler
than picking". It is not harmless.

1. It reverts configuration, not just derived state

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 (fast_perfect_hash.hpp:293). 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:

before: handler still installed after the failed initialize = 1
after:  handler still installed after the failed initialize = 0

2. It made every policy state copy-constructible, silently

Including states of policies that have no initialize at all. That rules out a state
holding 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.

The change

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:

error: static assertion failed: the `state` of a policy that defines `initialize` must be
copyable: initialize() saves it, and puts it back if a policy throws

What this does not do

It does not remove the copy of the control vector and the vptr vector from the success
path. fast_perfect_hash and vptr_vector 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.

The restore is also 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; separate issue.

Test

test/test_initialize_policy_state_scope.cpp covers both halves at once:

  • it fails to compile without this change, because its configuration policy's state is
    deliberately move-only (static_asserts in the test pin that down, so it cannot decay
    into proving nothing);
  • with the state made copyable to isolate the other half, it fails 1 != 2 on the
    rolled-back generation counter.

Deliberately a new file rather than an addition to test_initialize_transaction.cpp, which
#101 rewrites — this way the two do not conflict.

Verification

  • CMake/Ninja, gcc, Debug: 161/161 ctest pass.
  • b2 toolset=gcc: 644 targets, no failures.
  • Both halves confirmed red before, green after.
  • clang-format-22 applied.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JQa4fuiwcfsheZYTCyfPPr

…hose

boostorg#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 boostorg#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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JQa4fuiwcfsheZYTCyfPPr
@cppalliance-bot

Copy link
Copy Markdown

An automated preview of the documentation is available at https://105.openmethod.prtest3.cppalliance.org/libs/openmethod/doc/html/index.html

If more commits are pushed to the pull request, the docs will rebuild at the same URL.

2026-09-12 13:41:54 UTC

@jll63

jll63 commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator Author

(Written by Claude Code, on behalf of @jll63.)

Closing in favour of #101, which now carries this change as its third commit, cherry-picked
unmodified (test/test_initialize_policy_state_scope.cpp included).

Keeping the two separate would have put a conflict between them: this branch rewrote
registry_state_transaction's restore element-wise, and #101 needed to touch the same
destructor to stop it terminating when a policy state's move-assignment throws. Both fixes
now sit side by side there, as two static_asserts in the same class.

No work is lost — nothing here is abandoned, only relocated.

@jll63 jll63 closed this Sep 12, 2026
@codecov

codecov Bot commented Sep 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.55%. Comparing base (5ec487c) to head (bd65eb2).
⚠️ Report is 10 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #105      +/-   ##
===========================================
+ Coverage    93.46%   93.55%   +0.09%     
===========================================
  Files           22       22              
  Lines         1653     1692      +39     
  Branches       500      509       +9     
===========================================
+ Hits          1545     1583      +38     
  Misses          64       64              
- Partials        44       45       +1     
Files with missing lines Coverage Δ
include/boost/openmethod/initialize.hpp 92.83% <100.00%> (+0.17%) ⬆️

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba56242...bd65eb2. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jll63
jll63 deleted the fix/transaction-save-scope branch September 13, 2026 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants