cmcfarlen commented on code in PR #14005:
URL: https://github.com/apache/trafficserver/pull/14005#discussion_r4494810146
##########
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:
`HttpConfig::release()` calls `ConfigProcessor::release()`, which already
ignores a null pointer (`if (info && info->refcount_dec() == 0)`). And the
HttpConfig slot is always set once `HttpConfig::startup()` has run, so no
change here.
##########
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:
Same as the other thread: `ConfigProcessor::release()` is null-safe, so the
destructor's fallback path is fine with an empty lease.
##########
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:
`ParentConfig` doesn't have a `config_id()` accessor, and its own
`acquire()`/`release()` in ParentSelection.h read `m_id` directly. The
accessors in this PR exist for the `scoped_config` template, which
`ParentConfig` doesn't use, so I've left it consistent with that header.
##########
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:
This single hook set is deliberate: it lets binaries that don't link
ConfigProcessor (such as `test_HPACK`) skip it entirely, and nothing else
installs hooks today. A registration API can come with the second user. For
now, 2827cc01a2 asserts that the hooks are unset before ConfigProcessor
installs its own, so a future clash fails loudly.
##########
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:
Added `static_assert(MAX_CONFIGS <= 256)` next to the slot list in
2827cc01a2.
##########
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:
This uses the same listener setup as the other eventsystem unit tests
(test_Event, test_Lock, test_DeferredCounters): they start the event processor
once per process and let process exit end it. I've kept it consistent with them.
--
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]
