Copilot commented on code in PR #12593:
URL: https://github.com/apache/trafficserver/pull/12593#discussion_r4483957452
##########
plugins/lua/ts_lua_http.cc:
##########
@@ -549,6 +551,31 @@ ts_lua_http_get_next_hop_strategy(lua_State *L)
return 1;
}
+namespace
+{
+// A script naming a missing strategy fails on every transaction, so emit at
most one error per interval.
+constexpr TSHRTime strategy_error_interval = TS_HRTIME_SECONDS(60);
+std::atomic<TSHRTime> next_strategy_error{0};
+std::atomic<uint64_t> suppressed_strategy_errors{0};
+
+void
+log_strategy_lookup_failure(char const *name)
+{
+ TSHRTime const now = TShrtime();
+ TSHRTime next = next_strategy_error.load(std::memory_order_relaxed);
+
+ if (now >= next && next_strategy_error.compare_exchange_strong(next, now +
strategy_error_interval, std::memory_order_relaxed)) {
+ uint64_t const suppressed = suppressed_strategy_errors.exchange(0,
std::memory_order_relaxed);
+ TSError("[ts_lua][ts_lua_http_set_next_hop_strategy] Failed get next hop
strategy name '%s' (%" PRIu64
+ " similar errors suppressed)",
+ name, suppressed);
+ } else {
+ suppressed_strategy_errors.fetch_add(1, std::memory_order_relaxed);
+ Dbg(dbg_ctl, "Failed get next hop strategy name '%s'", name);
+ }
Review Comment:
This file uses `PRIu64` in a format string but only adds `<atomic>`;
`PRIu64` is provided by `<cinttypes>` (or `<inttypes.h>`). Add the appropriate
include to avoid build failures on platforms where it isn’t transitively
included.
##########
src/proxy/http/remap/NextHopStrategyFactory.cc:
##########
@@ -167,23 +175,26 @@ NextHopStrategyFactory::createStrategy(const std::string
&name, const NHPolicyTy
NextHopSelectionStrategy *
NextHopStrategyFactory::strategyInstance(const char *name) const
{
- NextHopSelectionStrategy *ps_strategy = nullptr;
-
+ // Called from transaction threads, must not mutate strategy state.
if (!strategies_loaded) {
- NH_Error("no strategy configurations were defined, see definitions in '%s'
file", fn.c_str());
- return nullptr;
- } else {
- auto it = _strategies.find(name);
- if (it == _strategies.end()) {
- // NH_Error("no strategy found for name: %s", name);
- return nullptr;
- } else {
- ps_strategy = it->second;
- ps_strategy->distance = std::distance(_strategies.begin(), it);
+ if (!_not_loaded_logged.exchange(true, std::memory_order_relaxed)) {
+ NH_Error("no strategy configurations were defined, see definitions in
'%s' file", fn.c_str());
}
+ return nullptr;
}
- return ps_strategy;
+ if (name == nullptr) {
+ return nullptr;
+ }
+
+ auto it = _strategies.find(name);
+ return (it == _strategies.end()) ? nullptr : it->second;
+}
+
+bool
+NextHopStrategyFactory::contains(NextHopSelectionStrategy const *strategy)
const
+{
+ return _strategy_ptrs.contains(strategy);
Review Comment:
`std::unordered_set::contains()` is a C++20 API. If this project (or any
supported build configuration) is still compiled as C++17 (the minimum stated
for plugins), this will fail to compile. Use `return
_strategy_ptrs.find(strategy) != _strategy_ptrs.end();` instead, or explicitly
raise the project-wide C++ standard to C++20 where appropriate.
##########
src/api/InkAPI.cc:
##########
@@ -5472,57 +5472,142 @@ TSHttpTxnServerRequestBodySet(TSHttpTxn txnp, char
*buf, int64_t buflength)
s->internal_msg_buffer_fast_allocator_size = -1;
}
-void const *
+TSStrategy
TSHttpTxnNextHopStrategyGet(TSHttpTxn txnp)
{
sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
auto sm = reinterpret_cast<HttpSM const *>(txnp);
- return static_cast<void *>(sm->t_state.next_hop_strategy);
+ return reinterpret_cast<TSStrategy>(sm->t_state.next_hop_strategy);
}
-void
-TSHttpTxnNextHopStrategySet(TSHttpTxn txnp, void const *stratptr)
+TSStrategy
+TSHttpTxnNextHopStrategyFind(TSHttpTxn txnp, const char *name)
{
sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
- // null strategy falls back to parent.config
- // sdk_assert(sdk_sanity_check_null_ptr(strategy) == TS_SUCCESS);
+ sdk_assert(sdk_sanity_check_null_ptr((void *)name) == TS_SUCCESS);
+
+ auto sm = reinterpret_cast<HttpSM const *>(txnp);
+
+ // m_remap is null if no rewrite table existed when the transaction started.
+ if (sm->m_remap.get() == nullptr || sm->m_remap->strategyFactory == nullptr)
{
+ return nullptr;
+ }
- auto sm = reinterpret_cast<HttpSM *>(txnp);
- auto strategy = reinterpret_cast<NextHopSelectionStrategy const *>(stratptr);
+ NextHopSelectionStrategy *const strategy =
sm->m_remap->strategyFactory->strategyInstance(name);
- sm->t_state.next_hop_strategy = const_cast<NextHopSelectionStrategy
*>(strategy);
+ return reinterpret_cast<TSStrategy>(strategy);
}
-char const *
-TSHttpNextHopStrategyNameGet(void const *stratptr)
+void
+TSHttpTxnNextHopStrategySet(TSHttpTxn txnp, TSStrategy stratptr)
{
- char const *name = nullptr;
- if (nullptr != stratptr) {
- auto strategy = reinterpret_cast<NextHopSelectionStrategy const
*>(stratptr);
- name = strategy->strategy_name.c_str();
+ sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
+
+ auto sm = reinterpret_cast<HttpSM *>(txnp);
+
+ // A null strategy falls back to parent.config.
+ if (stratptr == nullptr) {
+ sm->t_state.next_hop_strategy = nullptr;
+ sm->m_strategy_owner.reset();
+ return;
}
- return name;
+ // The core dereferences this pointer during parent selection, so reject
+ // handles not owned by this transaction's factory (e.g. cached across a
reload).
+ auto const candidate = reinterpret_cast<NextHopSelectionStrategy
*>(stratptr);
+ if (sm->m_remap == nullptr || sm->m_remap->strategyFactory == nullptr ||
!sm->m_remap->strategyFactory->contains(candidate)) {
+ SiteThrottledError("%s: strategy %p is not present in the live strategy
factory; ignoring", __func__, stratptr);
+ Dbg(dbg_ctl_parent_select, "%s: strategy %p is not present in the live
strategy factory; ignoring", __func__, stratptr);
Review Comment:
These log calls use `%p` but pass `TSStrategy` (`struct tsapi_strategy *`)
directly. In varargs/printf-style functions, `%p` requires an argument of type
`void*`; passing another pointer type is undefined behavior. Cast the
strategy/handle to `void*` (or `void const*` with a corresponding format
strategy) before logging.
##########
src/api/InkAPI.cc:
##########
@@ -5472,57 +5472,142 @@ TSHttpTxnServerRequestBodySet(TSHttpTxn txnp, char
*buf, int64_t buflength)
s->internal_msg_buffer_fast_allocator_size = -1;
}
-void const *
+TSStrategy
TSHttpTxnNextHopStrategyGet(TSHttpTxn txnp)
{
sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
auto sm = reinterpret_cast<HttpSM const *>(txnp);
- return static_cast<void *>(sm->t_state.next_hop_strategy);
+ return reinterpret_cast<TSStrategy>(sm->t_state.next_hop_strategy);
}
-void
-TSHttpTxnNextHopStrategySet(TSHttpTxn txnp, void const *stratptr)
+TSStrategy
+TSHttpTxnNextHopStrategyFind(TSHttpTxn txnp, const char *name)
{
sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
- // null strategy falls back to parent.config
- // sdk_assert(sdk_sanity_check_null_ptr(strategy) == TS_SUCCESS);
+ sdk_assert(sdk_sanity_check_null_ptr((void *)name) == TS_SUCCESS);
+
+ auto sm = reinterpret_cast<HttpSM const *>(txnp);
+
+ // m_remap is null if no rewrite table existed when the transaction started.
+ if (sm->m_remap.get() == nullptr || sm->m_remap->strategyFactory == nullptr)
{
+ return nullptr;
+ }
- auto sm = reinterpret_cast<HttpSM *>(txnp);
- auto strategy = reinterpret_cast<NextHopSelectionStrategy const *>(stratptr);
+ NextHopSelectionStrategy *const strategy =
sm->m_remap->strategyFactory->strategyInstance(name);
- sm->t_state.next_hop_strategy = const_cast<NextHopSelectionStrategy
*>(strategy);
+ return reinterpret_cast<TSStrategy>(strategy);
}
-char const *
-TSHttpNextHopStrategyNameGet(void const *stratptr)
+void
+TSHttpTxnNextHopStrategySet(TSHttpTxn txnp, TSStrategy stratptr)
{
- char const *name = nullptr;
- if (nullptr != stratptr) {
- auto strategy = reinterpret_cast<NextHopSelectionStrategy const
*>(stratptr);
- name = strategy->strategy_name.c_str();
+ sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
+
+ auto sm = reinterpret_cast<HttpSM *>(txnp);
+
+ // A null strategy falls back to parent.config.
+ if (stratptr == nullptr) {
+ sm->t_state.next_hop_strategy = nullptr;
+ sm->m_strategy_owner.reset();
+ return;
}
- return name;
+ // The core dereferences this pointer during parent selection, so reject
+ // handles not owned by this transaction's factory (e.g. cached across a
reload).
+ auto const candidate = reinterpret_cast<NextHopSelectionStrategy
*>(stratptr);
+ if (sm->m_remap == nullptr || sm->m_remap->strategyFactory == nullptr ||
!sm->m_remap->strategyFactory->contains(candidate)) {
+ SiteThrottledError("%s: strategy %p is not present in the live strategy
factory; ignoring", __func__, stratptr);
+ Dbg(dbg_ctl_parent_select, "%s: strategy %p is not present in the live
strategy factory; ignoring", __func__, stratptr);
+ return;
+ }
+
+ sm->t_state.next_hop_strategy = candidate;
+ sm->m_strategy_owner = sm->m_remap;
}
-void const *
-TSHttpTxnNextHopNamedStrategyGet(TSHttpTxn txnp, const char *name)
+TSStrategy
+TSRemapNextHopStrategyFind(const char *name)
{
- sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
sdk_assert(sdk_sanity_check_null_ptr((void *)name) == TS_SUCCESS);
- auto sm = reinterpret_cast<HttpSM const *>(txnp);
+ auto const um = url_mapping::loading_instance();
+ if (um == nullptr) {
+ SiteThrottledError("%s: must be called during remap plugin initialization
(no loading remap rule); returning nullptr",
+ __func__);
+ return nullptr;
+ }
- sdk_assert(sdk_sanity_check_null_ptr((void *)sm->m_remap.get()) ==
TS_SUCCESS);
- sdk_assert(sdk_sanity_check_null_ptr((void *)sm->m_remap->strategyFactory)
== TS_SUCCESS);
+ // No strategies.yaml is loaded.
+ if (um->strategyFactory == nullptr) {
+ return nullptr;
+ }
+
+ return
reinterpret_cast<TSStrategy>(um->strategyFactory->strategyInstance(name));
+}
- // HttpSM has a reference count handle to UrlRewrite which has a
- // pointer to NextHopStrategyFactory
- NextHopSelectionStrategy const *const strat =
sm->m_remap->strategyFactory->strategyInstance(name);
+void
+TSRemapNextHopStrategySet(TSStrategy strategy)
+{
+ auto const um = url_mapping::loading_instance();
+ if (um == nullptr) {
+ SiteThrottledError("%s: must be called during remap plugin initialization
(no loading remap rule); ignoring", __func__);
+ return;
+ }
- return static_cast<void const *>(strat);
+ if (strategy == nullptr) {
+ um->strategy = nullptr;
+ return;
+ }
+
+ auto const candidate = reinterpret_cast<NextHopSelectionStrategy
*>(strategy);
+ if (um->strategyFactory == nullptr ||
!um->strategyFactory->contains(candidate)) {
+ TSError("%s: strategy %p is not present in the loading strategy factory;
ignoring", __func__, strategy);
Review Comment:
These log calls use `%p` but pass `TSStrategy` (`struct tsapi_strategy *`)
directly. In varargs/printf-style functions, `%p` requires an argument of type
`void*`; passing another pointer type is undefined behavior. Cast the
strategy/handle to `void*` (or `void const*` with a corresponding format
strategy) before logging.
##########
src/api/InkAPITest.cc:
##########
@@ -3193,23 +3193,21 @@ checkHttpTxnClientProtocolStackContains(SocketTest
*test, void *data)
return TS_EVENT_CONTINUE;
}
-// This func is called by us from mytest_handler to check for
TSHttpTxnNextStrategyGet
+// This func is called by us from mytest_handler to check
TSHttpTxnNextHopStrategySet/Get.
+// No strategies are loaded here, so only rejection of an invalid handle is
exercised.
static int
-checkHttpTxnNextHopStrategyGet(SocketTest *test, void *data)
+checkHttpTxnNextHopStrategySetGet(SocketTest *test, void *data)
{
TSHttpTxn txnp = static_cast<TSHttpTxn>(data);
- // this is an invalid pointer but the contents don't matter for this test.
- void const *const exp = reinterpret_cast<void *>(0x01);
+ test->test_next_hop_strategy_get = true;
- void const *const strategy = TSHttpTxnNextHopStrategyGet(txnp);
- if (strategy == exp) {
- test->test_next_hop_strategy_get = true;
- SDK_RPRINT(test->regtest, "TSHttpTxnNextHopStrategyGet", "TestCase1",
TC_PASS, "ok");
+ TSHttpTxnNextHopStrategySet(txnp, reinterpret_cast<TSStrategy>(0x01));
+ if (TSStrategy const strategy = TSHttpTxnNextHopStrategyGet(txnp); strategy
== nullptr) {
+ SDK_RPRINT(test->regtest, "TSHttpTxnNextHopStrategySet", "TestCase1",
TC_PASS, "ok");
} else {
test->test_next_hop_strategy_get = false;
- SDK_RPRINT(test->regtest, "TSHttpTxnNextHopStrategyGet", "TestCase1",
TC_FAIL, "Value's Mismatch [expected '%jx', got '%jx'",
- exp, strategy);
+ SDK_RPRINT(test->regtest, "TSHttpTxnNextHopStrategySet", "TestCase1",
TC_FAIL, "Invalid handle accepted [got '%p']", strategy);
Review Comment:
`SDK_RPRINT` is printf-style, and `%p` requires a `void*` argument. Passing
`TSStrategy` (a distinct pointer type) through varargs is undefined behavior.
Cast `strategy` to `void*` when printing.
--
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]
