diff --git a/src/iocore/eventsystem/ConfigProcessor.cc b/src/iocore/eventsystem/ConfigProcessor.cc index a49335154ac..adf3cbc69f8 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 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")