Copilot commented on code in PR #13904:
URL: https://github.com/apache/trafficserver/pull/13904#discussion_r4393329649
##########
include/mgmt/config/ConfigContext.h:
##########
@@ -90,20 +90,23 @@ class RecordTriggeredReloadContinuation;
class ConfigContext
{
public:
- ConfigContext();
+ ConfigContext() = default;
explicit ConfigContext(std::shared_ptr<ConfigReloadTask> t, std::string_view
description = "", std::string_view filename = "");
- ~ConfigContext();
+ ~ConfigContext() = default;
// Copy only - move is intentionally suppressed.
- // ConfigContext holds a weak_ptr (cheap to copy) and a YAML::Node
(ref-counted).
+ // ConfigContext holds a weak_ptr to the task and a shared_ptr to the
RPC-supplied
+ // YAML and reload directives, so a copy is at most two reference-count
increments.
+ // Keep it that small: it is passed by value everywhere (see the size check
in
+ // ConfigContext.cc).
// Suppressing move ensures that std::move(ctx) silently copies, keeping the
// original valid. This is critical for execute_reload()'s post-handler
check:
// if a handler defers work (e.g. LogConfig), the original ctx must remain
// valid so is_terminal() can detect the non-terminal state and emit a
warning.
- ConfigContext(ConfigContext const &);
- ConfigContext &operator=(ConfigContext const &);
+ ConfigContext(ConfigContext const &) = default;
+ ConfigContext &operator=(ConfigContext const &) = default;
Review Comment:
Copy-assignment now rebinds the context's payload rather than writing
through the shared `YAML::Node`, which changes behavior from before. The
comment above these defaulted operations does not mention this. Add a note that
assignment replaces this context's content without modifying other copies, so
callers that relied on the old write-through behavior can find the change.
##########
src/records/unit_tests/test_ConfigReloadProgress.cc:
##########
@@ -86,3 +99,52 @@ TEST_CASE("Progress checker releases a task that timed out",
"[config][reload][l
REQUIRE(wait_for_sole_owner(task, std::chrono::seconds{10}));
REQUIRE(task->get_state() == ConfigReloadTask::State::TIMEOUT);
}
+
+// execute_reload() gives a handler a context with a task only while a reload
is in
+// progress, and starting one needs ET_TASK for its progress checker, so this
case
+// lives in this binary.
+TEST_CASE("A dependent context inherits its parent's RPC content",
"[config][reload][context]")
+{
+ static std::optional<ConfigContext> parent;
+ static std::optional<ConfigContext> child;
+
+ std::string token;
+ REQUIRE(ReloadCoordinator::Get_Instance().prepare_reload(token,
"test-dep-").is_ok());
+ auto const main_task = ReloadCoordinator::Get_Instance().get_current_task();
+ REQUIRE(main_task);
+ // prepare_reload() started a progress checker, which holds one of these.
+ auto const owners_with_checker = main_task.use_count();
+
+ auto ® = config::ConfigRegistry::Get_Instance();
+ reg.register_config("test_dependent_payload", "", "",
+ [](ConfigContext ctx) {
+ parent.emplace(ctx);
+ child.emplace(ctx.add_dependent_ctx("dependent"));
+ child->complete();
+ ctx.complete();
+ },
+ config::ConfigSource::FileAndRpc, {});
+ reg.set_passed_config("test_dependent_payload", YAML::Load("{_reload: {id:
foo}, data: x}"));
+ reg.execute_reload("test_dependent_payload");
+
+ REQUIRE(parent.has_value());
+ REQUIRE(child.has_value());
+ REQUIRE(static_cast<bool>(*parent));
+ REQUIRE(static_cast<bool>(*child));
+
+ YAML::Node const content = child->supplied_yaml();
+ REQUIRE(content);
+ REQUIRE(content["data"].as<std::string>() == "x");
+ YAML::Node const directives = child->reload_directives();
+ REQUIRE(directives);
+ REQUIRE(directives["id"].as<std::string>() == "foo");
+ REQUIRE(content.is(parent->supplied_yaml()));
+ REQUIRE(directives.is(parent->reload_directives()));
+
+ child.reset();
+ parent.reset();
+
+ // Like the cases above, don't leave the checker running past the test. Same
bound as the
+ // terminal case: the checker confirms a terminal state about 7 s after it
starts.
+ REQUIRE(wait_for_owners_below(main_task, owners_with_checker,
std::chrono::seconds{15}));
Review Comment:
The cleanup at lines 144-149 is skipped if any REQUIRE above it fails,
because Catch2 throws. The progress checker then keeps running past the test,
and the static optionals `parent` and `child` keep state for later cases. Move
the reset and the wait into a scope guard (an RAII object whose destructor
resets both optionals and waits on `main_task`) so cleanup runs on every exit
path. This keeps the intent stated in the comment at line 147.
##########
src/records/unit_tests/test_ConfigReloadProgress.cc:
##########
@@ -86,3 +99,52 @@ TEST_CASE("Progress checker releases a task that timed out",
"[config][reload][l
REQUIRE(wait_for_sole_owner(task, std::chrono::seconds{10}));
REQUIRE(task->get_state() == ConfigReloadTask::State::TIMEOUT);
}
+
+// execute_reload() gives a handler a context with a task only while a reload
is in
+// progress, and starting one needs ET_TASK for its progress checker, so this
case
+// lives in this binary.
+TEST_CASE("A dependent context inherits its parent's RPC content",
"[config][reload][context]")
+{
+ static std::optional<ConfigContext> parent;
+ static std::optional<ConfigContext> child;
+
+ std::string token;
+ REQUIRE(ReloadCoordinator::Get_Instance().prepare_reload(token,
"test-dep-").is_ok());
+ auto const main_task = ReloadCoordinator::Get_Instance().get_current_task();
+ REQUIRE(main_task);
+ // prepare_reload() started a progress checker, which holds one of these.
+ auto const owners_with_checker = main_task.use_count();
+
+ auto ® = config::ConfigRegistry::Get_Instance();
+ reg.register_config("test_dependent_payload", "", "",
+ [](ConfigContext ctx) {
+ parent.emplace(ctx);
+ child.emplace(ctx.add_dependent_ctx("dependent"));
+ child->complete();
+ ctx.complete();
+ },
+ config::ConfigSource::FileAndRpc, {});
+ reg.set_passed_config("test_dependent_payload", YAML::Load("{_reload: {id:
foo}, data: x}"));
+ reg.execute_reload("test_dependent_payload");
+
+ REQUIRE(parent.has_value());
+ REQUIRE(child.has_value());
+ REQUIRE(static_cast<bool>(*parent));
+ REQUIRE(static_cast<bool>(*child));
Review Comment:
The cleanup at lines 144-149 is skipped if any REQUIRE above it fails,
because Catch2 throws. The progress checker then keeps running past the test,
and the static optionals `parent` and `child` keep state for later cases. Move
the reset and the wait into a scope guard (an RAII object whose destructor
resets both optionals and waits on `main_task`) so cleanup runs on every exit
path. This keeps the intent stated in the comment at line 147.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
