This is an automated email from the ASF dual-hosted git repository.
moonchen pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 46be2f5008 Destroy replaced configs on ET_TASK (#13491)
46be2f5008 is described below
commit 46be2f5008dd9026d2bdd6b3f02c5066031605e9
Author: Mo Chen <[email protected]>
AuthorDate: Fri Aug 7 09:55:56 2026 -0500
Destroy replaced configs on ET_TASK (#13491)
* 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 ++++++++++++++++++++-
.../config_processor/config_destroy_thread.test.py | 77 ++++++++++++++++++++++
2 files changed, 151 insertions(+), 2 deletions(-)
diff --git a/src/iocore/eventsystem/ConfigProcessor.cc
b/src/iocore/eventsystem/ConfigProcessor.cc
index a49335154a..adf3cbc69f 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(nullptr), 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();
+
+ // 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;
+ }
+
+ 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);
+ }
}
}
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 0000000000..0909c0f3bd
--- /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")
