From e355bf8d92f4f436b1e8d4988a6f7ed3998ff524 Mon Sep 17 00:00:00 2001 From: Mo Chen Date: Tue, 4 Aug 2026 11:37:24 -0500 Subject: [PATCH 1/4] Destroy replaced configs on ET_TASK ConfigProcessor::set() scheduled the deferred destruction of the replaced config with schedule_in(), which defaults to ET_CALL, so a network thread ran the destructor 60 seconds later inside the drain phase of its event loop. The destructor blocks that thread for as long as the config takes to release, which is bounded only by the size of the config. ConfigProcessor::release() is the only place a config is destroyed, and two callers reach it: the releaser at 60 seconds, which destroys the config whenever nothing else still holds a reference, and a transaction that outlived the releaser and drops the last reference itself. Schedule the releaser on ET_TASK, and hand the destructor from the transaction path to ET_TASK as well, so neither can block a network thread. The 60 second wait is unchanged. Shortening it would narrow the window that makes the load-then-increment in get() safe. The config debug tag now reports the duration of each destruction and the thread that ran it. --- src/iocore/eventsystem/ConfigProcessor.cc | 76 ++++++++++++++++++++++- 1 file changed, 74 insertions(+), 2 deletions(-) diff --git a/src/iocore/eventsystem/ConfigProcessor.cc b/src/iocore/eventsystem/ConfigProcessor.cc index a49335154ac..31e5cbe4e61 100644 --- a/src/iocore/eventsystem/ConfigProcessor.cc +++ b/src/iocore/eventsystem/ConfigProcessor.cc @@ -22,7 +22,10 @@ */ #include "iocore/eventsystem/ConfigProcessor.h" +#include "iocore/eventsystem/EThread.h" +#include "iocore/eventsystem/Tasks.h" #include "tscore/ink_atomic.h" +#include "tscore/ink_thread.h" #if TS_HAS_TESTS #include "tscore/TestBox.h" #endif @@ -34,8 +37,69 @@ namespace DbgCtl dbg_ctl_config{"config"}; +void +destroy_config(unsigned int id, ConfigInfo *info) +{ + ink_hrtime start = ink_get_hrtime(); + + delete info; + + if (dbg_ctl_config.on()) { + char thread_name[MAX_THREAD_NAME_LENGTH] = {}; + + ink_get_thread_name(thread_name, sizeof(thread_name)); + DbgPrint(dbg_ctl_config, "Destroyed config %u in %" PRId64 " ns on thread %s", id, ink_get_hrtime() - start, thread_name); + } +} + +/// Runs the destructor of a detached ConfigInfo on ET_TASK. +class ConfigInfoDestroyer : public Continuation +{ +public: + ConfigInfoDestroyer(unsigned int id, ConfigInfo *info) : Continuation(new_ProxyMutex()), m_id(id), m_info(info) + { + SET_HANDLER(&ConfigInfoDestroyer::handle_event); + } + + int + handle_event(int /* event ATS_UNUSED */, void * /* edata ATS_UNUSED */) + { + destroy_config(m_id, m_info); + delete this; + return EVENT_DONE; + } + +private: + unsigned int m_id; + ConfigInfo *m_info; +}; + +/// Hand a detached ConfigInfo to ET_TASK for destruction. Returns false when the caller has to +/// destroy it itself. +bool +destroy_config_on_task_thread(unsigned int id, ConfigInfo *info) +{ + EThread *ethread = this_ethread(); + + // There is nowhere to send this unless the task threads are up. Before they are registered ET_TASK + // is ET_CALL, and between registration and spawning the group is still empty. + if (ethread == nullptr || ethread->is_event_type(ET_TASK) || eventProcessor.thread_group[ET_TASK]._count == 0) { + return false; + } + + ConfigInfoDestroyer *destroyer = new ConfigInfoDestroyer(id, info); + + if (eventProcessor.schedule_imm(destroyer, ET_TASK) == nullptr) { + // The event system is shutting down and will never run the destroyer. + delete destroyer; + return false; + } + + return true; } +} // namespace + class ConfigInfoReleaser : public Continuation { public: @@ -94,7 +158,10 @@ ConfigProcessor::set(unsigned int id, ConfigInfo *info, unsigned timeout_secs) // The ConfigInfoReleaser now takes our refcount, but // some other thread might also have one ... ink_assert(old_info->refcount() > 0); - eventProcessor.schedule_in(new ConfigInfoReleaser(id, old_info), HRTIME_SECONDS(timeout_secs)); + // Destroying a config releases everything it owns - a replaced certificate table takes its whole + // certificate set with the chains, keys and staples. Run it on ET_TASK, which already carries the + // config load, so the cost cannot land on a network event loop. + eventProcessor.schedule_in(new ConfigInfoReleaser(id, old_info), HRTIME_SECONDS(timeout_secs), ET_TASK); } return id; @@ -140,7 +207,12 @@ ConfigProcessor::release(unsigned int id, ConfigInfo *info) // When we release, we should already have replaced this object in the index. Dbg(dbg_ctl_config, "Release config %d %p", id, info); ink_release_assert(info != this->infos[idx]); - delete info; + + // The releaser runs on ET_TASK, but a transaction that outlived it drops the last reference on + // its own thread, which serves network connections. + if (!destroy_config_on_task_thread(id, info)) { + destroy_config(id, info); + } } } From f60454226d8df7aa59d7d68d48be7c2a93b41199 Mon Sep 17 00:00:00 2001 From: Mo Chen Date: Wed, 5 Aug 2026 10:32:17 -0500 Subject: [PATCH 2/4] Drop the ET_TASK group count check destroy_config_on_task_thread() also refused the hand off when thread_group[ET_TASK]._count read 0, to cover the gap between tasksProcessor.register_event_type() and tasksProcessor.start(). A config cannot be destroyed in that gap. release() reaches the destructor only when the reference count drops to zero, and the processor gives up its own reference only through the ConfigInfoReleaser that set() schedules timeout_secs ahead, 60 seconds for every caller in the tree. Both spans where the count reads 0 are straight-line startup code on the main thread, so nothing can drop a reference inside them. The check was also the wrong test for that gap. spawn_event_threads() stores _thread[i] inside its construction loop and sets _count after the loop, so for most of the gap the count still reads 0 while _thread[0] is already a usable thread. --- src/iocore/eventsystem/ConfigProcessor.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/iocore/eventsystem/ConfigProcessor.cc b/src/iocore/eventsystem/ConfigProcessor.cc index 31e5cbe4e61..82744d51113 100644 --- a/src/iocore/eventsystem/ConfigProcessor.cc +++ b/src/iocore/eventsystem/ConfigProcessor.cc @@ -81,9 +81,9 @@ destroy_config_on_task_thread(unsigned int id, ConfigInfo *info) { EThread *ethread = this_ethread(); - // There is nowhere to send this unless the task threads are up. Before they are registered ET_TASK - // is ET_CALL, and between registration and spawning the group is still empty. - if (ethread == nullptr || ethread->is_event_type(ET_TASK) || eventProcessor.thread_group[ET_TASK]._count == 0) { + // ET_TASK is ET_CALL until the task threads are registered, so before that point an ET_NET caller + // destroys the config on its own thread. + if (ethread == nullptr || ethread->is_event_type(ET_TASK)) { return false; } From f380f022cbfb6805c52d503efb9afd0cc8346560 Mon Sep 17 00:00:00 2001 From: Mo Chen Date: Wed, 5 Aug 2026 10:32:29 -0500 Subject: [PATCH 3/4] Drop the mutex from ConfigInfoDestroyer The destroyer is created and scheduled in one breath and nothing else can reach it, so the mutex serialized nothing and cost a ProxyMutex allocation per destroyed config. EThread::process_event() treats a null event mutex as already acquired, which is how FreeCallContinuation, FreerContinuation and DereferContinuation already run. --- src/iocore/eventsystem/ConfigProcessor.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/iocore/eventsystem/ConfigProcessor.cc b/src/iocore/eventsystem/ConfigProcessor.cc index 82744d51113..adf3cbc69f8 100644 --- a/src/iocore/eventsystem/ConfigProcessor.cc +++ b/src/iocore/eventsystem/ConfigProcessor.cc @@ -56,7 +56,7 @@ destroy_config(unsigned int id, ConfigInfo *info) class ConfigInfoDestroyer : public Continuation { public: - ConfigInfoDestroyer(unsigned int id, ConfigInfo *info) : Continuation(new_ProxyMutex()), m_id(id), m_info(info) + ConfigInfoDestroyer(unsigned int id, ConfigInfo *info) : Continuation(nullptr), m_id(id), m_info(info) { SET_HANDLER(&ConfigInfoDestroyer::handle_event); } From 4d6597ce3c82061f36def5171a55d38822e82141 Mon Sep 17 00:00:00 2001 From: Mo Chen Date: Wed, 5 Aug 2026 13:46:07 -0500 Subject: [PATCH 4/4] Add an autest for config destruction on ET_TASK The destruction path had no coverage. Autest never reached it: a config is destroyed only after ConfigProcessor::set() releases the config it replaced, and that timeout is a compile time constant of 60 seconds, which outlives every existing test. The test replaces two configs, one through the reload framework and one from a network thread by way of a record update, waits out the timeout and checks that the debug line reports an ET_TASK thread for both. It also fails if any config is destroyed on a network thread. The 60 second wait keeps it out of CI, so it is skipped the same way post_slow_server is. --- .../config_destroy_thread.test.py | 77 +++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100644 tests/gold_tests/config_processor/config_destroy_thread.test.py diff --git a/tests/gold_tests/config_processor/config_destroy_thread.test.py b/tests/gold_tests/config_processor/config_destroy_thread.test.py new file mode 100644 index 00000000000..0909c0f3bd3 --- /dev/null +++ b/tests/gold_tests/config_processor/config_destroy_thread.test.py @@ -0,0 +1,77 @@ +''' +Verify that a replaced config is destroyed on an ET_TASK thread. +''' +# 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. + +import os + +Test.Summary = ''' +Verify that a replaced config is destroyed on an ET_TASK thread. +''' + +# ConfigProcessor::set() waits CONFIG_PROCESSOR_RELEASE_SECS before it releases the config that it +# replaced. That timeout is a compile time constant of 60 seconds, so this test needs more than a +# minute of wall clock and does not run in CI. Comment out the next line to run it. +Test.SkipIf(Condition.true("Test takes over 60 seconds to run.")) + +Test.ContinueOnFail = True + +ts = Test.MakeATSProcess("ts") + +ts.Disk.records_config.update({ + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'config', +}) + +ts.Disk.remap_config.AddLine('map / http://127.0.0.1:8080') + +config_dir = ts.Variables.CONFIGDIR + +# Two replacements, reached two different ways. Touching parent.config replaces ParentConfigParams +# through the reload framework, which runs on ET_TASK. Changing an HTTP record replaces +# HttpConfigParams from a network thread, which is the case this test is really about. Neither old +# config is referenced once the test stops sending traffic, so both reach a zero reference count and +# are destroyed when the release timeout expires. +tr = Test.AddTestRun("Mark parent.config for reload") +tr.Processes.Default.StartBefore(ts) +tr.Processes.Default.Command = f"sleep 3 && touch {os.path.join(config_dir, 'parent.config')} && sleep 1" +tr.Processes.Default.ReturnCode = 0 +tr.StillRunningAfter = ts + +Test.AddConfigReload(ts, expect="any", token="config_destroy_thread") + +tr = Test.AddTestRun("Replace the HTTP config from a network thread") +tr.DelayStart = 3 +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Command = "traffic_ctl config set proxy.config.http.response_server_str probe && sleep 3" +tr.Processes.Default.ReturnCode = 0 +tr.StillRunningAfter = ts + +tr = Test.AddTestRun("Wait for the release timeout to expire") +tr.DelayStart = 3 +tr.Processes.Default.Command = "sleep 80" +tr.Processes.Default.ReturnCode = 0 +tr.TimeOut = 150 +tr.StillRunningAfter = ts + +# The releaser runs on ET_TASK, so it destroys the replaced config there. +ts.Disk.traffic_out.Content = Testers.ContainsExpression( + r"Destroyed config \d+ in \d+ ns on thread \[ET_TASK", "a replaced config should be destroyed on a task thread") + +# Destroying a config on a network thread is the regression this test guards against. +ts.Disk.traffic_out.Content += Testers.ExcludesExpression( + r"Destroyed config \d+ in \d+ ns on thread \[ET_NET", "no config should be destroyed on a network thread")