brbzull0 commented on code in PR #13904:
URL: https://github.com/apache/trafficserver/pull/13904#discussion_r4399264562
##########
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:
The PR description covers this under "What changes": copy-assignment rebinds
the payload, so other copies keep their content. I'm leaving the header as it
is so the approved head doesn't 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));
Review Comment:
Keeping the cleanup inline, as the two cases above do: they also `REQUIRE`
before their wait. `parent` and `child` are local to this test case, so a
failed `REQUIRE` leaves nothing for other cases, and the test has already
failed.
--
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]
