Destroy replaced configs on ET_TASK

[moonchen]

The deferred destruction of a replaced config runs on a network thread today. ConfigProcessor::set() schedules it with schedule_in(), which defaults to ET_CALL, so 60 seconds later an ET_NET thread runs the destructor inside the drain phase of its event loop and is blocked for as long as the config takes to release, which is bounded only by the size of the config.

Move that work to ET_TASK. ConfigProcessor::release() is the only place a config is destroyed, but 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. This schedules the releaser on ET_TASK and hands 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 ConfigProcessor::get() safe, so it should not be tuned just because the destructor moved.

The config debug tag now reports the duration of each destruction and the thread that ran it.

[Copilot]

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Moves destruction of replaced ConfigInfo objects off network threads by scheduling the work onto ET_TASK, and adds debug logging to report destruction duration and thread.

Changes:

• Schedule deferred config release (ConfigInfoReleaser) on ET_TASK instead of the default event type.
• Route last-reference destruction (ConfigProcessor::release) to ET_TASK when possible via a destroyer continuation.
• Add debug logging for per-config destruction time and executing thread.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/iocore/eventsystem/ConfigProcessor.cc:208

• The debug log uses %d for id, but id is an unsigned int. This varargs type mismatch is undefined behavior; use %u to match the argument type.

    Dbg(dbg_ctl_config, "Release config %d %p", id, info);

src/iocore/eventsystem/ConfigProcessor.cc:53

• destroy_config() calls ink_get_hrtime() unconditionally, even though the timing is only used when the config debug tag is enabled. This adds avoidable overhead in the non-debug case; you can early-return when dbg_ctl_config is off and only measure/log when it’s on (and use Dbg() for consistency with the rest of the file).

destroy_config(unsigned int id, ConfigInfo *info)
{
  ink_hrtime start = ink_get_hrtime();

  delete info;

[cmcfarlen]

[approve ci autest 1]

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/iocore/eventsystem/ConfigProcessor.cc:96

• The comment says ET_TASK behaves as ET_CALL until task threads are registered and that an ET_NET caller should then destroy inline, but the code doesn’t actually detect that state. If ET_TASK still maps to ET_CALL here, schedule_imm(..., ET_TASK) can still enqueue the destructor work onto the caller’s (potentially ET_NET) loop, reintroducing the drain-phase blocking this PR aims to avoid. Consider explicitly checking whether ET_TASK threads are registered/available (e.g., via an eventProcessor query for ET_TASK thread count/availability) and returning false when they aren’t, rather than relying on is_event_type(ET_TASK).

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

src/iocore/eventsystem/ConfigProcessor.cc:51

• This uses PRId64 but the shown includes don’t include <inttypes.h>/<cinttypes>. Depending on transitive includes can break portability/builds. Add the appropriate standard header in this file (preferred: <cinttypes> in C++), or use an existing project-provided formatting utility if one is standard in this codebase.

    DbgPrint(dbg_ctl_config, "Destroyed config %u in %" PRId64 " ns on thread %s", id, ink_get_hrtime() - start, thread_name);

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/iocore/eventsystem/ConfigProcessor.cc:88

• destroy_config_on_task_thread() returns false when this_ethread() is nullptr, which forces inline destruction even though eventProcessor.schedule_imm(..., ET_TASK) can be called from non-EThread contexts. This can still block whatever thread is dropping the last ref (including non-event threads). Consider removing the ethread == nullptr early-return and instead only falling back to inline destruction when ET_TASK scheduling is unavailable (e.g., during shutdown / no task threads).

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

tests/gold_tests/config_processor/config_destroy_thread.test.py:29

• This test is unconditionally skipped, so it won’t prevent regressions in automated runs. Instead of a hard skip, consider gating it behind an env/flag (e.g., Condition.EnvironmentVariableSet(...)) or placing it in an explicitly-run slow/soak suite so it can be executed in at least one CI lane.

Test.SkipIf(Condition.true("Test takes over 60 seconds to run."))

[JosiahWI]

Approving, although I do not think it is wise to lock this in as a requirement. I don't think this should have an AuTest. This is a performance optimization and we should be able to change our minds and do something else without waiting for a major release. I would argue that adding the AuTest here is an unofficial guarantee that users can rely on this behavior.

[moonchen]

Approving, although I do not think it is wise to lock this in as a requirement. I don't think this should have an AuTest. This is a performance optimization and we should be able to change our minds and do something else without waiting for a major release. I would argue that adding the AuTest here is an unofficial guarantee that users can rely on this behavior.

I understand your concern. I'm not trying to guarantee this behavior going forward with the autest. My goal with the test is to ensure that the code change does what it's supposed to.

My understanding about autest is that they're not set in stone, even within one major release, and they can be changed when the expected behavior of the code changes.

[cmcfarlen]

Cherry-picked to the 10.2.x branch as 745b2e7 for the 10.2.0 release.