From b68af176420431ec2b8292c4a5bc7cf0a5db5373 Mon Sep 17 00:00:00 2001 From: Joe Rivera Date: Sat, 17 Oct 2026 18:46:02 -0500 Subject: [PATCH] state: json, traverse and reset without recursing once per level The walks #664 left recursive: ptree()/json()/save() and their loader, traverse(), and reset(). - ptree()/json()/save() write one level: an entry per node keyed by its full path, in pre-order, holding its value ("" for a childless empty node below the start). Those are the leaves of the nested document they used to write, in the same order. boost's ptree, write_json and read_json recurse once per nesting level, so the nested form could not be made deep-safe without a codec of our own. The loader walks every nesting level with a heap stack, so nested documents still load. - traverse() visits each node once. It traverse()d every descendant children() listed, so a node d levels down a chain was visited 2^(d-1) times (state_cluster_shard::snapshot() listed entries that many times). Pre-order with a heap stack, traverseEnter/Exit nested, the regex as before; it walks held nodes instead of re-finding them by name from the root, which recreated nodes removed meanwhile. - reset(true) clears pre-order and touches post-order as before, with a heap stack and no lock held while signaling. The recursion held every ancestor's lock during each touch, so a slot that looked its node up from the root (as the sync adapter's do) deadlocked. reset(false) drops the child map after releasing the lock, so destroyed() slots don't run under it. New state_deep_tree_test cases run on a 64 KiB stack: json/save/restore round trip 1000 deep, traverse 3000 deep, reset 100000 deep (3000 with signals), plus the visit order, reset order, flat format, legacy nested load and the lock-free signaling. state_snapshot_protocol_test checks a snapshot lists each node once. --- inc/cvc/state/state.h | 19 +- src/cvc/state/state.cpp | 264 ++++++++++++++---- src/cvc/tests/state_deep_tree_test.cpp | 231 ++++++++++++++- .../tests/state_snapshot_protocol_test.cpp | 16 ++ 4 files changed, 468 insertions(+), 62 deletions(-) diff --git a/inc/cvc/state/state.h b/inc/cvc/state/state.h index 14acd1b60..e80708525 100644 --- a/inc/cvc/state/state.h +++ b/inc/cvc/state/state.h @@ -454,10 +454,13 @@ class state : public boost::enable_shared_from_this { // Clear this node's value, data, comment and hidden flag. // - // resetChildren = true (the default) recurses, resetting the subtree in - // place and keeping every node. + // resetChildren = true (the default) resets the subtree in place and keeps + // every node: a node is cleared before the nodes under it and, with + // fireCallbacks, touched after them, a child's whole subtree before the next + // child's. A loop, not a call per level, and no lock is held while a touch + // signals. // - // resetChildren = false DESTROYS the children instead of recursing. + // resetChildren = false DESTROYS the children instead. // Beware: operator() returns state&, never the owning state_ptr, so the // parent's child map holds the only reference and dropping it deletes the // whole subtree. Any state& a caller still holds into that subtree @@ -466,6 +469,12 @@ class state : public boost::enable_shared_from_this { void reset(bool resetChildren = true, bool fireCallbacks = true); // converting to and from a boost property tree. Useful for saving and restoring state. + // + // ptree() is one level: an entry per node of this subtree, keyed by its full name from the + // root, in pre-order (children in name order), holding its value, or "" for a node below this + // one with neither a value nor children. Values only, not data(). ptree(pt) sets each + // non-empty value at its key, relative to this node, at every nesting level, so it also loads + // the nested trees older versions wrote. json(), save() and restore() are the same as JSON. boost::property_tree::ptree ptree(); operator boost::property_tree::ptree() { return ptree(); } void ptree(const boost::property_tree::ptree &); @@ -479,6 +488,10 @@ class state : public boost::enable_shared_from_this { void save(const std::string &filename); void restore(const std::string &filename); + // Call func with the full name of this node, then of each descendant whose full name matches + // `re` (all of them when it is empty), each once, in pre-order, children in name order. A + // visited node's traverseEnter fires before func sees it and its traverseExit after the nodes + // under it are visited. Descendants under a node that does not match are still visited. void traverse(traversal_unary_func func, const std::string &re = std::string()); signal traverseEnter; signal traverseExit; diff --git a/src/cvc/state/state.cpp b/src/cvc/state/state.cpp index ee63b89da..ec4476abc 100644 --- a/src/cvc/state/state.cpp +++ b/src/cvc/state/state.cpp @@ -579,38 +579,84 @@ void state::touch() { // 03/30/2012 -- Joe R. -- Resetting comment. void state::reset(bool resetChildren, bool fireCallbacks) { boost::this_thread::interruption_point(); - { - boost::mutex::scoped_lock lock(_mutex); - _value = std::string(); - _valueTypeName = std::string(); - _data = boost::any(); - _comment = std::string(); - _hidden = false; - _initialized = false; - if (resetChildren) { - BOOST_FOREACH (child_map::value_type val, _children) - val.second->reset(resetChildren, fireCallbacks); - } else { - // NOT "reset everything but the children" -- this DESTROYS them. - // - // Nothing else owns a child: operator() hands out state&, never the - // owning state_ptr, so dropping the map runs ~state over the whole - // subtree and invalidates every reference a caller still holds: - // - // state &s = root("a.b"); - // root("a").reset(false); // s dangles from here on - // - // A later operator()("a.b") therefore does not return the old node; - // it silently creates a fresh, uninitialized one. That is exactly - // what the reset(false) tests observe when they check initialized(). - // Only tests call this today. A caller that genuinely wants - // detach-and-keep must be handed the state_ptrs rather than having - // them dropped here. - _children.clear(); + // Clears a node's own fields. The caller holds its lock. + const auto clear = [](state &n) { + n._value = std::string(); + n._valueTypeName = std::string(); + n._data = boost::any(); + n._comment = std::string(); + n._hidden = false; + n._initialized = false; + }; + + if (!resetChildren) { + // NOT "reset everything but the children" -- this DESTROYS them. + // + // Nothing else owns a child: operator() hands out state&, never the + // owning state_ptr, so dropping the map runs ~state over the whole + // subtree and invalidates every reference a caller still holds: + // + // state &s = root("a.b"); + // root("a").reset(false); // s dangles from here on + // + // A later operator()("a.b") therefore does not return the old node; + // it silently creates a fresh, uninitialized one. That is exactly + // what the reset(false) tests observe when they check initialized(). + // Only tests call this today. A caller that genuinely wants + // detach-and-keep must be handed the state_ptrs rather than having + // them dropped here. + // + // The map is taken out under the lock and dropped after it, so the subtree's destroyed() + // slots do not run under this node's lock. + child_map doomed; + { + boost::mutex::scoped_lock lock(_mutex); + clear(*this); + doomed.swap(_children); } + doomed.clear(); + if (fireCallbacks) + touch(); + return; } - if (fireCallbacks) { - touch(); + + // The subtree in place, in the order the recursion had: a node is cleared before the nodes + // under it and touched after them, a child's whole subtree before the next child's. With a heap + // stack of levels, not a call per level, and no lock held while signaling: each node is cleared + // and its children snapshotted under its own lock, released before going down. The recursion + // held every ancestor's lock while touching, so a slot that read an ancestor or looked its node + // up from the root (the sync adapter's do) deadlocked. Each level holds its node, so a touch's + // ancestors stay alive even if something drops them from the tree meanwhile. + struct level { + state *node; + state_ptr held; // null for this node: the caller holds it + std::vector kids; + std::size_t next = 0; + }; + const auto open = [&clear](state &n, state_ptr held) { + level l{&n, std::move(held), {}, 0}; + boost::mutex::scoped_lock lock(n._mutex); + clear(n); + l.kids.reserve(n._children.size()); + for (const child_map::value_type &kv : n._children) + if (kv.second) + l.kids.push_back(kv.second); + return l; + }; + std::vector stack; + stack.push_back(open(*this, state_ptr())); + while (!stack.empty()) { + level &l = stack.back(); + if (l.next < l.kids.size()) { + state_ptr kid = std::move(l.kids[l.next++]); + boost::this_thread::interruption_point(); + state &k = *kid; + stack.push_back(open(k, std::move(kid))); // invalidates `l` + continue; + } + if (fireCallbacks) + l.node->touch(); + stack.pop_back(); } } @@ -627,21 +673,52 @@ boost::property_tree::ptree state::ptree() { property_tree::ptree pt; boost::this_thread::interruption_point(); - _ctx.log(6, boost::str(boost::format("%s :: %s = %s\n") % BOOST_CURRENT_FUNCTION % fullName() % - value())); - - if (!value().empty()) - pt.push_back(property_tree::ptree::value_type(fullName(), property_tree::ptree(value()))); - { - boost::mutex::scoped_lock lock(_mutex); - BOOST_FOREACH (child_map::value_type val, _children) { - boost::this_thread::interruption_point(); - property_tree::ptree child_pt = val.second->ptree(); - pt.push_back(property_tree::ptree::value_type(val.second->fullName(), child_pt)); + // One level: an entry per node, keyed by its full name, in pre-order (children in name order), + // holding the node's value -- or "" for a node other than this one with neither a value nor + // children. Those are exactly the leaves, in order, of the nested tree this used to build (each + // child's subtree under its full name, a node's value first), and the loader below reads both + // the same way. boost's ptree and its JSON writer and parser recurse once per nesting level, so + // with one level, building this, writing it as JSON, reading it back, copying and destroying it + // no longer recurse once per state level. A heap stack of levels; each node's value and + // children are snapshotted under its own lock, released before going down, and a child's full + // name is carried down as in children(). + struct level { + std::vector> kids; // with their full names + std::size_t next = 0; + }; + const char *const fn = BOOST_CURRENT_FUNCTION; + const auto open = [&pt, fn, this](state &n, const std::string &n_full, bool top) { + level l; + std::string v; + const std::string &base = n._name.empty() ? std::string() : n_full; + { + boost::mutex::scoped_lock lock(n._mutex); + v = n._value; + l.kids.reserve(n._children.size()); + for (const child_map::value_type &kv : n._children) + if (kv.second) + l.kids.emplace_back(kv.second, base.empty() ? kv.first : base + SEPARATOR + kv.first); } + _ctx.log(6, boost::str(boost::format("%s :: %s = %s\n") % fn % n_full % v)); + if (!v.empty()) + pt.push_back(property_tree::ptree::value_type(n_full, property_tree::ptree(v))); + else if (!top && l.kids.empty()) + pt.push_back(property_tree::ptree::value_type(n_full, property_tree::ptree())); + return l; + }; + std::vector stack; + stack.push_back(open(*this, fullName(), true)); + while (!stack.empty()) { + level &l = stack.back(); + if (l.next == l.kids.size()) { + stack.pop_back(); + continue; + } + std::pair kid = std::move(l.kids[l.next++]); + boost::this_thread::interruption_point(); + stack.push_back(open(*kid.first, kid.second, false)); // invalidates `l` } - return pt; } @@ -655,20 +732,31 @@ boost::property_tree::ptree state::ptree() { // 03/16/2012 -- Joe R. -- Creation. namespace { // Deserialize helper. state::ptree() (the serializer above) keys every node by -// its FULL dotted path from the root and NESTS the subtrees, so a node's value -// and its descendants live one or more levels deep. The old loop here was NOT -// recursive: it set only the top-level entries and dropped every nested child, -// so json()/restore()/from_json round-trips silently lost the whole subtree -// (only the shallowest nodes survived). Because every key is a full path from -// the root, a single flat recursive walk restores the tree correctly: for each -// entry that carries a scalar value, set it at its full-path key from `root`. +// its FULL dotted path from the root. It writes one level now, but it used to +// NEST the subtrees, so in documents written before, a node's value and its +// descendants live one or more levels deep. An older loop here set only the +// top-level entries and dropped every nested child, so json()/restore()/from_json +// round-trips silently lost the whole subtree (only the shallowest nodes +// survived). Because every key is a full path from the root, one walk over every +// entry at every level restores either form: for each entry that carries a scalar +// value, set it at its full-path key from `root`. Pre-order, with a heap stack of +// positions rather than a call per nesting level. void apply_state_ptree(cvc::state &root, const boost::property_tree::ptree &pt) { - BOOST_FOREACH (const boost::property_tree::ptree::value_type &v, pt) { + using ptree = boost::property_tree::ptree; + std::vector> stack; + stack.emplace_back(pt.begin(), pt.end()); + while (!stack.empty()) { + std::pair &at = stack.back(); + if (at.first == at.second) { + stack.pop_back(); + continue; + } + const ptree::value_type &v = *at.first++; std::string own = v.second.get_value(); if (!own.empty()) root(v.first).value(own); if (!v.second.empty()) - apply_state_ptree(root, v.second); + stack.emplace_back(v.second.begin(), v.second.end()); // invalidates `at` } } } // namespace @@ -728,12 +816,76 @@ void state::restore(const std::string &filename) { // 03/16/2012 -- Joe R. -- Creation. // 04/15/2012 -- Joe R. -- Triggering enter/exit signals. void state::traverse(traversal_unary_func func, const std::string &re) { + boost::this_thread::interruption_point(); + boost::regex expression; + bool valid = true; + if (!re.empty()) { + try { + expression = boost::regex(re.c_str()); + } catch (boost::bad_expression &) { + _ctx.log( + 2, boost::str(boost::format("%s :: invalid regex '%s'\n") % BOOST_CURRENT_FUNCTION % re)); + valid = false; // as children() does: nothing under this node matches + } + } + + // Each node once, in pre-order (children in name order): this node, then each descendant whose + // full name matches `re` (every one when it is empty). A visit fires the node's traverseEnter, + // calls func with its full name, visits what is under it, then fires its traverseExit, so a + // visited node's enter/exit pair encloses those of the visited nodes below it. A node that + // does not match is not visited, but the nodes under it still are. + // + // This used to traverse() every descendant children() listed, each of which did the same, so + // a node was visited once for every ancestor in the walk (2^(d-1) times d levels down a + // chain), one native call per level. Now a heap stack of levels. A node's children are + // snapshotted under its own lock once func has seen it, as children() was called after func, + // and are held: the walk never finds a node again by name, which recreated one removed + // meanwhile, and each level keeps its node, so a visited node's ancestors outlive its visit. + struct level { + state *node; + state_ptr held; // null for this node: the caller holds it + bool visited; // fire traverseExit when the level closes + std::vector> kids; // with their full names + std::size_t next = 0; + }; + const auto open = [](state &n, const std::string &n_full, state_ptr held, bool visited) { + level l{&n, std::move(held), visited, {}, 0}; + const std::string &base = n._name.empty() ? std::string() : n_full; + boost::mutex::scoped_lock lock(n._mutex); + l.kids.reserve(n._children.size()); + for (const child_map::value_type &kv : n._children) + if (kv.second) + l.kids.emplace_back(kv.second, base.empty() ? kv.first : base + SEPARATOR + kv.first); + return l; + }; + + const std::string full = fullName(); traverseEnter(); - func(fullName()); - std::vector ch = children(re); - BOOST_FOREACH (std::string c, ch) - instance(_ctx)(c).traverse(func, re); - traverseExit(); + func(full); + if (!valid) { + traverseExit(); + return; + } + std::vector stack; + stack.push_back(open(*this, full, state_ptr(), true)); + while (!stack.empty()) { + level &l = stack.back(); + if (l.next == l.kids.size()) { + if (l.visited) + l.node->traverseExit(); + stack.pop_back(); + continue; + } + std::pair kid = std::move(l.kids[l.next++]); + boost::this_thread::interruption_point(); + state &k = *kid.first; + const bool visit = re.empty() || boost::regex_match(kid.second, expression); + if (visit) { + k.traverseEnter(); + func(kid.second); + } + stack.push_back(open(k, kid.second, std::move(kid.first), visit)); // invalidates `l` + } } // ----------- diff --git a/src/cvc/tests/state_deep_tree_test.cpp b/src/cvc/tests/state_deep_tree_test.cpp index 85c1b8c09..c76ea5eb3 100644 --- a/src/cvc/tests/state_deep_tree_test.cpp +++ b/src/cvc/tests/state_deep_tree_test.cpp @@ -1,13 +1,17 @@ // cvc::state on deep trees. A write builds the node's full path (fullName()) and signals every // ancestor (childChanged, the root's slots first); listing descendants (children()) walks the -// subtree; destroying a node destroys its subtree. None of that may recurse once per tree -// level, or a deep tree overflows the native stack. Each body runs on a thread with a small -// fixed stack (kStackKiB, wasm's 64 KiB), so per-level recursion fails here on every platform. +// subtree; destroying a node destroys its subtree; json()/save() and their loaders, traverse() +// and reset() walk it too. None of that may recurse once per tree level, or a deep tree +// overflows the native stack. Each body runs on a thread with a small fixed stack (kStackKiB, +// wasm's 64 KiB), so per-level recursion fails here on every platform. // // For probing, CVC_STATE_DEEP_DEPTH overrides the depth and CVC_STATE_DEEP_STACK_KIB the stack // (0 runs the body on the calling thread). CVC_STATE_BENCH=1 runs the write benchmark. #include +#include +#include +#include #include #include #include @@ -16,8 +20,10 @@ #include #include #include +#include #include #include +#include #include #include #include @@ -243,6 +249,225 @@ TEST_F(StateDeepTree, WriteThroughALinkSignalsTheTargetsAncestors) { c2.disconnect(); } +// ---- json / save / restore ----------------------------------------------------- + +namespace { +// The values down the chain of "n" nodes under `top`, top first. Creates nothing. +std::vector chain_values(cvc::state &top) { + std::vector out; + for (cvc::state *node = ⊤ node; node = node->findDescendant("n")) + out.push_back(node->value()); + return out; +} + +// The entries of a json() document, in order, as (key, value). An entry that nests fails. +std::vector> json_entries(const std::string &j) { + std::stringstream ss(j); + boost::property_tree::ptree pt; + boost::property_tree::read_json(ss, pt); + std::vector> out; + for (const boost::property_tree::ptree::value_type &kv : pt) { + EXPECT_TRUE(kv.second.empty()) << kv.first << " nests"; + out.emplace_back(kv.first, kv.second.data()); + } + return out; +} +} // namespace + +TEST_F(StateDeepTree, JsonRoundTripsADeepTree) { + // Every node is keyed by its full path, so the document is quadratic in depth, and loading it + // resolves each key from the root: a shorter chain. json() and save() write it, json(j) and + // restore() read it into a fresh app. + const std::size_t n = std::min(depth(), 1000); + cvc::state *node = &root("saved"); + node->value("0"); + for (std::size_t i = 1; i <= n; ++i) { + node = &(*node)("n"); + node->value(std::to_string(i)); + } + const std::vector expected = chain_values(root("saved")); + ASSERT_EQ(expected.size(), n + 1); + const std::string file = + (std::filesystem::temp_directory_path() / "state_deep_tree_save.json").string(); + std::string j; + on_small_stack([&] { + j = root("saved").json(); + root("saved").save(file); + }); + cvc::app from_json, from_file; + cvc::state &r1 = cvc::state::instance(from_json); + cvc::state &r2 = cvc::state::instance(from_file); + on_small_stack([&] { + r1.json(j); + r2.restore(file); + }); + std::error_code ec; + std::filesystem::remove(file, ec); + EXPECT_EQ(chain_values(r1("saved")), expected); + EXPECT_EQ(chain_values(r2("saved")), expected); +} + +TEST_F(StateDeepTree, JsonIsOneLevelOfFullPaths) { + // One entry per node, keyed by its full path, in pre-order (children in name order): the + // node's value, or "" for a node with neither a value nor children. Those are the leaves of + // the nested document json() used to write, which still loads. + root("j").value("1"); + root("j.b.c").value("3"); + root("j.b.e").value("5"); + root("j.d"); + const std::vector> expected{ + {"j", "1"}, {"j.b.c", "3"}, {"j.b.e", "5"}, {"j.d", ""}}; + EXPECT_EQ(json_entries(root("j").json()), expected); + boost::property_tree::ptree pt = root("j").ptree(); + ASSERT_EQ(pt.size(), expected.size()); + for (const boost::property_tree::ptree::value_type &kv : pt) + EXPECT_TRUE(kv.second.empty()) << kv.first << " nests"; + + const std::string nested = + R"({"j": "1", "j.b": {"j.b.c": {"j.b.c": "3"}, "j.b.e": {"j.b.e": "5"}}, "j.d": ""})"; + cvc::app other; + cvc::state &r = cvc::state::instance(other); + r.json(nested); + EXPECT_EQ(r("j").value(), "1"); + EXPECT_EQ(r("j.b.c").value(), "3"); + EXPECT_EQ(r("j.b.e").value(), "5"); + // The loader skips empty values, so j.d is not recreated. + EXPECT_EQ(json_entries(r("j").json()), (std::vector>{ + {"j", "1"}, {"j.b.c", "3"}, {"j.b.e", "5"}})); +} + +// ---- traverse ------------------------------------------------------------------ + +TEST_F(StateDeepTree, TraverseVisitsEachNodeOnce) { + // Pre-order, children in name order, each node once. A node's traverseEnter fires before func + // sees it and its traverseExit after everything under it. With a pattern: the node traverse() + // was called on, then the descendants whose full names match; the walk still goes under the + // ones that do not. + const std::vector paths{"t", "t.b", "t.b.c", "t.b.d", "t.e"}; + std::vector seen; + std::vector conns; + for (const std::string &p : paths) { + const std::string tag = p.substr(p.rfind('.') + 1); + conns.push_back(root(p).traverseEnter.connect([&seen, tag] { seen.push_back("+" + tag); })); + conns.push_back(root(p).traverseExit.connect([&seen, tag] { seen.push_back("-" + tag); })); + } + const auto record = [&seen](std::string name) { seen.push_back(name); }; + root("t").traverse(record); + EXPECT_EQ(seen, (std::vector{"+t", "t", "+b", "t.b", "+c", "t.b.c", "-c", "+d", + "t.b.d", "-d", "-b", "+e", "t.e", "-e", "-t"})); + seen.clear(); + root("t").traverse(record, ".*[ce]"); + EXPECT_EQ(seen, + (std::vector{"+t", "t", "+c", "t.b.c", "-c", "+e", "t.e", "-e", "-t"})); + for (boost::signals2::connection &c : conns) + c.disconnect(); +} + +TEST_F(StateDeepTree, TraverseADeepTree) { + // func gets every node's full name, quadratic in depth by nature: a shorter chain. + const std::size_t n = std::min(depth(), 3000); + cvc::state &top = root("walked"); + chain(top, n); + std::size_t visits = 0, enters = 0, exits = 0; + std::string last; + auto c1 = top.traverseEnter.connect([&] { ++enters; }); + auto c2 = top.traverseExit.connect([&] { ++exits; }); + on_small_stack([&] { + top.traverse([&](std::string name) { + ++visits; + last = std::move(name); + }); + }); + EXPECT_EQ(visits, n + 1); + EXPECT_EQ(last, repeat_path("walked", n)); + EXPECT_EQ(enters, 1u); + EXPECT_EQ(exits, 1u); + c1.disconnect(); + c2.disconnect(); +} + +// ---- reset --------------------------------------------------------------------- + +TEST_F(StateDeepTree, ResetADeepTree) { + cvc::state &top = root("cleared"); + top.value("top"); + cvc::state &leaf = chain(top, depth()); + leaf.value("leaf"); + on_small_stack([&] { top.reset(true, false); }); + EXPECT_EQ(top.value(), ""); + EXPECT_EQ(leaf.value(), ""); + EXPECT_FALSE(leaf.initialized()); + EXPECT_FALSE(leaf.parent()->initialized()); + // reset(true) keeps every node. + EXPECT_EQ(top.findDescendant(repeat_path("n", depth() - 1)), &leaf); +} + +TEST_F(StateDeepTree, ResetADeepTreeSignalsLeafFirst) { + // Each touch signals every ancestor, quadratic in depth: a shorter chain. + const std::size_t n = std::min(depth(), 3000); + cvc::state &top = root("touched"); + chain(top, n).value("leaf"); + std::vector heard; + auto c = root.childChanged.connect([&](const std::string &p) { heard.push_back(p); }); + on_small_stack([&] { top.reset(); }); + c.disconnect(); + ASSERT_EQ(heard.size(), n + 1); + EXPECT_EQ(heard.front(), repeat_path("touched", n)); + EXPECT_EQ(heard.back(), "touched"); +} + +TEST_F(StateDeepTree, ResetClearsTopDownAndTouchesBottomUp) { + // reset(true) clears a node before the nodes under it and touches it after them, a subtree at + // a time: when c (the first leaf) is touched, its later sibling d is not cleared yet. + const std::vector paths{"r", "r.b", "r.b.c", "r.b.d", "r.e"}; + for (const std::string &p : paths) + root(p).value(p); + std::vector seen; + std::vector conns; + for (const std::string &p : paths) + conns.push_back(root(p).valueChanged.connect([&seen, p] { seen.push_back(p); })); + conns.push_back( + root.childChanged.connect([&](const std::string &p) { seen.push_back("^" + p); })); + cvc::state &d = root("r.b.d"); + std::string d_at_c; + conns.push_back(root("r.b.c").valueChanged.connect([&] { d_at_c = d.value(); })); + root("r").reset(); + EXPECT_EQ(seen, (std::vector{"r.b.c", "^r.b.c", "r.b.d", "^r.b.d", "r.b", "^r.b", + "r.e", "^r.e", "r", "^r"})); + EXPECT_EQ(d_at_c, "r.b.d"); + for (boost::signals2::connection &c : conns) + c.disconnect(); +} + +TEST_F(StateDeepTree, ResetSignalsWithoutTheAncestorsLocks) { + // A slot that finds its node from the root (as the sync adapter's do) or reads an ancestor + // takes the ancestors' locks, so reset() must not hold them while it signals. Nor while + // reset(false) destroys the subtree and fires its destroyed() slots. + root("w.b.c").value("c"); + root("w").value("w"); + std::string at_c = "unset"; + auto c1 = root("w.b.c").valueChanged.connect( + [&] { at_c = root("w.b.c").value() + "|" + root("w.b").value() + "|" + root("w").value(); }); + root("z.k").value("k"); + root("z").value("z"); + std::string at_destroy = "unset"; + auto c2 = root("z.k").destroyed.connect([&] { at_destroy = root("z").value(); }); + boost::thread t([&] { + root("w").reset(); + root("z").reset(false); + }); + if (!t.try_join_for(boost::chrono::seconds(60))) { + // Deadlocked on a lock it holds itself; nothing can unblock it. + std::fprintf(stderr, "reset() deadlocked: it signals while holding an ancestor's lock\n"); + std::fflush(stderr); + std::_Exit(1); + } + EXPECT_EQ(at_c, "||"); // its ancestors were cleared first + EXPECT_EQ(at_destroy, ""); + c1.disconnect(); + c2.disconnect(); +} + TEST_F(StateDeepTree, WriteBench) { if (env_size("CVC_STATE_BENCH", 0) != 1) { SUCCEED(); diff --git a/src/cvc/tests/state_snapshot_protocol_test.cpp b/src/cvc/tests/state_snapshot_protocol_test.cpp index 42b9604c2..89f7cddd6 100644 --- a/src/cvc/tests/state_snapshot_protocol_test.cpp +++ b/src/cvc/tests/state_snapshot_protocol_test.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -87,6 +88,21 @@ TEST_F(StateSnapshotProtocolTest, SnapshotWithPrefixFilter) { shard.detach(); } +TEST_F(StateSnapshotProtocolTest, SnapshotListsEachNodeOnce) { + // The snapshot walks the subtree with state::traverse(), which visits each node once. It used + // to visit a node once per ancestor in the walk, so a.b.c was listed (and encoded) twice. + cvc::app ctx; + cvc::state_cluster_shard shard(ctx, "clust", "node_A"); + shard.attach(); + cvc::state::instance(ctx)("a.b.c").value(std::string("deep")); + cvc::state::instance(ctx)("a.d").value(std::string("x")); + std::map seen; + for (const auto &e : shard.snapshot("a")) + ++seen[e.path]; + EXPECT_EQ(seen, (std::map{{"a", 1}, {"a.b", 1}, {"a.b.c", 1}, {"a.d", 1}})); + shard.detach(); +} + TEST_F(StateSnapshotProtocolTest, IpcSnapshotRequestResponse) { std::string sock = make_socket_path("snap_ipc"); sockets.push_back(sock);