Copilot commented on code in PR #13904:
URL: https://github.com/apache/trafficserver/pull/13904#discussion_r4376560282
##########
include/mgmt/config/ConfigContext.h:
##########
@@ -186,7 +189,8 @@ class ConfigContext
/// @code
/// if (auto yaml = ctx.supplied_yaml()) { /* use yaml node */ }
/// @endcode
- /// @return copy of the supplied YAML node (cheap - YAML::Node is internally
reference-counted).
+ /// @return copy of the supplied YAML node (cheap - YAML::Node is internally
reference-counted),
+ /// or a new Undefined node on each call when none was supplied.
Review Comment:
A YAML::Node copy is a handle to the same underlying node, not an
independent value. Because the payload is shared by copies and dependents,
writing through the returned node is visible to every context that shares that
content, as the comment in ConfigContext.cc notes. Document this aliasing here
(for example, 'returns a handle to the shared node; changes are visible to all
contexts sharing this content') so callers do not assume they are isolated. The
same applies to reload_directives().
##########
src/records/unit_tests/test_ConfigRegistry.cc:
##########
@@ -309,3 +314,131 @@ TEST_CASE("ConfigRegistry is_reloadable distinguishes
handler-less entries", "[c
// An unknown key is not reloadable.
CHECK_FALSE(reg.is_reloadable("test_reloadable_unknown_xyz"));
}
+
+// ─── ConfigContext content on the execute_reload() path
───────────────────────
+//
+// No reload is in progress here, so execute_reload() hands the handler a
task-less
+// context, and the RPC-supplied content is still applied to it.
+
+namespace
+{
+ConfigContext
+reload_with(std::string const &key, YAML::Node const &content)
Review Comment:
The name reload_with does not say that the helper runs a reload and returns
the context the handler received. A name such as capture_reload_context(key,
content) would make the test setup clearer, since it is used by many of the new
cases.
##########
src/records/unit_tests/test_ConfigReloadProgress.cc:
##########
@@ -86,3 +93,55 @@ 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.
+ auto const deadline = std::chrono::steady_clock::now() +
std::chrono::seconds{15};
+ while (main_task.use_count() >= owners_with_checker &&
std::chrono::steady_clock::now() < deadline) {
Review Comment:
"The 15-second deadline is an unexplained magic number, and this polling
loop repeats the wait logic already used by the existing timeout case, which
waits 10 seconds through wait_for_sole_owner. Extract a shared helper that
waits until the owner count drops below a threshold, with the timeout passed in
or defined as a named constant. That keeps the timeout values consistent and
makes the test easier to maintain."
- fix
--
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]
