Destroy replaced configs on ET_TASK

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);
+    }
   }
 }
 

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")