Copilot commented on code in PR #14005:
URL: https://github.com/apache/trafficserver/pull/14005#discussion_r4486040804
##########
src/proxy/http/HttpSM.cc:
##########
@@ -284,8 +284,13 @@ HttpSM::~HttpSM()
{
http_parser_clear(&http_parser);
- // coverity[exn_spec_violation] - release() only does ref counting and
delete on POD types
- HttpConfig::release(t_state.http_config_param);
+ if (_http_config_lease) {
+ _http_config_lease.reset();
+ } else {
+ // Callers that set http_config_param themselves, such as unit tests, hand
over an acquired reference.
+ // coverity[exn_spec_violation] - release() only does ref counting and
delete on POD types
+ HttpConfig::release(t_state.http_config_param);
Review Comment:
With the new init path, if `get_leased(HttpConfig::config_id())` returns an
empty `shared_ptr` (e.g., slot unset/invalid), then `t_state.http_config_param`
becomes `nullptr` and the destructor will fall into the `else` branch and call
`HttpConfig::release(nullptr)`. To avoid a potential crash/UB, either (a) make
`init()` fall back to `HttpConfig::acquire()` when the lease is empty, or (b)
guard the `release()` call with `t_state.http_config_param != nullptr`.
##########
src/iocore/eventsystem/ConfigProcessor.cc:
##########
@@ -216,6 +227,99 @@ ConfigProcessor::release(unsigned int id, ConfigInfo *info)
}
}
+struct ConfigProcessor::ThreadPins {
+ struct Pin {
+ uint64_t generation = 0;
+ std::shared_ptr<ConfigInfo> config;
+ bool listed = false;
+ };
+
+ std::array<Pin, MAX_CONFIGS> pins;
+ // Slots this thread has pinned at some point, so the per-loop sweep skips
the rest.
+ std::array<uint8_t, MAX_CONFIGS> used{};
+ size_t used_count = 0;
+};
Review Comment:
`used` stores slot indices in `uint8_t`, which silently assumes
`(MAX_CONFIGS - 1) <= 255`. If `MAX_CONFIGS` ever exceeds 256, indices will
truncate and `release_stale_pins()` will sweep the wrong slots. Prefer storing
indices as `uint16_t`/`unsigned int`, or add a `static_assert(MAX_CONFIGS <=
256)` to make the constraint explicit.
##########
src/iocore/eventsystem/ConfigProcessor.cc:
##########
@@ -121,6 +124,13 @@ class ConfigInfoReleaser : public Continuation
ConfigInfo *m_info;
};
+ConfigProcessor::ConfigProcessor()
+{
+ // release_stale_pins() runs only between events, which is what makes
get_pinned() results safe to use.
+ ethread_loop_hooks = {&ConfigProcessor::enable_thread_pins,
&ConfigProcessor::release_stale_pins,
+ &ConfigProcessor::disable_thread_pins};
Review Comment:
`ethread_loop_hooks` is a single global hook set that gets overwritten here
unconditionally, which prevents other subsystems from registering hooks (or
risks last-writer-wins behavior). Consider switching to an explicit
registration API (e.g., add/remove callbacks) or at least assert/log if hooks
are already set to avoid silently clobbering an existing hook configuration.
##########
include/proxy/http/HttpTransact.h:
##########
@@ -839,7 +841,8 @@ class HttpTransact
void
init()
{
- parent_params = ParentConfig::acquire();
+ parent_params_lease = configProcessor.get_leased(ParentConfig::m_id);
Review Comment:
This reaches into `ParentConfig::m_id` directly, which bypasses the new
`config_id()` accessor pattern introduced across other config types in this PR.
Recommend adding `ParentConfig::config_id()` (if not already present) and
calling that here to keep slot lookup encapsulated and consistent.
##########
src/proxy/http/HttpSM.cc:
##########
@@ -327,7 +332,8 @@ HttpSM::init(bool from_early_data)
ATS_PROBE1(milestone_sm_start, sm_id);
t_state.state_machine = this;
- t_state.http_config_param = HttpConfig::acquire();
+ _http_config_lease =
configProcessor.get_leased(HttpConfig::config_id());
+ t_state.http_config_param = static_cast<HttpConfigParams
*>(_http_config_lease.get());
Review Comment:
With the new init path, if `get_leased(HttpConfig::config_id())` returns an
empty `shared_ptr` (e.g., slot unset/invalid), then `t_state.http_config_param`
becomes `nullptr` and the destructor will fall into the `else` branch and call
`HttpConfig::release(nullptr)`. To avoid a potential crash/UB, either (a) make
`init()` fall back to `HttpConfig::acquire()` when the lease is empty, or (b)
guard the `release()` call with `t_state.http_config_param != nullptr`.
##########
src/iocore/eventsystem/unit_tests/test_ConfigPins.cc:
##########
@@ -0,0 +1,177 @@
+/** @file
+
+ ConfigProcessor per-thread pins and leases.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include <catch2/catch_test_macros.hpp>
+#include <catch2/reporters/catch_reporter_event_listener.hpp>
+#include <catch2/reporters/catch_reporter_registrars.hpp>
+#include <catch2/interfaces/catch_interfaces_config.hpp>
+
+#include "iocore/eventsystem/EventSystem.h"
+#include "iocore/eventsystem/ConfigProcessor.h"
+#include "iocore/utils/diags.i"
+#include "tscore/Layout.h"
+
+#include <atomic>
+#include <chrono>
+#include <thread>
+
+namespace
+{
+std::atomic<int> destroyed{0};
+
+struct TestConfig : public ConfigInfo {
+ explicit TestConfig(int v) : value(v) {}
+ ~TestConfig() override { destroyed.fetch_add(1); }
+ int value;
+};
+
+// A config class shaped like the ones that use scoped_config.
+struct TestConfigClass {
+ static inline unsigned id = 0;
+
+ static unsigned
+ config_id()
+ {
+ return id;
+ }
+ static TestConfig *
+ acquire()
+ {
+ return static_cast<TestConfig *>(configProcessor.get(id));
+ }
+ static void
+ release(TestConfig *config)
+ {
+ configProcessor.release(id, config);
+ }
+ using scoped_config = ConfigProcessor::scoped_config<TestConfigClass,
TestConfig>;
+};
+
+int
+refcount(ConfigInfo *config)
+{
+ return config->refcount();
+}
+
+/// Polls until @a count objects have been destroyed or the deadline passes.
+int
+wait_for_destroyed(int count, std::chrono::milliseconds deadline)
+{
+ auto const until = std::chrono::steady_clock::now() + deadline;
+ while (destroyed.load() < count && std::chrono::steady_clock::now() < until)
{
+ std::this_thread::sleep_for(std::chrono::milliseconds(10));
+ }
+ return destroyed.load();
+}
+} // namespace
+
+TEST_CASE("ConfigProcessor pins and leases", "[iocore][config]")
+{
+ auto *first = new TestConfig(1);
+ TestConfigClass::id = configProcessor.set(0, first, 1);
+ unsigned const id = TestConfigClass::id;
+ int const baseline = refcount(first); // the processor's own reference
+
+ ConfigProcessor::enable_thread_pins();
+
+ SECTION("repeated pinned and scoped reads share one reference per thread")
+ {
+ CHECK(configProcessor.get_pinned(id) == first);
+ CHECK(refcount(first) == baseline + 1);
+ for (int i = 0; i < 10; ++i) {
+ TestConfigClass::scoped_config scoped;
+ CHECK(scoped->value == 1);
+ }
+ CHECK(configProcessor.get_pinned(id) == first);
+ CHECK(refcount(first) == baseline + 1);
+ }
+
+ SECTION("leases count against the thread's pin")
+ {
+ auto lease_a = configProcessor.get_leased(id);
+ auto lease_b = configProcessor.get_leased(id);
+ CHECK(lease_a.get() == first);
+ CHECK(refcount(first) == baseline + 1);
+ }
+
+ SECTION("a replaced configuration is released by the sweep but kept alive by
a lease")
+ {
+ auto lease = configProcessor.get_leased(id);
+
+ auto *second = new TestConfig(2);
+ configProcessor.set(id, second, 1);
+ CHECK(static_cast<TestConfig *>(configProcessor.get_pinned(id))->value ==
2);
+
+ ConfigProcessor::release_stale_pins();
+ // The processor still holds its reference for the release timeout, and
the lease holds the thread's.
+ CHECK(refcount(first) == baseline + 1);
+ CHECK(lease.get() == first);
+
+ // A lease released on another thread still releases the thread's
reference.
+ std::thread([&lease] { lease.reset(); }).join();
+ CHECK(refcount(first) == baseline);
+
+ // Once the processor drops its reference, nothing holds the first
configuration.
+ int const before = destroyed.load();
+ CHECK(wait_for_destroyed(before + 1, std::chrono::seconds(5)) == before +
1);
+ }
+
+ SECTION("threads without pins use the shared refcount")
+ {
+ ConfigInfo *current = configProcessor.get_pinned(id);
+ int const pinned = refcount(current);
+ std::thread([id, current, pinned] {
+ CHECK_FALSE(ConfigProcessor::thread_pins_enabled());
+ {
+ TestConfigClass::scoped_config scoped;
+ CHECK(refcount(current) == pinned + 1);
+ }
+ auto lease = configProcessor.get_leased(id);
+ CHECK(refcount(current) == pinned + 1);
+ lease.reset();
+ CHECK(refcount(current) == pinned);
+ }).join();
+ }
+
+ ConfigProcessor::disable_thread_pins();
+}
+
+struct ConfigPinsListener : Catch::EventListenerBase {
+ using EventListenerBase::EventListenerBase;
+
+ void
+ testRunStarting(Catch::TestRunInfo const &) override
+ {
+ Layout::create();
+ init_diags("", nullptr);
+ RecProcessInit();
+
+ ink_event_system_init(EVENT_SYSTEM_MODULE_PUBLIC_VERSION);
+ eventProcessor.start(2, 1048576);
+
+ EThread *main_thread = new EThread;
+ main_thread->set_specific();
+ }
Review Comment:
The test listener allocates `main_thread` with `new` and never frees it, and
it starts the event processor without any corresponding shutdown/stop. This can
cause persistent background threads and leaks that show up under sanitizers or
when running tests under more strict harnesses. Consider adding a matching
teardown (listener `testRunEnded` / destructor) to stop/join the event threads
and delete `main_thread`.
--
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]
